On Sat, Jun 22, 2013 at 4:16 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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...
Thanks for your review.
>
>> 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.
OK.
>
>> 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.
OK, I will move it into ehci_qh_alloc().
>
>> @@ -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.
Considered without this patch, enabling BH_HCD isn't very good
for interrupt transfer, I will remove the check in 6/6.
>
>> +
>> + ehci->enabled_hrtimer_events &= ~(1 << event);
>> + if (!ehci->enabled_hrtimer_events)
>
> Here you need to add:
>
> ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
Good catch.
>
>> + hrtimer_cancel(&ehci->hrtimer);
>> +}
>
> This could also be useful for the IAA watchdog timer in the STS_IAA
> code in ehci_irq().
Right, it might be useful for any ehci timers which won't expire at
most of times, especially when the timer's frequency is high.
>
>> @@ -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.
OK.
> Otherwise this seems to be about right.
Thanks,
--
Ming Lei
--
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
