On Mon, 16 Jul 2012, Ming Lei wrote:
> On Wed, Jul 11, 2012 at 11:21 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > This patch (as1572) begins the conversion of ehci-hcd over to using
> > high-resolution timers rather than old-fashioned low-resolution kernel
> > timers. This reduces overhead caused by timer roundoff on systems
> > where HZ is smaller than 1000. Also, the new timer framework
> > introduced here is much more logical and easily extended than the
> > ad-hoc approach ehci-hcd currently uses for timers.
> >
> > An hrtimer structure is added to ehci_hcd, along with a bitflag array
> > and an array of ktime_t values, to keep track of which timing events
> > are pending and what their expiration times are.
> >
> > Only the infrastructure for the timing operations is added in this
> > patch. Later patches will add routines for handling each of the
> > various timing events the driver needs. In some cases the new hrtimer
> > handlers will replace the existing handlers for ehci-hcd's kernel
> > timers; as this happens the old timers will be removed. In other
> > cases the new timing events will replace busy-wait loops.
Thanks for the review.
...
> > +/*
> > + * EHCI timer support... Now using hrtimers.
> > + *
> > + * Lots of different events are triggered from ehci->hrtimer. Whenever
> > + * the timer routine runs, it checks each possible event; events that are
> > + * currently enabled and whose expiration time has passed get handled.
> > + * The set of enabled events is stored as a collection of bitflags in
> > + * ehci->enabled_hrtimer_events, and they are numbered in order of
> > + * increasing delay values (ranging between 1 ms and 100 ms).
> > + *
> > + * Rather than implementing a sorted list or tree of all pending events,
>
> Another candidate may be to use one standalone hrtimer for each event.
Yes. But this way is almost as simple and it requires fewer resources.
For each event, I have to store only the expiration time rather than an
entire hrtimer structure.
> > + * we keep track only of the lowest-numbered pending event, in
> > + * ehci->next_hrtimer_event. Whenever ehci->hrtimer gets restarted, its
> > + * expiration time is set to the timeout value for this event.
> > + *
> > + * As a result, events might not get handled right away; the actual delay
> > + * could be anywhere up to twice the requested delay. This doesn't
> > + * matter, because none of the events are especially time-critical. The
> > + * ones that matter most all have a delay of 1 ms, so they will be
> > + * handled after 2 ms at most, which is okay. In addition to this, we
> > + * allow for an expiration range of 1 ms.
> > + */
> > +
> > +/*
> > + * Delay lengths for the hrtimer event types.
> > + * Keep this list sorted by delay length, in the same order as
> > + * the event types indexed by enum ehci_hrtimer_event in ehci.h.
> > + */
> > +static unsigned event_delays_ns[] = {
>
> According to your design, the time in the above array can't be changed,
> so it should be marked as 'const' to reflect the design principle.
Right. I meant to do that -- in fact, I think an earlier version of
the patchset did include "const" -- but somehow it got lost. This can
be a future enhancement.
> > +};
> > +
> > +/* Enable a pending hrtimer event */
> > +static void ehci_enable_event(struct ehci_hcd *ehci, unsigned event,
> > + bool resched)
> > +{
> > + ktime_t *timeout = &ehci->hr_timeouts[event];
> > +
> > + if (resched)
> > + *timeout = ktime_add(ktime_get(),
> > + ktime_set(0, event_delays_ns[event]));
> > + ehci->enabled_hrtimer_events |= (1 << event);
> > +
> > + /* Track only the lowest-numbered pending event */
> > + if (event < ehci->next_hrtimer_event) {
> > + ehci->next_hrtimer_event = event;
> > + hrtimer_start_range_ns(&ehci->hrtimer, *timeout,
> > + NSEC_PER_MSEC, HRTIMER_MODE_ABS);
>
> The *timeout may have been expired for some time, so can the timer handler
> still be called after return from hrtimer_start_range_ns? If not, it
> may be a problem.
After return would be okay. There would be a problem if
hrtimer_start_range_ns tried to call the handler directly, because the
handler acquires ehci->lock and so it would self-deadlock.
I can't find any place where the hrtimer documentation explains what
happens when you set an expiration time that is in the past.
> > + }
> > +}
> > +
> > +
> > +/*
> > + * Handler functions for the hrtimer event types.
> > + * Keep this array in the same order as the event types indexed by
> > + * enum ehci_hrtimer_event in ehci.h.
> > + */
> > +static void (*event_handlers[])(struct ehci_hcd *) = {
> > +};
> > +
> > +static enum hrtimer_restart ehci_hrtimer_func(struct hrtimer *t)
> > +{
> > + struct ehci_hcd *ehci = container_of(t, struct ehci_hcd, hrtimer);
> > + ktime_t now;
> > + unsigned long events;
> > + unsigned long flags;
> > + unsigned e;
> > +
> > + spin_lock_irqsave(&ehci->lock, flags);
> > +
> > + events = ehci->enabled_hrtimer_events;
> > + ehci->enabled_hrtimer_events = 0;
> > + ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
> > +
> > + /*
> > + * Check each pending event. If its time has expired, handle
> > + * the event; otherwise re-enable it.
> > + */
> > + now = ktime_get();
>
> It is better to move the above line after the next line since the event_hander
> may consume some timer.
It probably won't consume very much time. Most of the handlers run in
a lot less than 1 ms, and the expiration times are allowed 1 ms of
slack anyway. Even if a handler does take a long time, all that could
happen is we call some handler a little late. These expiration times
are not meant to be very precise, in any case.
Doing it this way reduces the overhead, by avoiding multiple calls to
ktime_get.
> > + for_each_set_bit(e, &events, EHCI_HRTIMER_NUM_EVENTS) {
> > + if (now.tv64 >= ehci->hr_timeouts[e].tv64)
> > + event_handlers[e](ehci);
> > + else
> > + ehci_enable_event(ehci, e, false);
> > + }
> > +
> > + spin_unlock_irqrestore(&ehci->lock, flags);
> > + return HRTIMER_NORESTART;
> > +}
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
