Re: [PATCH 8/25] USB: EHCI: introduce high-res timer

[Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>]

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