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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87ins7phcc.fsf@linux.intel.com>
Date:   Tue, 01 Nov 2016 13:30:59 +0200
From:   Felipe Balbi <felipe.balbi@...ux.intel.com>
To:     David Miller <davem@...emloft.net>, ville.syrjala@...ux.intel.com
Cc:     linux-usb@...r.kernel.org, stable@...r.kernel.org,
        oneukum@...e.com, netdev@...r.kernel.org
Subject: Re: [PATCH] Revert "usb: dwc3: gadget: use allocated/queued reqs for LST bit"


Hi,

David Miller <davem@...emloft.net> writes:
> From: Ville Syrjälä <ville.syrjala@...ux.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

Download attachment "signature.asc" of type "application/pgp-signature" (801 bytes)