[RFC PATCH v1 5/6] USB: EHCI: improve interrupt qh unlink

[Alan Stern]

On Thu, 20 Jun 2013, Ming Lei wrote:

> IMO, there is no any side effect when we change the state to
> QH_STATE_UNLINK_WAIT and later change back to QH_STATE_LINKED
> later under this situation.

I don't like the idea of changing an invariant.

> The reason why I use QH_STATE_UNLINK_WAIT here is for avoiding
> unnecessary CPU wakeup.  Without the state, the unlink wait timer
> is still triggered to check if there are intr QHs to be unlinked, but in most of
> situations, there aren't QHs to be unlinked since tasklet is surely run
> before the wait timer is expired. So generally, without knowing the wait state,
> CPU wakeup events may be introduced unnecessarily.

Avoiding unnecessary timer events is a good thing, but I don't 
understand how it is connected with QH_STATE_UNLINK_WAIT.  Doesn't this
revised patch avoid the events without using this state?

Besides, don't you already know the wait state by checking whether 
qh->unlink_node is empty?

> Considered that the interval of interrupt endpoint might be very
> small(e.g. 125us,
> 1ms) and some devices have very frequent intr event, I think we
> need to pay attention to the problem.

Yes, we do.  The hrtimer code in ehci-timer is written specifically to 
handle that sort of situation.

> Even on async QH situation, we might need to consider the problem too
> since some application(eg. bulk only mass storage on xhci) may have
> thousands of interrupts per seconds during data transfer, so CPU wakeup
> events could be increased much by letting wait timer expire unnecessarily.

I suspect it's the other way around.  Letting the timer expire
_reduces_ the amount of work, because we don't have to start and stop
the timer as often.

It's a tradeoff.  One approach starts and cancels the timer N times
(where N can be fairly large); the other approach starts the timer
once and lets it expire, and then the handler routine does almost no
work.  Which approach uses more CPU time?  I don't know; I haven't
measured it.

> Also the async QH unlink approach has another disadvantage:
> 
> - if there are several QHs which are become empty during one wait period,
> the hrtimer has to be scheduled several times for starting unlink one qh
> each time.

That's because the driver unlinks only one async QH at a time.  It is
unavoidable.  In earlier kernels the driver would unlink multiple async
QHs simultaneously, and it needed to schedule the timer only once.  
For some reason (I still don't understand why), this did not work on
some systems.

>  And after introducing the unlink wait list like the patch, we only
> need schedule the hrtimer one time for unlinking all these empty QHs.

The async code used to work that way, before I changed it to unlink
only one async QH at a time.

> Finally, unlink wait for intr qh is only needed when the qh is completed
> right now, and other cases(eg. unlink) needn't the wait.

Right.

> The attached patch removes the QH_STATE_UNLINK_WAIT, and can
> avoid the above disadvantages of async QH unlink, could you comment
> on the new patch?

Okay...

> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 246e124..35b4148 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -304,7 +304,8 @@ static void end_unlink_async(struct ehci_hcd *ehci);
>  static void unlink_empty_async(struct ehci_hcd *ehci);
>  static void unlink_empty_async_suspended(struct ehci_hcd *ehci);
>  static void ehci_work(struct ehci_hcd *ehci);
> -static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh);
> +static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
> +			      bool wait);

Adding a "wait" argument is a bad approach.  You should create a new 
function: start_unlink_intr_wait() or something like that.  After all, 
it is used in only one place.

> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index acff5b8..5dfda56 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -548,6 +548,9 @@ static void qh_link_periodic(struct ehci_hcd
> *ehci, struct ehci_qh *qh)
> 
>  	list_add(&qh->intr_node, &ehci->intr_qh_list);
> 
> +	/* unlink need this node to be initialized */
> +	INIT_LIST_HEAD(&qh->unlink_node);

This should be done only once, when the QH is created.  And the comment 
is unnecessary.

> @@ -98,6 +99,17 @@ static void ehci_enable_event(struct ehci_hcd
> *ehci, unsigned event,
>  	}
>  }
> 
> +/* Warning: don't call this function from hrtimer handler context */
> +static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
> +{
> +	if (!hcd_giveback_urb_in_bh(ehci_to_hcd(ehci)))
> +		return;

This test looks really ugly, and it won't be needed after the driver
switches over to tasklets.  Of course, there's a problem before we
switch over: this routine will be called by an interrupt URB
submission, which could occur during a giveback in the timer handler
context.

Perhaps the best approach is to leave this routine out until after the 
driver switches over.

> +
> +	ehci->enabled_hrtimer_events &= ~(1 << event);
> +	if (!ehci->enabled_hrtimer_events)

Here you need to add:

		ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;

> +		hrtimer_cancel(&ehci->hrtimer);
> +}

This could also be useful for the IAA watchdog timer in the STS_IAA
code in ehci_irq().

> @@ -215,6 +227,36 @@ static void ehci_handle_controller_death(struct
> ehci_hcd *ehci)
>  	/* Not in process context, so don't try to reset the controller */
>  }
> 
> +/* start to unlink interrupt QHs  */
> +static void ehci_handle_start_intr_unlinks(struct ehci_hcd *ehci)
> +{
> +	bool		stopped = (ehci->rh_state < EHCI_RH_RUNNING);
> +
> +	/*
> +	 * Process all the QHs on the intr_unlink list that were added
> +	 * before the current unlink cycle began.  The list is in
> +	 * temporal order, so stop when we reach the first entry in the
> +	 * current cycle.  But if the root hub isn't running then
> +	 * process all the QHs on the list.
> +	 */
> +	while (!list_empty(&ehci->intr_unlink_wait)) {
> +		struct ehci_qh	*qh;
> +
> +		qh = list_first_entry(&ehci->intr_unlink_wait,
> +				      struct ehci_qh, unlink_node);
> +		if (!stopped &&
> +		    qh->unlink_cycle == ehci->intr_unlink_wait_cycle)
> +			break;

The style in this driver is to add two tab stops to continuation lines, 
not to line them up with respect to the previous line.

Otherwise this seems to be about right.

Alan Stern