Re: [PATCH] Revert "usb: dwc3: gadget: use allocated/queued reqs for LST bit"

Re: [PATCH] Revert "usb: dwc3: gadget: use allocated/queued reqs for LST bit"

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5856 bytes --]


Hi,

Ville Syrjälä <ville.syrjala@linux.intel.com> writes:
> On Thu, Oct 06, 2016 at 12:08:26PM +0300, Ville Syrjälä wrote:
>> On Thu, Oct 06, 2016 at 10:36:09AM +0300, Felipe Balbi wrote:
>> > 
>> > Hi,
>> > 
>> > Felipe Balbi <felipe.balbi@linux.intel.com> writes:
>> <snip>
>> > Okay, I have found a regression on dwc3 and another patch follows:
>> > 
>> > commit 5e1a2af3e46248c55098cdae643c4141851b703e
>> > Author: Felipe Balbi <felipe.balbi@linux.intel.com>
>> > Date:   Wed Oct 5 14:24:37 2016 +0300
>> > 
>> >     usb: dwc3: gadget: properly account queued requests
>> >     
>> >     Some requests could be accounted for multiple
>> >     times. Let's fix that so each and every requests is
>> >     accounted for only once.
>> >     
>> >     Cc: <stable@vger.kernel.org> # v4.8
>> >     Fixes: 55a0237f8f47 ("usb: dwc3: gadget: use allocated/queued reqs for LST bit")
>> >     Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> > 
>> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> > index 07cc8929f271..3c3ced128c77 100644
>> > --- a/drivers/usb/dwc3/gadget.c
>> > +++ b/drivers/usb/dwc3/gadget.c
>> > @@ -783,6 +783,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
>> >  		req->trb = trb;
>> >  		req->trb_dma = dwc3_trb_dma_offset(dep, trb);
>> >  		req->first_trb_index = dep->trb_enqueue;
>> > +		dep->queued_requests++;
>> >  	}
>> >  
>> >  	dwc3_ep_inc_enq(dep);
>> > @@ -833,8 +834,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
>> >  
>> >  	trb->ctrl |= DWC3_TRB_CTRL_HWO;
>> >  
>> > -	dep->queued_requests++;
>> > -
>> >  	trace_dwc3_prepare_trb(dep, trb);
>> >  }
>> >  
>> > @@ -1861,8 +1860,11 @@ static int __dwc3_cleanup_done_trbs(struct dwc3 *dwc, struct dwc3_ep *dep,
>> >  	unsigned int		s_pkt = 0;
>> >  	unsigned int		trb_status;
>> >  
>> > -	dep->queued_requests--;
>> >  	dwc3_ep_inc_deq(dep);
>> > +
>> > +	if (req->trb == trb)
>> > +		dep->queued_requests--;
>> > +
>> >  	trace_dwc3_complete_trb(dep, trb);
>> >  
>> >  	/*
>> > 
>> > I have also built a branch which you can use for testing. Here's a pull
>> > request, once you tell me it works for you, then I can send proper
>> > patches out:
>> > 
>> > The following changes since commit c8d2bc9bc39ebea8437fd974fdbc21847bb897a3:
>> > 
>> >   Linux 4.8 (2016-10-02 16:24:33 -0700)
>> > 
>> > are available in the git repository at:
>> > 
>> >   git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git tmp-test-v4.8
>> > 
>> > for you to fetch changes up to c968b8d1effe64a7980802d1eef29f4e1922faca:
>> > 
>> >   usb: dwc3: gadget: properly account queued requests (2016-10-06 10:16:37 +0300)
>> > 
>> > ----------------------------------------------------------------
>> > Felipe Balbi (2):
>> >       usb: gadget: function: u_ether: don't starve tx request queue
>> >       usb: dwc3: gadget: properly account queued requests
>> > 
>> >  drivers/usb/dwc3/gadget.c             | 7 ++++---
>> >  drivers/usb/gadget/function/u_ether.c | 5 +++--
>> >  2 files changed, 7 insertions(+), 5 deletions(-)
>> 
>> Tried your branch, but unfortunately I'm still seeing the lags. New trace
>> attached.
>
> Just a reminder that the regressions is still there in 4.9-rc2.
> Unfortunateyly with all the stuff already piled on top, getting USB
> working on my device is no longer a simple matter of reverting the
> one commit. I had to revert 10 patches to get even a clean revert, but
> unfortunately that approach just lead to the transfer hanging immediately.
>
> So what I ended up doing is reverting all of this:
> git log --no-merges --format=oneline 55a0237f8f47957163125e20ee9260538c5c341c^..HEAD -- drivers/usb/dwc3/ include/linux/ulpi/interface.h drivers/usb/Kconfig drivers/usb/core/Kconfig
>
> which comes out at whopping 70 commits. Having to carry that around
> is going to be quite a pain especially as more stuff might be piled on
> top.
>
> /me thinks a stable backport of any fix (assuming one is found
> eventually) is going to be quite the challenge...

Yeah, I'm guessing we're gonna need some help from networking folks. The
only thing we did since v4.7 was actually respect req->no_interrupt flag
coming from u_ether itself. No idea why that causes so much trouble for
u_ether.

BTW, Instead of reverting so many patches, you can just remove
throttling:

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index f4a640216913..119a2e5848e8 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -589,14 +589,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 
        req->length = length;
 
-       /* throttle high/super speed IRQ rate back slightly */
-       if (gadget_is_dualspeed(dev->gadget))
-               req->no_interrupt = (((dev->gadget->speed == USB_SPEED_HIGH ||
-                                      dev->gadget->speed == USB_SPEED_SUPER)) &&
-                                       !list_empty(&dev->tx_reqs))
-                       ? ((atomic_read(&dev->tx_qlen) % dev->qmult) != 0)
-                       : 0;
-
        retval = usb_ep_queue(in, req, GFP_ATOMIC);
        switch (retval) {
        default:

I'm adding netdev and couple other folks to the loop.

Just to summarize, USB peripheral controller now actually throttles
interrupt when requested to do so and that causes lags for USB
networking gadgets.

Without throttle we, potentially, call netif_wake_queue() more
frequently than with throttling. I'm wondering if something changed in
NET layer within the past few years but the USB networking gadgets ended
up being forgotten.

Anyway, if anybody has any hints, I'd be glad to hear about them.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH] Revert "usb: dwc3: gadget: use allocated/queued reqs for LST bit"

[Ville Syrjälä]

On Fri, Oct 28, 2016 at 01:16:13PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Ville Syrjälä <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> writes:
> > On Thu, Oct 06, 2016 at 12:08:26PM +0300, Ville Syrjälä wrote:
> >> On Thu, Oct 06, 2016 at 10:36:09AM +0300, Felipe Balbi wrote:
> >> > 
> >> > Hi,
> >> > 
> >> > Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> writes:
> >> <snip>
> >> > Okay, I have found a regression on dwc3 and another patch follows:
> >> > 
> >> > commit 5e1a2af3e46248c55098cdae643c4141851b703e
> >> > Author: Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >> > Date:   Wed Oct 5 14:24:37 2016 +0300
> >> > 
> >> >     usb: dwc3: gadget: properly account queued requests
> >> >     
> >> >     Some requests could be accounted for multiple
> >> >     times. Let's fix that so each and every requests is
> >> >     accounted for only once.
> >> >     
> >> >     Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v4.8
> >> >     Fixes: 55a0237f8f47 ("usb: dwc3: gadget: use allocated/queued reqs for LST bit")
> >> >     Signed-off-by: Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >> > 
> >> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >> > index 07cc8929f271..3c3ced128c77 100644
> >> > --- a/drivers/usb/dwc3/gadget.c
> >> > +++ b/drivers/usb/dwc3/gadget.c
> >> > @@ -783,6 +783,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
> >> >  		req->trb = trb;
> >> >  		req->trb_dma = dwc3_trb_dma_offset(dep, trb);
> >> >  		req->first_trb_index = dep->trb_enqueue;
> >> > +		dep->queued_requests++;
> >> >  	}
> >> >  
> >> >  	dwc3_ep_inc_enq(dep);
> >> > @@ -833,8 +834,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
> >> >  
> >> >  	trb->ctrl |= DWC3_TRB_CTRL_HWO;
> >> >  
> >> > -	dep->queued_requests++;
> >> > -
> >> >  	trace_dwc3_prepare_trb(dep, trb);
> >> >  }
> >> >  
> >> > @@ -1861,8 +1860,11 @@ static int __dwc3_cleanup_done_trbs(struct dwc3 *dwc, struct dwc3_ep *dep,
> >> >  	unsigned int		s_pkt = 0;
> >> >  	unsigned int		trb_status;
> >> >  
> >> > -	dep->queued_requests--;
> >> >  	dwc3_ep_inc_deq(dep);
> >> > +
> >> > +	if (req->trb == trb)
> >> > +		dep->queued_requests--;
> >> > +
> >> >  	trace_dwc3_complete_trb(dep, trb);
> >> >  
> >> >  	/*
> >> > 
> >> > I have also built a branch which you can use for testing. Here's a pull
> >> > request, once you tell me it works for you, then I can send proper
> >> > patches out:
> >> > 
> >> > The following changes since commit c8d2bc9bc39ebea8437fd974fdbc21847bb897a3:
> >> > 
> >> >   Linux 4.8 (2016-10-02 16:24:33 -0700)
> >> > 
> >> > are available in the git repository at:
> >> > 
> >> >   git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git tmp-test-v4.8
> >> > 
> >> > for you to fetch changes up to c968b8d1effe64a7980802d1eef29f4e1922faca:
> >> > 
> >> >   usb: dwc3: gadget: properly account queued requests (2016-10-06 10:16:37 +0300)
> >> > 
> >> > ----------------------------------------------------------------
> >> > Felipe Balbi (2):
> >> >       usb: gadget: function: u_ether: don't starve tx request queue
> >> >       usb: dwc3: gadget: properly account queued requests
> >> > 
> >> >  drivers/usb/dwc3/gadget.c             | 7 ++++---
> >> >  drivers/usb/gadget/function/u_ether.c | 5 +++--
> >> >  2 files changed, 7 insertions(+), 5 deletions(-)
> >> 
> >> Tried your branch, but unfortunately I'm still seeing the lags. New trace
> >> attached.
> >
> > Just a reminder that the regressions is still there in 4.9-rc2.
> > Unfortunateyly with all the stuff already piled on top, getting USB
> > working on my device is no longer a simple matter of reverting the
> > one commit. I had to revert 10 patches to get even a clean revert, but
> > unfortunately that approach just lead to the transfer hanging immediately.
> >
> > So what I ended up doing is reverting all of this:
> > git log --no-merges --format=oneline 55a0237f8f47957163125e20ee9260538c5c341c^..HEAD -- drivers/usb/dwc3/ include/linux/ulpi/interface.h drivers/usb/Kconfig drivers/usb/core/Kconfig
> >
> > which comes out at whopping 70 commits. Having to carry that around
> > is going to be quite a pain especially as more stuff might be piled on
> > top.
> >
> > /me thinks a stable backport of any fix (assuming one is found
> > eventually) is going to be quite the challenge...
> 
> Yeah, I'm guessing we're gonna need some help from networking folks. The
> only thing we did since v4.7 was actually respect req->no_interrupt flag
> coming from u_ether itself. No idea why that causes so much trouble for
> u_ether.
> 
> BTW, Instead of reverting so many patches, you can just remove
> throttling:
> 
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index f4a640216913..119a2e5848e8 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -589,14 +589,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
>  
>         req->length = length;
>  
> -       /* throttle high/super speed IRQ rate back slightly */
> -       if (gadget_is_dualspeed(dev->gadget))
> -               req->no_interrupt = (((dev->gadget->speed == USB_SPEED_HIGH ||
> -                                      dev->gadget->speed == USB_SPEED_SUPER)) &&
> -                                       !list_empty(&dev->tx_reqs))
> -                       ? ((atomic_read(&dev->tx_qlen) % dev->qmult) != 0)
> -                       : 0;
> -
>         retval = usb_ep_queue(in, req, GFP_ATOMIC);
>         switch (retval) {
>         default:

Ah cool. That indeed fixes the problem for me.

> 
> I'm adding netdev and couple other folks to the loop.
> 
> Just to summarize, USB peripheral controller now actually throttles
> interrupt when requested to do so and that causes lags for USB
> networking gadgets.
> 
> Without throttle we, potentially, call netif_wake_queue() more
> frequently than with throttling. I'm wondering if something changed in
> NET layer within the past few years but the USB networking gadgets ended
> up being forgotten.
> 
> Anyway, if anybody has any hints, I'd be glad to hear about them.
> 
> -- 
> balbi



-- 
Ville Syrjälä
Intel OTC
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert "usb: dwc3: gadget: use allocated/queued reqs for LST bit"

[David Miller]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date: Fri, 28 Oct 2016 19:33:32 +0300

> On Fri, Oct 28, 2016 at 01:16:13PM +0300, Felipe Balbi wrote:
>> Yeah, I'm guessing we're gonna need some help from networking folks. The
>> only thing we did since v4.7 was actually respect req->no_interrupt flag
>> coming from u_ether itself. No idea why that causes so much trouble for
>> u_ether.
>> 
>> BTW, Instead of reverting so many patches, you can just remove
>> throttling:
>> 
>> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
>> index f4a640216913..119a2e5848e8 100644
>> --- a/drivers/usb/gadget/function/u_ether.c
>> +++ b/drivers/usb/gadget/function/u_ether.c
>> @@ -589,14 +589,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
>>  
>>         req->length = length;
>>  
>> -       /* throttle high/super speed IRQ rate back slightly */
>> -       if (gadget_is_dualspeed(dev->gadget))
>> -               req->no_interrupt = (((dev->gadget->speed == USB_SPEED_HIGH ||
>> -                                      dev->gadget->speed == USB_SPEED_SUPER)) &&
>> -                                       !list_empty(&dev->tx_reqs))
>> -                       ? ((atomic_read(&dev->tx_qlen) % dev->qmult) != 0)
>> -                       : 0;
>> -
>>         retval = usb_ep_queue(in, req, GFP_ATOMIC);
>>         switch (retval) {
>>         default:
> 
> Ah cool. That indeed fixes the problem for me.
> 
>> 
>> I'm adding netdev and couple other folks to the loop.
>> 
>> Just to summarize, USB peripheral controller now actually throttles
>> interrupt when requested to do so and that causes lags for USB
>> networking gadgets.
>> 
>> Without throttle we, potentially, call netif_wake_queue() more
>> frequently than with throttling. I'm wondering if something changed in
>> NET layer within the past few years but the USB networking gadgets ended
>> up being forgotten.
>> 
>> Anyway, if anybody has any hints, I'd be glad to hear about them.

This throttling mechanism seems to have the same problem we've seen in
the past with some ethernet drivers trying to do TX mitigation in
software.

If I understand correctly, the interrupt bit for TX completions is set
only periodically.

However, the networking stack has a hard requirement that all SKBs
which are transmitted must have their completion signalled in a finite
amount of time.  This is because, until the SKB is freed by the
driver, it holds onto socket, netfilter, and other subsystem
resources.

So, for example, if your scheme is that only every 8th TX packet will
generate an interrupt you run into problems if you suddenly have 7
pending TX packets and no more traffic is generated for a long time.

Those 7 packets will sit in the TX queue indefinitely, and this is the
situation which drivers must avoid.

Therefore, for devices with per-TX-queue-entry interrupt bit schemes,
it's not easy to take advantage of this facility.  The safest thing to
do is to interrupt for every queue entry.

For the time being, this revert is the way to go and it should be
submitted formally, with proper commit message and signoffs, via
whatever tree this gadget driver's changes should be submitted via.

It might be possible to elide TX queue entry interrupts using the
skb->xmit_more state.  Basically, if the TX queue is not full and
skb->xmit_more is set, you can skip the interrupt indication bit
in the descriptor.

Thanks.

Re: [PATCH] Revert "usb: dwc3: gadget: use allocated/queued reqs for LST bit"

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3762 bytes --]


Hi,

David Miller <davem@davemloft.net> writes:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date: Fri, 28 Oct 2016 19:33:32 +0300
>
>> On Fri, Oct 28, 2016 at 01:16:13PM +0300, Felipe Balbi wrote:
>>> Yeah, I'm guessing we're gonna need some help from networking folks. The
>>> only thing we did since v4.7 was actually respect req->no_interrupt flag
>>> coming from u_ether itself. No idea why that causes so much trouble for
>>> u_ether.
>>> 
>>> BTW, Instead of reverting so many patches, you can just remove
>>> throttling:
>>> 
>>> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
>>> index f4a640216913..119a2e5848e8 100644
>>> --- a/drivers/usb/gadget/function/u_ether.c
>>> +++ b/drivers/usb/gadget/function/u_ether.c
>>> @@ -589,14 +589,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
>>>  
>>>         req->length = length;
>>>  
>>> -       /* throttle high/super speed IRQ rate back slightly */
>>> -       if (gadget_is_dualspeed(dev->gadget))
>>> -               req->no_interrupt = (((dev->gadget->speed == USB_SPEED_HIGH ||
>>> -                                      dev->gadget->speed == USB_SPEED_SUPER)) &&
>>> -                                       !list_empty(&dev->tx_reqs))
>>> -                       ? ((atomic_read(&dev->tx_qlen) % dev->qmult) != 0)
>>> -                       : 0;
>>> -
>>>         retval = usb_ep_queue(in, req, GFP_ATOMIC);
>>>         switch (retval) {
>>>         default:
>> 
>> Ah cool. That indeed fixes the problem for me.
>> 
>>> 
>>> I'm adding netdev and couple other folks to the loop.
>>> 
>>> Just to summarize, USB peripheral controller now actually throttles
>>> interrupt when requested to do so and that causes lags for USB
>>> networking gadgets.
>>> 
>>> Without throttle we, potentially, call netif_wake_queue() more
>>> frequently than with throttling. I'm wondering if something changed in
>>> NET layer within the past few years but the USB networking gadgets ended
>>> up being forgotten.
>>> 
>>> Anyway, if anybody has any hints, I'd be glad to hear about them.
>
> This throttling mechanism seems to have the same problem we've seen in
> The past with some ethernet drivers trying to do TX mitigation in
> software.
>
> If I understand correctly, the interrupt bit for TX completions is set
> only periodically.
>
> However, the networking stack has a hard requirement that all SKBs
> which are transmitted must have their completion signalled in a finite
> amount of time.  This is because, until the SKB is freed by the
> driver, it holds onto socket, netfilter, and other subsystem
> resources.
>
> So, for example, if your scheme is that only every 8th TX packet will
> generate an interrupt you run into problems if you suddenly have 7
> pending TX packets and no more traffic is generated for a long time.
>
> Those 7 packets will sit in the TX queue indefinitely, and this is the
> situation which drivers must avoid.
>
> Therefore, for devices with per-TX-queue-entry interrupt bit schemes,
> it's not easy to take advantage of this facility.  The safest thing to
> do is to interrupt for every queue entry.
>
> For the time being, this revert is the way to go and it should be
> submitted formally, with proper commit message and signoffs, via
> whatever tree this gadget driver's changes should be submitted via.
>
> It might be possible to elide TX queue entry interrupts using the
> skb->xmit_more state.  Basically, if the TX queue is not full and
> skb->xmit_more is set, you can skip the interrupt indication bit
> in the descriptor.

thanks for confirming, patch removing throttling sent. You're in Cc.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]