[PATCH net] net: usbnet: fix potential deadlock on 32bit hosts

[Eric Dumazet]

From: Eric Dumazet <eduma...@google.com>

Marek reported a LOCKDEP issue occurring on 32bit host,
that we tracked down to the fact that usbnet could either
run from soft or hard irqs.

This patch adds u64_stats_update_begin_irqsave() and
u64_stats_update_end_irqrestore() helpers to solve this case.

[   17.768040] 
[   17.772239] WARNING: inconsistent lock state
[   17.776511] 4.16.0-rc3-next-20180227-7-g876c53a7493c #453 Not tainted
[   17.783329] 
[   17.787580] inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
[   17.793607] swapper/0/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
[   17.798751]  (>seq#5){?.-.}, at: [<9b22e5f0>] 
asix_rx_fixup_internal+0x188/0x288
[   17.806790] {IN-HARDIRQ-W} state was registered at:
[   17.811677]   tx_complete+0x100/0x208
[   17.815319]   __usb_hcd_giveback_urb+0x60/0xf0
[   17.819770]   xhci_giveback_urb_in_irq+0xa8/0x240
[   17.824469]   xhci_td_cleanup+0xf4/0x16c
[   17.828367]   xhci_irq+0xe74/0x2240
[   17.831827]   usb_hcd_irq+0x24/0x38
[   17.835343]   __handle_irq_event_percpu+0x98/0x510
[   17.840111]   handle_irq_event_percpu+0x1c/0x58
[   17.844623]   handle_irq_event+0x38/0x5c
[   17.848519]   handle_fasteoi_irq+0xa4/0x138
[   17.852681]   generic_handle_irq+0x18/0x28
[   17.856760]   __handle_domain_irq+0x6c/0xe4
[   17.860941]   gic_handle_irq+0x54/0xa0
[   17.864666]   __irq_svc+0x70/0xb0
[   17.867964]   arch_cpu_idle+0x20/0x3c
[   17.871578]   arch_cpu_idle+0x20/0x3c
[   17.875190]   do_idle+0x144/0x218
[   17.878468]   cpu_startup_entry+0x18/0x1c
[   17.882454]   start_kernel+0x394/0x400
[   17.886177] irq event stamp: 161912
[   17.889616] hardirqs last  enabled at (161912): [<7bedfacf>] 
__netdev_alloc_skb+0xcc/0x140
[   17.897893] hardirqs last disabled at (161911): [] 
__netdev_alloc_skb+0x94/0x140
[   17.904903] exynos5-hsi2c 12ca.i2c: tx timeout
[   17.906116] softirqs last  enabled at (161904): [<387102ff>] 
irq_enter+0x78/0x80
[   17.906123] softirqs last disabled at (161905): [] 
irq_exit+0x134/0x158
[   17.925722].
[   17.925722] other info that might help us debug this:
[   17.933435]  Possible unsafe locking scenario:
[   17.933435].
[   17.940331]CPU0
[   17.942488]
[   17.944894]   lock(>seq#5);
[   17.948274]   
[   17.950847] lock(>seq#5);
[   17.954386].
[   17.954386]  *** DEADLOCK ***
[   17.954386].
[   17.962422] no locks held by swapper/0/0.

Fixes: c8b5d129ee29 ("net: usbnet: support 64bit stats")
Signed-off-by: Eric Dumazet <eduma...@google.com>
Reported-by: Marek Szyprowski <m.szyprow...@samsung.com>
Cc: Greg Ungerer <g...@linux-m68k.org>
---
 drivers/net/usb/usbnet.c   |   10 ++
 include/linux/u64_stats_sync.h |   22 ++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 
8a22ff67b0268a588428c61c6a6211e3c6c2a02a..d9eea8cfe6cb9a3bf8d0d4ce9198af9bccf9c757
 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -315,6 +315,7 @@ static void __usbnet_status_stop_force(struct usbnet *dev)
 void usbnet_skb_return (struct usbnet *dev, struct sk_buff *skb)
 {
struct pcpu_sw_netstats *stats64 = this_cpu_ptr(dev->stats64);
+   unsigned long flags;
int status;
 
if (test_bit(EVENT_RX_PAUSED, >flags)) {
@@ -326,10 +327,10 @@ void usbnet_skb_return (struct usbnet *dev, struct 
sk_buff *skb)
if (skb->protocol == 0)
skb->protocol = eth_type_trans (skb, dev->net);
 
-   u64_stats_update_begin(>syncp);
+   flags = u64_stats_update_begin_irqsave(>syncp);
stats64->rx_packets++;
stats64->rx_bytes += skb->len;
-   u64_stats_update_end(>syncp);
+   u64_stats_update_end_irqrestore(>syncp, flags);
 
netif_dbg(dev, rx_status, dev->net, "< rx, len %zu, type 0x%x\n",
  skb->len + sizeof (struct ethhdr), skb->protocol);
@@ -1248,11 +1249,12 @@ static void tx_complete (struct urb *urb)
 
if (urb->status == 0) {
struct pcpu_sw_netstats *stats64 = this_cpu_ptr(dev->stats64);
+   unsigned long flags;
 
-   u64_stats_update_begin(>syncp);
+   flags = u64_stats_update_begin_irqsave(>syncp);
stats64->tx_packets += entry->packets;
stats64->tx_bytes += entry->length;
-   u64_stats_update_end(>syncp);
+   u64_stats_update_end_irqrestore(>syncp, flags);
} else {
dev->net->stats.tx_errors++;
 
diff --git a/include/linux/u64_stats_sync.h b/include/linux/u64_stats_sync.h
index 
5bdbd9f49395f883ca2dc5aa0d7bbde11f379063..07ee0f84a46caa9e2b1c446f96009f63b3b99f50
 100644
--- a/include/linux/u64_stats_sync.h
+++ b/include/linux/u64_stats_sync.h
@@ -90,6 +90

Re: inconsistent lock state with usbnet/asix usb ethernet and xhci

[Eric Dumazet]

On Mon, 2018-03-05 at 12:46 +0100, Oliver Neukum wrote:
> On Mon, 2018-03-05 at 08:45 +0100, Marek Szyprowski wrote:
> > Hi Oliver,
> > 
> > On 2018-02-27 17:07, Oliver Neukum wrote:
> > > Am Dienstag, den 27.02.2018, 07:13 -0800 schrieb Eric Dumazet:
> > > > On Tue, 2018-02-27 at 07:09 -0800, Eric Dumazet wrote:
> > > > > 
> > > > > Note that for this one, it seems we also could perform stats
> > > > > updates in
> > > > > BH context, since skb is queued via defer_bh()
> > > > > 
> > > > > But simplicity wins I guess.
> > > > 
> > > > Thinking more about this, I am not sure we have any guarantee
> > > > that TX
> > > > and RX can not run on multiple cpus.
> > > > 
> > > > Using an unique syncp is not going to be safe, even if we make
> > > > lockdep
> > > > happy enough with the local_irq save/restore.
> > > 
> > > Unfortunately you are right. It is not guaranteed for some
> > > hardware.
> > 
> > Does it mean that the fix proposed by Eric is not the proper
> > solution?
> 
> For asix it should work, but asix is unlikely to be the only driver
> with that issue. 32 bit recieves less testing nowadays.

Yes, although the lockdep part could be enforced in 64bit if we really
care.

I will send a patch using two different sync (one for RX, one for TX)


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: inconsistent lock state with usbnet/asix usb ethernet and xhci

[Eric Dumazet]

On Tue, 2018-02-27 at 07:09 -0800, Eric Dumazet wrote:
> 
> Note that for this one, it seems we also could perform stats updates in
> BH context, since skb is queued via defer_bh()
> 
> But simplicity wins I guess.

Thinking more about this, I am not sure we have any guarantee that TX
and RX can not run on multiple cpus.

Using an unique syncp is not going to be safe, even if we make lockdep
happy enough with the local_irq save/restore.


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: inconsistent lock state with usbnet/asix usb ethernet and xhci

[Eric Dumazet]

On Tue, 2018-02-27 at 15:42 +0100, Marek Szyprowski wrote:
> Hi Eric,
> 
> On 2018-02-27 15:07, Eric Dumazet wrote:
> > On Tue, 2018-02-27 at 08:26 +0100, Marek Szyprowski wrote:
> > > I've noticed that USBnet/ASIX AX88772B USB driver produces deplock kernel
> > > warning ("inconsistent lock state") on Chromebook2 Peach-PIT board. No
> > > special activity is needed to reproduce this issue, it happens almost
> > > on every boot. ASIX USB ethernet is connected to XHCI USB host controller
> > > on that board. Is it a known issue? Frankly I have no idea where to look
> > > to fix it. The same adapter connected to EHCI ports on other boards based
> > > on the same SoC works fine without any warnings.
> > > 
> > > Here are some more information from that board:
> > > # lsusb
> > > Bus 006 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> > > Bus 005 Device 002: ID 0b95:772b ASIX Electronics Corp. AX88772B
> > > Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > > Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> > > Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > > Bus 002 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> > > Bus 001 Device 002: ID 2232:1056 Silicon Motion
> > > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > > 
> > > # lsusb -t
> > > /:  Bus 06.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
> > > /:  Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M
> > >       |__ Port 1: Dev 2, If 0, Class=Vendor Specific Class, Driver=asix, 
> > > 480M
> > > /:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
> > > /:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M
> > > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=exynos-ohci/3p, 12M
> > > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=exynos-ehci/3p, 480M
> > >       |__ Port 1: Dev 2, If 0, Class=Video, Driver=, 480M
> > >       |__ Port 1: Dev 2, If 1, Class=Video, Driver=, 480M
> > > 
> > > 
> > > And the log with mentioned warning:
> > > 
> > > [   17.768040] 
> > > [   17.772239] WARNING: inconsistent lock state
> > > [   17.776511] 4.16.0-rc3-next-20180227-7-g876c53a7493c #453 Not 
> > > tainted
> > > [   17.783329] 
> > > [   17.787580] inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
> > > [   17.793607] swapper/0/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
> > > [   17.798751]  (>seq#5){?.-.}, at: [<9b22e5f0>]
> > > asix_rx_fixup_internal+0x188/0x288
> > > [   17.806790] {IN-HARDIRQ-W} state was registered at:
> > > [   17.811677]   tx_complete+0x100/0x208
> > > [   17.815319]   __usb_hcd_giveback_urb+0x60/0xf0
> > > [   17.819770]   xhci_giveback_urb_in_irq+0xa8/0x240
> > > [   17.824469]   xhci_td_cleanup+0xf4/0x16c
> > > [   17.828367]   xhci_irq+0xe74/0x2240
> > > [   17.831827]   usb_hcd_irq+0x24/0x38
> > > [   17.835343]   __handle_irq_event_percpu+0x98/0x510
> > > [   17.840111]   handle_irq_event_percpu+0x1c/0x58
> > > [   17.844623]   handle_irq_event+0x38/0x5c
> > > [   17.848519]   handle_fasteoi_irq+0xa4/0x138
> > > [   17.852681]   generic_handle_irq+0x18/0x28
> > > [   17.856760]   __handle_domain_irq+0x6c/0xe4
> > > [   17.860941]   gic_handle_irq+0x54/0xa0
> > > [   17.864666]   __irq_svc+0x70/0xb0
> > > [   17.867964]   arch_cpu_idle+0x20/0x3c
> > > [   17.871578]   arch_cpu_idle+0x20/0x3c
> > > [   17.875190]   do_idle+0x144/0x218
> > > [   17.878468]   cpu_startup_entry+0x18/0x1c
> > > [   17.882454]   start_kernel+0x394/0x400
> > > [   17.886177] irq event stamp: 161912
> > > [   17.889616] hardirqs last  enabled at (161912): [<7bedfacf>]
> > > __netdev_alloc_skb+0xcc/0x140
> > > [   17.897893] hardirqs last disabled at (161911): []
> > > __netdev_alloc_skb+0x94/0x140
> > > [   17.904903] exynos5-hsi2c 12ca.i2c: tx timeout
> > > [   17.906116] softirqs last  enabled at (161904): [<387102ff>]
> > > irq_enter+0x78/0x80
> > > [   17.906123] softirqs last disabled at (161905): []
> > > irq_exit+0x134/0x158
> > > [   17.925722].
> > > [   17.925722] other info that might help us debug this:
> > > [   17.933435]  Possible unsafe locking scenario:
> > > [   17.933435].
> &g

Re: inconsistent lock state with usbnet/asix usb ethernet and xhci

[Eric Dumazet]

On Tue, 2018-02-27 at 08:26 +0100, Marek Szyprowski wrote:
> Hi
> 
> I've noticed that USBnet/ASIX AX88772B USB driver produces deplock kernel
> warning ("inconsistent lock state") on Chromebook2 Peach-PIT board. No
> special activity is needed to reproduce this issue, it happens almost
> on every boot. ASIX USB ethernet is connected to XHCI USB host controller
> on that board. Is it a known issue? Frankly I have no idea where to look
> to fix it. The same adapter connected to EHCI ports on other boards based
> on the same SoC works fine without any warnings.
> 
> Here are some more information from that board:
> # lsusb
> Bus 006 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 005 Device 002: ID 0b95:772b ASIX Electronics Corp. AX88772B
> Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 002 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 001 Device 002: ID 2232:1056 Silicon Motion
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> 
> # lsusb -t
> /:  Bus 06.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
> /:  Bus 05.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M
>      |__ Port 1: Dev 2, If 0, Class=Vendor Specific Class, Driver=asix, 480M
> /:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
> /:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M
> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=exynos-ohci/3p, 12M
> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=exynos-ehci/3p, 480M
>      |__ Port 1: Dev 2, If 0, Class=Video, Driver=, 480M
>      |__ Port 1: Dev 2, If 1, Class=Video, Driver=, 480M
> 
> 
> And the log with mentioned warning:
> 
> [   17.768040] 
> [   17.772239] WARNING: inconsistent lock state
> [   17.776511] 4.16.0-rc3-next-20180227-7-g876c53a7493c #453 Not tainted
> [   17.783329] 
> [   17.787580] inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
> [   17.793607] swapper/0/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
> [   17.798751]  (>seq#5){?.-.}, at: [<9b22e5f0>] 
> asix_rx_fixup_internal+0x188/0x288
> [   17.806790] {IN-HARDIRQ-W} state was registered at:
> [   17.811677]   tx_complete+0x100/0x208
> [   17.815319]   __usb_hcd_giveback_urb+0x60/0xf0
> [   17.819770]   xhci_giveback_urb_in_irq+0xa8/0x240
> [   17.824469]   xhci_td_cleanup+0xf4/0x16c
> [   17.828367]   xhci_irq+0xe74/0x2240
> [   17.831827]   usb_hcd_irq+0x24/0x38
> [   17.835343]   __handle_irq_event_percpu+0x98/0x510
> [   17.840111]   handle_irq_event_percpu+0x1c/0x58
> [   17.844623]   handle_irq_event+0x38/0x5c
> [   17.848519]   handle_fasteoi_irq+0xa4/0x138
> [   17.852681]   generic_handle_irq+0x18/0x28
> [   17.856760]   __handle_domain_irq+0x6c/0xe4
> [   17.860941]   gic_handle_irq+0x54/0xa0
> [   17.864666]   __irq_svc+0x70/0xb0
> [   17.867964]   arch_cpu_idle+0x20/0x3c
> [   17.871578]   arch_cpu_idle+0x20/0x3c
> [   17.875190]   do_idle+0x144/0x218
> [   17.878468]   cpu_startup_entry+0x18/0x1c
> [   17.882454]   start_kernel+0x394/0x400
> [   17.886177] irq event stamp: 161912
> [   17.889616] hardirqs last  enabled at (161912): [<7bedfacf>] 
> __netdev_alloc_skb+0xcc/0x140
> [   17.897893] hardirqs last disabled at (161911): [] 
> __netdev_alloc_skb+0x94/0x140
> [   17.904903] exynos5-hsi2c 12ca.i2c: tx timeout
> [   17.906116] softirqs last  enabled at (161904): [<387102ff>] 
> irq_enter+0x78/0x80
> [   17.906123] softirqs last disabled at (161905): [] 
> irq_exit+0x134/0x158
> [   17.925722].
> [   17.925722] other info that might help us debug this:
> [   17.933435]  Possible unsafe locking scenario:
> [   17.933435].
> [   17.940331]    CPU0
> [   17.942488]    
> [   17.944894]   lock(>seq#5);
> [   17.948274]   
> [   17.950847] lock(>seq#5);
> [   17.954386].
> [   17.954386]  *** DEADLOCK ***
> [   17.954386].
> [   17.962422] no locks held by swapper/0/0.
> [   17.966011].
> [   17.966011] stack backtrace:
> [   17.971333] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
> 4.16.0-rc3-next-20180227-7-g876c53a7493c #453
> [   17.980312] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   17.986380] [] (unwind_backtrace) from [] 
> (show_stack+0x10/0x14)
> [   17.994128] [] (show_stack) from [] 
> (dump_stack+0x90/0xc8)
> [   18.001339] [] (dump_stack) from [] 
> (print_usage_bug+0x25c/0x2cc)
> [   18.009161] [] (print_usage_bug) from [] 
> (mark_lock+0x290/0x698)
> [   18.014952] exynos5-hsi2c 12ca.i2c: tx timeout
> [   18.016899] [] (mark_lock) from [] 
> (__lock_acquire+0x454/0x1850)
> [   18.029449] [] (__lock_acquire) from [] 
> (lock_acquire+0xc8/0x2b8)
> [   18.037272] [] (lock_acquire) from [] 
> (usbnet_skb_return+0x7c/0x1a0)
> [   18.045356] [] (usbnet_skb_return) from [] 
> (asix_rx_fixup_internal+0x188/0x288)
> [   18.054420] [] 

Re: dvb usb issues since kernel 4.9

[Eric Dumazet]

On Fri, 2018-01-12 at 19:13 -0200, Mauro Carvalho Chehab wrote:
> 
> 
> The .config file used to build the Kernel is at:
>   https://pastebin.com/wpZghann
> 

Hi Mauro

Any chance you can try CONFIG_HZ_1000=y, CONFIG_HZ=1000 ?

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: dvb usb issues since kernel 4.9

[Eric Dumazet]

On Tue, Jan 9, 2018 at 10:58 AM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> On Tue, Jan 9, 2018 at 9:57 AM, Eric Dumazet <eduma...@google.com> wrote:
>>
>> Your patch considers TASKLET_SOFTIRQ being a candidate for 'immediate
>> handling', but TCP Small queues heavily use TASKLET,
>> so as far as I am concerned a revert would have the same effect.
>
> Does it actually?
>
> TCP ends up dropping packets outside of the window etc, so flooding a
> machine with TCP packets and causing some further processing up the
> stack sounds very different from the basic packet flooding thing that
> happens with NET_RX_SOFTIRQ.
>
> Also, honestly, the kinds of people who really worry about flooding
> tend to have packet filtering in the receive path etc.
>
> So I really think "you can use up 90% of CPU time with a UDP packet
> flood from the same network" is very very very different - and
> honestly not at all as important - as "you want to be able to use a
> USB DVB receiver and watch/record TV".
>
> Because that whole "UDP packet flood from the same network" really is
> something you _fundamentally_ have other mitigations for.
>
> I bet that whole commit was introduced because of a benchmark test,
> rather than real life. No?
>
> In contrast, now people are complaining about real loads not working.
>
>  Linus

I said that a revert was fine, maybe I was not clear.
Clearly we can not touch anything scheduler related without breaking
someone workload/assumptions on how system behaved at some point.

Your patch wont solve other workloads that might have been impacted by my patch,
so in one year (or next week), we will have to cope with another device driver
not using tasklet but still relying on immediate softirq processing.
Apparently, we have to live with softirq model forever, or switch to RT kernels.

Note that we have no mitigation for something that involve flood of
valid packets that no firewall can drop
(without dropping legitimate packets).
The 'benchmark' here is not really the trigger, only a tool validating
an idea/patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: dvb usb issues since kernel 4.9

[Eric Dumazet]

On Tue, Jan 9, 2018 at 9:48 AM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> On Tue, Jan 9, 2018 at 9:27 AM, Eric Dumazet <eduma...@google.com> wrote:
>>
>> So yes, commit 4cd13c21b207 ("softirq: Let ksoftirqd do its job") has
>> shown up multiple times in various 'regressions'
>> simply because it could surface the problem more often.
>> But even if you revert it, you can still make the faulty
>> driver/subsystem misbehave by adding more stress to the cpu handling
>> the IRQ.
>
> ..but that's always true. People sometimes live on the edge - often by
> design (ie hardware has been designed/selected to be the crappiest
> possible that still work).
>
> That doesn't change anything. A patch that takes "bad things can
> happen" to "bad things DO happen" is a bad patch.

I was expecting that people could get a chance to fix the root cause,
instead of trying to keep status quo.

Strangely, it took 18 months for someone to complain enough and
'bisect to this commit'

Your patch considers TASKLET_SOFTIRQ being a candidate for 'immediate
handling', but TCP Small queues heavily use TASKLET,
so as far as I am concerned a revert would have the same effect.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: dvb usb issues since kernel 4.9

[Eric Dumazet]

On Tue, Jan 9, 2018 at 8:51 AM, Josef Griebichler
 wrote:
> Hi Linus,
>
> your patch works very good for me and others (please see 
> https://forum.libreelec.tv/thread/4235-dvb-issue-since-le-switched-to-kernel-4-9-x/?postID=77006#post77006).
>  No errors in recordings any more.
> The patch was also tested on x86_64 (Revo 3700) with positive effect.
> I agree with the forum poster, that there's still an issue when recording and 
> watching livetv at same time. I also get audio dropouts and audio is out of 
> sync.
> According to user smp kernel 4.9.73 with your patch on rpi and according to 
> user jahutchi kernel 4.11.12 on x86_64 have no such issues.
> I don't know if this dropouts are related to this topic.
>
> If of any help I could provide perf output on raspberry with libreelec and 
> tvheadend.
>

Sorry to come late to the party.

It seems problem comes from some piece of hardware/driver having some
precise timing prereq, and opportunistic use of softirq/tasklet
(instead maybe of hard irq handlers )

While it is true that softirq might do the job in most cases, we
already have cases where this can be easily defeated,
say if one cpu has suddenly to handle multiple sources of interrupts
for various devices.
NET_RX can easily lock the cpu for 10ms (on HZ=100 builds)

So yes, commit 4cd13c21b207 ("softirq: Let ksoftirqd do its job") has
shown up multiple times in various 'regressions'
simply because it could surface the problem more often.
But even if you revert it, you can still make the faulty
driver/subsystem misbehave by adding more stress to the cpu handling
the IRQ.

Note that networking lacks fine control of its softirq processing.
Some people found/complained that relying more on ksoftirqd was
potentially adding tail latencies.

Maybe the answer is to tune the kernel for small latencies at the
price of small throughput (situation before the patch)

1) Revert the patch
2) get rid of ksoftirqd since it adds unexpected latencies.
3) Let applications that expect to have high throughput make sure to
pin their threads on cpus that are not processing IRQ.
(And make sure to not use irqbalance, and setup IRQ cpu affinities)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] net: usbnet: support 64bit stats in qmi_wwan driver

[Eric Dumazet]

On Fri, 2017-03-24 at 15:42 +1000, Greg Ungerer wrote:

> The usbnet core is used by a number of drivers. This patch only
> updates the qmi-wwan driver to use stats64. If you remove the
> dev->stats.rx_* updates all those other driver users will have
> no counts.

I see. Then I guess the u64 stuff could be done only if running 32bit
kernels. (The existing stats are using 'unsigned long' so are already
64bit wide on 64bit kernels)



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] net: usbnet: support 64bit stats in qmi_wwan driver

[Eric Dumazet]

On Fri, 2017-03-24 at 11:27 +1000, Greg Ungerer wrote:
> Add support for the net stats64 counters to the usbnet core and then to
> the qmi_wwan driver.
> 
> This is a strait forward addition of 64bit counters for RX and TX packets
> and byte counts. It is done in the same style as for the other net drivers
> that support stats64.
> 
> The bulk of the change is to the usbnet core. Then it is trivial to use
> that in the qmi_wwan.c driver. It would be very simple to extend this
> support to other usbnet based drivers.
> 
> The motivation to add this is that it is not particularly difficult to
> get the RX and TX byte counts to wrap on 32bit platforms.
> 
> Signed-off-by: Greg Ungerer 
> ---
>  drivers/net/usb/qmi_wwan.c |  1 +
>  drivers/net/usb/usbnet.c   | 28 
>  include/linux/usb/usbnet.h | 12 
>  3 files changed, 41 insertions(+)
> 
> v2: EXPORT symbol usbnet_get_stats64()
> rebase on top of net-next
> 
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 8056745..db12a66 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -251,6 +251,7 @@ static int qmi_wwan_mac_addr(struct net_device *dev, void 
> *p)
>   .ndo_change_mtu = usbnet_change_mtu,
>   .ndo_set_mac_address= qmi_wwan_mac_addr,
>   .ndo_validate_addr  = eth_validate_addr,
> + .ndo_get_stats64= usbnet_get_stats64,
>  };
>  
>  /* using a counter to merge subdriver requests with our own into a
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 13d4ec5..4499d89 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -330,6 +330,11 @@ void usbnet_skb_return (struct usbnet *dev, struct 
> sk_buff *skb)
>   dev->net->stats.rx_packets++;
>   dev->net->stats.rx_bytes += skb->len;

Why updating dev->net->stats.rx_{packets|bytes} and
dev->stats.rx_{packets|bytes}  ?

Seems redundant.


> + u64_stats_update_begin(>stats.syncp);
> + dev->stats.rx_packets++;
> + dev->stats.rx_bytes += skb->len;
> + u64_stats_update_end(>stats.syncp);
> +
>   netif_dbg(dev, rx_status, dev->net, "< rx, len %zu, type 0x%x\n",
> skb->len + sizeof (struct ethhdr), skb->protocol);
>   memset (skb->cb, 0, sizeof (struct skb_data));
> @@ -981,6 +986,23 @@ int usbnet_set_link_ksettings(struct net_device *net,
>  }
>  EXPORT_SYMBOL_GPL(usbnet_set_link_ksettings);
>  
> +void usbnet_get_stats64(struct net_device *net, struct rtnl_link_stats64 
> *stats)
> +{
> + struct usbnet *dev = netdev_priv(net);
> + unsigned int start;
> +
> + netdev_stats_to_stats64(stats, >stats);
> +
> + do {
> + start = u64_stats_fetch_begin_irq(>stats.syncp);
> + stats->rx_packets = dev->stats.rx_packets;
> + stats->rx_bytes = dev->stats.rx_bytes;
> + stats->tx_packets = dev->stats.tx_packets;
> + stats->tx_bytes = dev->stats.tx_bytes;
> + } while (u64_stats_fetch_retry_irq(>stats.syncp, start));
> +}
> +EXPORT_SYMBOL_GPL(usbnet_get_stats64);
> +
>  u32 usbnet_get_link (struct net_device *net)
>  {
>   struct usbnet *dev = netdev_priv(net);
> @@ -1214,6 +1236,11 @@ static void tx_complete (struct urb *urb)
>   if (urb->status == 0) {
>   dev->net->stats.tx_packets += entry->packets;
>   dev->net->stats.tx_bytes += entry->length;


Why updating dev->net->stats.tx_{packets|bytes} and
dev->stats.tx_{packets|bytes}  ?

> +
> + u64_stats_update_begin(>stats.syncp);
> + dev->stats.tx_packets += entry->packets;
> + dev->stats.tx_bytes += entry->length;
> + u64_stats_update_end(>stats.syncp);
>   } else {
>   dev->net->stats.tx_errors++;
>  
> @@ -1658,6 +1685,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>   init_timer (>delay);
>   mutex_init (>phy_mutex);
>   mutex_init(>interrupt_mutex);
> + u64_stats_init(>stats.syncp);
>   dev->interrupt_count = 0;
>  
>   dev->net = net;
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index e2b5691..d1501b8 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -22,6 +22,14 @@
>  #ifndef  __LINUX_USB_USBNET_H
>  #define  __LINUX_USB_USBNET_H
>  
> +struct usbnet_stats64 {
> + struct u64_stats_sync   syncp;
> + u64 rx_packets;
> + u64 rx_bytes;
> + u64 tx_packets;
> + u64 tx_bytes;
> +};
> +

You use the same syncp for TX and RX.

Do you have guarantee that TX and RX paths can not run concurrently (on
different cpus) ?



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] r8152: Allocate interrupt buffer as part of struct r8152

[Eric Dumazet]

On Tue, 2017-01-31 at 11:06 -0800, Guenter Roeck wrote:
> When unloading the r8152 driver using the 'unbind' sysfs attribute
> in a system with KASAN enabled, the following error message is seen
> on a regular basis.

>  
>  static int alloc_all_mem(struct r8152 *tp)
> @@ -1423,10 +1420,6 @@ static int alloc_all_mem(struct r8152 *tp)
>   if (!tp->intr_urb)
>   goto err1;
>  
> - tp->intr_buff = kmalloc(INTBUFSIZE, GFP_KERNEL);
> - if (!tp->intr_buff)
> - goto err1;
> -
>   tp->intr_interval = (int)ep_intr->desc.bInterval;
>   usb_fill_int_urb(tp->intr_urb, tp->udev, usb_rcvintpipe(tp->udev, 3),
>tp->intr_buff, INTBUFSIZE, intr_callback,

This might lead to intr_buff being backed by vzalloc() instead of
kzalloc() (check alloc_netdev_mqs())

It looks like it could cause a bug.



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NAPI on USB network drivers

[Eric Dumazet]

On Wed, 2017-01-25 at 09:39 +, Hayes Wang wrote:
> Oliver Neukum [mailto:oneu...@suse.com]
> > Sent: Wednesday, January 25, 2017 5:35 PM
> [...]
> > looking at r8152 I noticed that it uses NAPI. I never considered
> > this for the generic USB networking code as you cannot disable
> > interrupts for USB. Is it still worth it? What are the benefits?
> 
> You could use napi_gro_receive() and it influences the performance.

You also could use napi_complete_done() instead of napi_complete(), as
it allows users to tune the performance vs latency for GRO.

Looking at this driver, I do not see any limitation on the number of
skbs that can be pushed into tp->rx_queue.

I wonder if this queue can end up consuming all memory of a host under
stress.

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 
e1466b4d2b6c727148a884672bbd9593bf04b3ac..221df4a931b5c1073f1922d0fa0bbff158c73b7d
 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1840,7 +1840,10 @@ static int rx_bottom(struct r8152 *tp, int budget)
stats->rx_packets++;
stats->rx_bytes += pkt_len;
} else {
-   __skb_queue_tail(>rx_queue, skb);
+   if (unlikely(skb_queue_len(>rx_queue) >= 
1000))
+   kfree_skb(skb);
+   else
+   __skb_queue_tail(>rx_queue, skb);
}
 
 find_next_rx:


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net v2 3/4] r8152: re-schedule napi for tx

[Eric Dumazet]

On Wed, 2017-01-25 at 16:13 +0800, Hayes Wang wrote:
> Re-schedule napi after napi_complete() for tx, if it is necessay.
> 
> In r8152_poll(), if the tx is completed after tx_bottom() and before
> napi_complete(), the scheduling of napi would be lost. Then, no
> one handles the next tx until the next napi_schedule() is called.
> 
> Signed-off-by: Hayes Wang 
> ---
>  drivers/net/usb/r8152.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index ec882be..45d168e 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1936,6 +1936,9 @@ static int r8152_poll(struct napi_struct *napi, int 
> budget)
>   napi_complete(napi);
>   if (!list_empty(>rx_done))
>   napi_schedule(napi);
> + else if (!skb_queue_empty(>tx_queue) &&
> +  !list_empty(>tx_free))
> + napi_schedule(>napi);

Why using >napi instead of napi here, as done 3 lines above ?



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/5] net: asix: autoneg will set WRITE_MEDIUM reg

[Eric Dumazet]

On Thu, 2016-09-01 at 12:47 -0400, Robert Foss wrote:

> I'm not quite sure how the first From line was added, it
> should not have been.
> Grant Grundler is most definitely the author.
> 
> Would you like me to resubmit in v++ and make sure that it has been 
> corrected?

This is too late, patches are already merged in David Miller net-next
tree.

These kinds of errors can not be fixed, we have to be very careful at
submission/review time.

I guess Grant does not care, but some contributors, especially new ones
would like to get proper attribution.

Thanks.


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/5] net: asix: autoneg will set WRITE_MEDIUM reg

[Eric Dumazet]

On Mon, 2016-08-29 at 09:32 -0400, robert.f...@collabora.com wrote:
> From: Robert Foss 
> 
> From: Grant Grundler 
> 
> The miii_nway_restart() causes a PHY link change activity and
> ax88772_link_reset will be called. link_reset will set
> AX_CMD_WRITE_MEDIUM_MODE register correctly.
> 
> The asix_write_medium_mode in reset() fills in a default value to the register
> which may be different from the negotiation result. So do this first.
> 
> Ignore the ret value since it's ignored in XXX_link_reset() functions.
> 
> Signed-off-by: Grant Grundler 
> Signed-off-by: Robert Foss 
> Tested-by: Robert Foss 
> ---

This is _really_ confusing Robert.

Why having two 'From:' clauses ?

Who wrote the patch in the first place ? You or Grant ?



End result is :

commit 535baf8588d04b177cb33700f81499f2b5203c2d
Author: Robert Foss 
Date:   Mon Aug 29 09:32:19 2016 -0400

net: asix: autoneg will set WRITE_MEDIUM reg

From: Grant Grundler 

The miii_nway_restart() causes a PHY link change activity and
ax88772_link_reset will be called. link_reset will set
AX_CMD_WRITE_MEDIUM_MODE register correctly.

The asix_write_medium_mode in reset() fills in a default value to the 
register
which may be different from the negotiation result. So do this first.

Ignore the ret value since it's ignored in XXX_link_reset() functions.

Signed-off-by: Grant Grundler 
Signed-off-by: Robert Foss 
Tested-by: Robert Foss 
Signed-off-by: David S. Miller 



I guess Grant wrote the patch, but attribution is wrong.


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Eric Dumazet]

On Sun, 2016-03-20 at 11:43 +0100, Geert Uytterhoeven wrote:
> If CONFIG_PM=n:
> 
> drivers/net/usb/lan78xx.c: In function ‘lan78xx_get_stats64’:
> drivers/net/usb/lan78xx.c:3274: error: ‘struct dev_pm_info’ has no member 
> named ‘runtime_auto’
> 
> If PM is disabled, the runtime_auto flag is not available, but auto
> suspend is not enabled anyway.  Hence protect the check for runtime_auto
> by #ifdef CONFIG_PM to fix this.
> 
> Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64")
> Reported-by: Guenter Roeck 
> Signed-off-by: Geert Uytterhoeven 
> ---
> Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper to
> include/linux/pm.h, which always return false if CONFIG_PM is disabled.
> 
> The only other user in non-core code (drivers/usb/core/sysfs.c) has a
> big #ifdef CONFIG_PM check around all PM-related code.
> 
> Thoughts?
> ---
>  drivers/net/usb/lan78xx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index d36d5ebf37f355f2..7b9ac47b2ecf9905 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -3271,7 +3271,9 @@ struct rtnl_link_stats64 *lan78xx_get_stats64(struct 
> net_device *netdev,
>* periodic reading from HW will prevent from entering USB auto suspend.
>* if autosuspend is disabled, read from HW.
>*/
> +#ifdef CONFIG_PM
>   if (!dev->udev->dev.power.runtime_auto)
> +#endif
>   lan78xx_update_stats(dev);
>  
>   mutex_lock(>stats.access_lock);

Note that a ndo_get_stat64() handler is not allowed to sleep,
so the mutex_lock() is not wise...

Historically /proc/net/dev handler but also bonding ndo_get_stats() used
RCU or a rwlock or a spinlock.

So a complete fix would need to get rid of this mutex as well.


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] r8152: drop the tx packet with invalid length

[Eric Dumazet]

On Fri, 2014-12-19 at 06:42 +, Hayes Wang wrote:

 Excuse me. I try to implement ndo_gso_check() with kernel 3.18.
 However, I still get packets with gso and their skb lengths are more
 than the acceptable one. Do I miss something?
 
 +static bool rtl8152_gso_check(struct sk_buff *skb, struct net_device *dev)
 +{
 + if ((skb-len + sizeof(struct tx_desc))  agg_buf_sz)
 + return false;
 +
 + return true;
 +}
  
 @@ -5861,6 +5876,9 @@ static const struct net_device_ops rtl8152_netdev_ops = 
 {
   .ndo_set_mac_address= rtl8152_set_mac_address,
   .ndo_change_mtu = rtl8152_change_mtu,
   .ndo_validate_addr  = eth_validate_addr,
 + .ndo_gso_check  = rtl8152_gso_check,
  };

You are right, it seems ndo_gso_check() is buggy at this moment.

Presumably this method should alter %features so that we really segment
the packets in software.

features = ~NETIF_F_GSO_MASK;



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] r8152: drop the tx packet with invalid length

[Eric Dumazet]

On Fri, 2014-12-19 at 09:36 -0800, Eric Dumazet wrote:
 On Fri, 2014-12-19 at 06:42 +, Hayes Wang wrote:
 
  Excuse me. I try to implement ndo_gso_check() with kernel 3.18.
  However, I still get packets with gso and their skb lengths are more
  than the acceptable one. Do I miss something?
  
  +static bool rtl8152_gso_check(struct sk_buff *skb, struct net_device *dev)
  +{
  +   if ((skb-len + sizeof(struct tx_desc))  agg_buf_sz)
  +   return false;
  +
  +   return true;
  +}
   
  @@ -5861,6 +5876,9 @@ static const struct net_device_ops rtl8152_netdev_ops 
  = {
  .ndo_set_mac_address= rtl8152_set_mac_address,
  .ndo_change_mtu = rtl8152_change_mtu,
  .ndo_validate_addr  = eth_validate_addr,
  +   .ndo_gso_check  = rtl8152_gso_check,
   };
 
 You are right, it seems ndo_gso_check() is buggy at this moment.
 
 Presumably this method should alter %features so that we really segment
 the packets in software.
 
 features = ~NETIF_F_GSO_MASK;

Could you try following patch ?

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 7df221788cd4..0346bcfe72a5 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -314,7 +314,7 @@ static rx_handler_result_t macvtap_handle_frame(struct 
sk_buff **pskb)
 */
if (q-flags  IFF_VNET_HDR)
features |= vlan-tap_features;
-   if (netif_needs_gso(dev, skb, features)) {
+   if (netif_needs_gso(dev, skb, features)) {
struct sk_buff *segs = __skb_gso_segment(skb, features, false);
 
if (IS_ERR(segs))
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 22bcb4e12e2a..9cacabaea175 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -578,6 +578,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
unsigned long flags;
struct netfront_queue *queue = NULL;
unsigned int num_queues = dev-real_num_tx_queues;
+   netdev_features_t features;
u16 queue_index;
 
/* Drop the packet if no queues are set up */
@@ -611,9 +612,10 @@ static int xennet_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
 
spin_lock_irqsave(queue-tx_lock, flags);
 
+   features = netif_skb_features(skb);
if (unlikely(!netif_carrier_ok(dev) ||
 (slots  1  !xennet_can_sg(dev)) ||
-netif_needs_gso(dev, skb, netif_skb_features(skb {
+netif_needs_gso(dev, skb, features))) {
spin_unlock_irqrestore(queue-tx_lock, flags);
goto drop;
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c31f74d76ebd..fb1f8c900df9 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3608,13 +3608,19 @@ static inline bool skb_gso_ok(struct sk_buff *skb, 
netdev_features_t features)
 }
 
 static inline bool netif_needs_gso(struct net_device *dev, struct sk_buff *skb,
-  netdev_features_t features)
+  netdev_features_t *features)
 {
-   return skb_is_gso(skb)  (!skb_gso_ok(skb, features) ||
-   (dev-netdev_ops-ndo_gso_check 
-!dev-netdev_ops-ndo_gso_check(skb, dev)) ||
-   unlikely((skb-ip_summed != CHECKSUM_PARTIAL) 
-(skb-ip_summed != CHECKSUM_UNNECESSARY)));
+   if (!skb_is_gso(skb))
+   return false;
+   if (!skb_gso_ok(skb, *features))
+   return true;
+   if (dev-netdev_ops-ndo_gso_check 
+   !dev-netdev_ops-ndo_gso_check(skb, dev)) {
+   *features = ~NETIF_F_GSO_MASK;
+   return true;
+   }
+   return skb-ip_summed != CHECKSUM_PARTIAL 
+  skb-ip_summed != CHECKSUM_UNNECESSARY;
 }
 
 static inline void netif_set_gso_max_size(struct net_device *dev,
diff --git a/net/core/dev.c b/net/core/dev.c
index f411c28d0a66..b61c26b45bb7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2668,7 +2668,7 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff 
*skb, struct net_device
if (skb-encapsulation)
features = dev-hw_enc_features;
 
-   if (netif_needs_gso(dev, skb, features)) {
+   if (netif_needs_gso(dev, skb, features)) {
struct sk_buff *segs;
 
segs = skb_gso_segment(skb, features);


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] r8152: reduce memory copy for rx

[Eric Dumazet]

On Wed, 2014-12-03 at 13:14 +0800, Hayes Wang wrote:
 If the data size is more than half of the AGG_BUG_SZ, allocate a new
 rx buffer and use skb_clone() to avoid the memory copy.
 
 The original method is that allocate the memory and copy data for each
 packet in a rx buffer. The new one is that when the data size for a rx
 buffer is more than RX_THRESHOLD_CLONED, allocate a new rx buffer and
 use skb_clone for each packet in the rx buffer. According to the
 experiment, the new mothod has better performance.

Better performance for what workload exactly ?

cloning in rx path has many drawbacks, with skb-truesize being usually
wrong.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] r8152: reduce memory copy for rx

[Eric Dumazet]

On Wed, 2014-12-03 at 07:05 +, Hayes Wang wrote:
 Eric Dumazet [mailto:eric.duma...@gmail.com] 
  Sent: Wednesday, December 03, 2014 2:08 PM
 [...]
  Better performance for what workload exactly ?
 
 I test it by using Chariot with 4 Tx and 4 Rx.
 It has about 4% improvement.
 

Have you tried using more concurrent RX flows, in a possibly lossy
environment (so that TCP is forced to queue packets in out of order
queue) ?

  cloning in rx path has many drawbacks, with skb-truesize 
  being usually wrong.
 
 Excuse me. I find the skb_clone() would copy the
 truesize from original skb. Do you mean the value
 may not be suitable for the cloned skb?

With cloning, (skb-len / skb-truesize) will eventually be very very
small, forcing TCP stack to perform collapses (copies !!!) under
pressure.

 
 Could other method do the same thing? Or, do you
 think keeping the original one is better?


skb cloning prevents GRO and TCP coalescing from working.

netfilter might also be forced to copy whole frame in case a mangle is
needed (eg with NAT ...)

I would rather try to implement GRO, and/or using fragments instead of
pure linear skbs.

(skb-head would be around 128 or 256 bytes, and you attach to skb the
frame as a page fragment)



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] r8152: drop the tx packet with invalid length

[Eric Dumazet]

On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote:
 Drop the tx packet which is more than the size of agg_buf_sz. When
 creating a bridge with the device, we may get the tx packet with
 TSO and the length is more than the gso_max_size which is set by
 the driver through netif_set_gso_max_size(). Such packets couldn't
 be transmitted and should be dropped directly.
 
 Signed-off-by: Hayes Wang hayesw...@realtek.com
 ---
  drivers/net/usb/r8152.c | 9 +
  1 file changed, 9 insertions(+)
 
 diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
 index c6554c7..ebdaff7 100644
 --- a/drivers/net/usb/r8152.c
 +++ b/drivers/net/usb/r8152.c
 @@ -1897,6 +1897,15 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff 
 *skb,
  {
   struct r8152 *tp = netdev_priv(netdev);
  
 + if ((skb-len + sizeof(struct tx_desc))  agg_buf_sz) {
 + struct net_device_stats *stats = netdev-stats;
 +
 + dev_kfree_skb_any(skb);
 + stats-tx_dropped++;
 + WARN_ON_ONCE(1);
 + return NETDEV_TX_OK;
 + }
 +
   skb_tx_timestamp(skb);
  
   skb_queue_tail(tp-tx_queue, skb);

Looks like a candidate for ndo_gso_check(), so that we do not drop, but
instead segment from netif_needs_gso()/validate_xmit_skb()



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] r8152: drop the tx packet with invalid length

[Eric Dumazet]

On Wed, 2014-11-26 at 12:06 -0500, David Miller wrote:
 From: Eric Dumazet eric.duma...@gmail.com
 Date: Wed, 26 Nov 2014 08:52:28 -0800
 
  On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote:
  Drop the tx packet which is more than the size of agg_buf_sz. When
  creating a bridge with the device, we may get the tx packet with
  TSO and the length is more than the gso_max_size which is set by
  the driver through netif_set_gso_max_size(). Such packets couldn't
  be transmitted and should be dropped directly.
  
  Signed-off-by: Hayes Wang hayesw...@realtek.com
  ...
  Looks like a candidate for ndo_gso_check(), so that we do not drop, but
  instead segment from netif_needs_gso()/validate_xmit_skb()
 
 You mean have the bridge implement the ndo_gso_check() method right?

No, I meant this particular driver.

Note that netif_skb_features() does only this check :

if (gso_segs  dev-gso_max_segs || gso_segs  dev-gso_min_segs)
  features = ~NETIF_F_GSO_MASK;


Ie not testing gso_max_size

It looks like all these particular tests should be moved on
ndo_gso_check(), to remove code from netif_skb_features()



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/3] usb: gadget: NCM: Stop RX TCP Bursts getting dropped.

[Eric Dumazet]

On Thu, 2014-05-29 at 18:12 +0100, Jim Baxter wrote:
 This fixes a problem with dropped packets over 16k CDC-NCM
 when the connection is being heavily used.
 
 The issue was that the skb truesize for the unpacked NCM
 packets was too high after they were cloned from the 16k
 skb, this lead to the potential memory calculated by the
 Kernel running out of memory earlier then it should.
 
 Signed-off-by: Jim Baxter jim_bax...@mentor.com
 ---

Note the patch is OK, but changelog a bit misleading ;)

Kernel was not running out of memory, because truesize was correct.

The problem here is that a frame was consuming more kernel memory than
really needed, so chances of hitting socket sk_rcvbuf limit was high.

BTW :
#define NTB_OUT_SIZE  16384

alloc_skb(size) -
kmalloc(16384 + sizeof(struct skb_shared_info)) -
roundup() = 32768

So truesize of the skb was infact ~32KB, which is really insane indeed.
After your patch, its back to ~2KB

Acked-by: Eric Dumazet eduma...@google.com



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: skbuff truesize incorrect.

[Eric Dumazet]

On Fri, 2014-05-23 at 12:13 +0100, Jim Baxter wrote:

 What are the side effects of changing the truesize, if the original
 uncloned skb has the full truesize then isn't the potential memory usage
 still counted for the avoidance of OOM?

Nope. This can be disastrous.

A malicious remote peer can crash your host by sending specially cooked
TCP messages.

Send messages with one byte of payload, and out of order so that they
cant be consumed by receiver, and cant be coalesced/collapsed.

If you claim the true size is sizeof(sk_buff) + 512, TCP stack will
accumulate these messages in out of order queue, and will not bother
with them, unless you hit sk_rcvbuf limit.

But in reality these messages uses sizeof(sk_buff) + 32768 bytes.

Divide your physical memory by 32768 : How many such messages will fit
in memory before the host crashes ?

I've seen that kind of attacks in real cases.

Even the fast clones sk_buff mismatch can be noticed. Luckily a 10%
error has no severe impact.

TCP stack uses fast clones, and current stack gives them a truesize of
2048 + sizeof(sk_buff), while it really should be 2048 +
2*sizeof(sk_buff)

Luckily, GSO/TSO tends to reduce the error, as skbs overhead is lower.


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: skbuff truesize incorrect.

[Eric Dumazet]

On Fri, 2014-05-23 at 11:33 +0200, Bjørn Mork wrote:
 Jim Baxter jim_bax...@mentor.com writes:
 
  I'll create and test a patch for the cdc_ncm host driver unless someone
  else wants to do that. I haven't really played with the gadget driver
  before, so I'd prefer if someone knowing it (Jim maybe?) could take care
  of it.  If not, then I can always make an attempt using dummy_hcd to
  test it.
  I can create a patch for the host driver, I will issue the gadget patch
  first to resolve any issues, the fix would be similar.
 
 Well, I couldn't help myself.  I just had to test it.  The attached
 patch works for me, briefly tested with an Ericsson H5321gw NCM device.
 I have no ideas about the performance impact as that modem is limited to
 21 Mbps HSDPA.

Ideally, the skb_in should not be freed/reallocated, but recycled,
especially if using 32KB.

But thats a minor detail, this patch is already a huge win for a 21Mbps
device.

Acked-by: Eric Dumazet eduma...@google.com


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: skbuff truesize incorrect.

[Eric Dumazet]

On Fri, 2014-05-23 at 15:30 +, David Laight wrote:
 From: Eric Dumazet
 ...
  TCP stack uses fast clones, and current stack gives them a truesize of
  2048 + sizeof(sk_buff), while it really should be 2048 +
  2*sizeof(sk_buff)
  
  Luckily, GSO/TSO tends to reduce the error, as skbs overhead is lower.
 
 Doesn't that affect the tx side - where the truesize doesn't matter as much?

Its not a matter of tx or rx, but percentage of error.

If truesize accounting is wrong by 10%, its not a big deal, because we
normally limit tcp_mem[] to about 16% of available physical memory.

Using 16% of physical memory instead of 16% should not really matter.

Now, if the truesize is wrong by 1600%, then its pretty clear we can
consume all the meory.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: skbuff truesize incorrect.

[Eric Dumazet]

On Fri, 2014-05-23 at 08:44 -0700, Rick Jones wrote:

 If you are measuring performance with the likes of netperf, you should 
 be able to get an idea of the performance effect from the change in 
 service demand (CPU consumed per unit of work) even if the maximum 
 throughput remains capped.

This wont be enough to truly get an idea of the gains this patch brings.

Add some random drops in the equation, and instead of dropping packets,
we gently add them in the out of order queue, gently coalesce them,
gently allow SACK enabled flows to recover thanks to fast retransmits,
with no latency effect (No expensive collapse/prunes)

# nstat -az|egrep TCPRcvCoalesce|TCPOFOQueue|TCPRcvCollapsed|Prune
TcpExtPruneCalled   1180.0
TcpExtRcvPruned 0  0.0
TcpExtOfoPruned 0  0.0
TcpExtTCPRcvCollapsed   1009   0.0
TcpExtTCPRcvCoalesce857806 0.0
TcpExtTCPOFOQueue   201246 0.0

Yep, we _might_ consume some cpu cycles in the perfect case where no
packet was dropped, but this is kind of an utopia.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: skbuff truesize incorrect.

[Eric Dumazet]

On Thu, 2014-05-22 at 20:39 +0100, Jim Baxter wrote:

 I now think that the correct solution here is to create a new smaller
 skb and copy the data from the sub packets into it.

For low speed devices, this is indeed the best way.

(this is called copybreak in some nic drivers)



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: skbuff truesize incorrect.

[Eric Dumazet]

On Thu, 2014-05-22 at 21:21 +0100, Jim Baxter wrote:


 OK, so it is the value of the memory that has been allocated for the SKB.
 If there are multiple clones for an skb all pointing at the same data,
 will that distort the memory used when they all have the same truesize?

Its always better to over estimate memory uses, than under estimating.

Its all about guarding against OOM.

Keep in mind some parts of networking stack do not like clones.

TCP coalescing works beautifully with non cloned skbs.

Instead of cloning skb or doing copies, you could play with having frame
on a page fragment. Only headers might need to be pulled from the frame
to linear part of skb.





--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: skbuff truesize incorrect.

[Eric Dumazet]

On Thu, 2014-05-22 at 20:07 +0100, Jim Baxter wrote:

 
 I have been investigating a network issue with bursts of network traffic
 over USB CDC-NCM, the issue is that the kernel is dropping packets
 because sk_rcvqueues_full() returns true due to skb2-truesize is always
 32960 instead of SKB_TRUESIZE(skb2-len) which is about 1800.
 
 The code I am trying to fix is this code below, it is splitting a set of
 multiple network packets compressed into a single 16k packet into
 individual skb's and sending them up the network stack.

if skb are allocated with 16k = 16384 bytes, adding the struct
skb_shared_info overhead and rounding up to power of two gives 32768
bytes.

Chances to be able to allocate 32KB contiguous memory are slim after a
while.

I would set rx_max (rx_urb_size) to SKB_MAX_HEAD(0) so that you do not
use high order allocations.

What is the exact 'overhead' using ~4K instead of 16K exactly on this
hardware ?



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: skbuff truesize incorrect.

[Eric Dumazet]

On Thu, 2014-05-22 at 13:58 -0700, Eric Dumazet wrote:

 I would set rx_max (rx_urb_size) to SKB_MAX_HEAD(0) so that you do not
 use high order allocations.

Correction, that would need SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN),
because drivers/net/usb/usbnet.c calls __netdev_alloc_skb_ip_align().



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net,stable] net: cdc_mbim: __vlan_find_dev_deep need rcu_read_lock

[Eric Dumazet]

On Fri, 2014-05-02 at 23:28 +0200, Bjørn Mork wrote:
 Fixes this warning introduced by commit 5b8f15f78e6f
 (net: cdc_mbim: handle IPv6 Neigbor Solicitations):
 
 ===
 [ INFO: suspicious RCU usage. ]
 3.15.0-rc3 #213 Tainted: GW  O
 ---
 net/8021q/vlan_core.c:69 suspicious rcu_dereference_check() usage!
 
 other info that might help us debug this:
 
 rcu_scheduler_active = 1, debug_locks = 1
 no locks held by ksoftirqd/0/3.
 
 stack backtrace:
 CPU: 0 PID: 3 Comm: ksoftirqd/0 Tainted: GW  O  3.15.0-rc3 #213
 Hardware name: LENOVO 2776LEG/2776LEG, BIOS 6EET55WW (3.15 ) 12/19/2011
  0001 880232533bf0 813a5ee6 0006
  880232530090 880232533c20 81076b94 0081
   8802085ac000 88007fc8ea00 880232533c50
 Call Trace:
  [813a5ee6] dump_stack+0x4e/0x68
  [81076b94] lockdep_rcu_suspicious+0xfa/0x103
  [813978a6] __vlan_find_dev_deep+0x54/0x94
  [a04a1938] cdc_mbim_rx_fixup+0x379/0x66a [cdc_mbim]
  [813ab76f] ? _raw_spin_unlock_irqrestore+0x3a/0x49
  [81079671] ? trace_hardirqs_on_caller+0x192/0x1a1
  [a059bd10] usbnet_bh+0x59/0x287 [usbnet]
  [8104067d] tasklet_action+0xbb/0xcd
  [81040057] __do_softirq+0x14c/0x30d
  [81040237] run_ksoftirqd+0x1f/0x50
  [8105f13e] smpboot_thread_fn+0x172/0x18e
  [8105efcc] ? SyS_setgroups+0xdf/0xdf
  [810594b0] kthread+0xb5/0xbd
  [813a84b1] ? __wait_for_common+0x13b/0x170
  [810593fb] ? __kthread_parkme+0x5c/0x5c
  [813b147c] ret_from_fork+0x7c/0xb0
  [810593fb] ? __kthread_parkme+0x5c/0x5c
 
 Fixes: 5b8f15f78e6f (net: cdc_mbim: handle IPv6 Neigbor Solicitations)
 Signed-off-by: Bjørn Mork bj...@mork.no
 ---
 Please add this to the stable v3.13 and v3.14 queues as well.  Thanks.
 
 
  drivers/net/usb/cdc_mbim.c | 13 -
  1 file changed, 8 insertions(+), 5 deletions(-)
 
 diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
 index c9f3281506af..34b5c5cc27ed 100644
 --- a/drivers/net/usb/cdc_mbim.c
 +++ b/drivers/net/usb/cdc_mbim.c
 @@ -204,13 +204,16 @@ static void do_neigh_solicit(struct usbnet *dev, u8 
 *buf, u16 tci)
   return;
  
   /* need to send the NA on the VLAN dev, if any */
 - if (tci)
 + if (tci) {
 + rcu_read_lock();
   netdev = __vlan_find_dev_deep(dev-net, htons(ETH_P_8021Q),
 -   tci);
 - else
 +   tci  VLAN_VID_MASK);
 + rcu_read_unlock();
 + if (!netdev)
 + return;
 + } else {
   netdev = dev-net;
 - if (!netdev)
 - return;
 + }
  
   in6_dev = in6_dev_get(netdev);
   if (!in6_dev)


While this 'removes' the warning, this doesn't solve the fundamental
problem.

If you write :

rcu_read_lock();
netdev = __vlan_find_dev_deep(...)
rcu_read_unlock();

Then you cannot dereference netdev safely after the unlock.

In order to do so, you need to take a reference on netdev (aka
dev_hold()) before doing rcu_read_unlock();

And of course, release it later (aka dev_put()) when you are done with
netdev.




--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH net-next 08/12] r8152: support TSO

[Eric Dumazet]

On Tue, 2014-03-04 at 14:35 +, David Laight wrote:

 Does that mean you are splitting the 64k 'ethernet packet' from TCP
 is software? I've looked at the ax88179 where the hardware can do it.
 
 Is there really a gain doing segmentation here if you have to do the
 extra data copy?

There is no 'extra' copy.

There is _already_ a copy, so what's the deal of doing this copy in a SG
enabled way ?



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 08/12] r8152: support TSO

[Eric Dumazet]

On Tue, 2014-03-04 at 20:01 +0800, Hayes Wang wrote:

 +static u32 r8152_xmit_frags(struct r8152 *tp, struct sk_buff *skb, u8 *data)
 +{
 + struct skb_shared_info *info = skb_shinfo(skb);
 + unsigned int cur_frag;
 + u32 total = skb_headlen(skb);
 +
 + memcpy(data, skb-data, total);
 + data += total;
 +
 + for (cur_frag = 0; cur_frag  info-nr_frags; cur_frag++) {
 + const skb_frag_t *frag = info-frags + cur_frag;
 + void *addr;
 + u32 len;
 +
 + len = skb_frag_size(frag);
 + addr = skb_frag_address(frag);
 + memcpy(data, addr, len);
 + data += len;
 + total += len;
   }
 +
 + return total;
  }
  

I would rather use skb_copy_bits(), because it correctly handles
kmap() case. (If a frag resides in high memory)



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 08/12] r8152: support TSO

[Eric Dumazet]

On Tue, 2014-03-04 at 20:01 +0800, Hayes Wang wrote:
 Support scatter gather and TSO.

  
 - netdev-features |= NETIF_F_RXCSUM | NETIF_F_IP_CSUM;
 - netdev-hw_features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM;
 + netdev-features |= NETIF_F_RXCSUM | NETIF_F_IP_CSUM | NETIF_F_SG |
 + NETIF_F_TSO;
 + netdev-hw_features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM | NETIF_F_SG |
 +   NETIF_F_TSO;
  

Minor nit :

If you use skb_copy_bits(), then you also can add NETIF_F_FRAGLIST here.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 09/12] r8152: support IPv6

[Eric Dumazet]

On Tue, 2014-03-04 at 20:01 +0800, Hayes Wang wrote:
 Support hw IPv6 checksum for TCP and UDP packets.


 +/*
 + * r8152_csum_workaround()
 + * The hw limites the value the transport offset. When the offset is out of 
 the
 + * range, calculate the checksum by sw.
 + */
 +static void r8152_csum_workaround(struct r8152 *tp, struct sk_buff *skb,
 +   struct sk_buff_head *list)
 +{
 + if (skb_shinfo(skb)-gso_size) {
 + netdev_features_t features = tp-netdev-features;
 + struct sk_buff *segs, *nskb;
 +
 + features = ~(NETIF_F_IP_CSUM | NETIF_F_SG | NETIF_F_TSO);
 + segs = skb_gso_segment(skb, features);
 + if (IS_ERR(segs) || !segs)
 + goto drop;
 +
 + do {
 + nskb = segs;
 + segs = segs-next;
 + nskb-next = NULL;
 + __skb_queue_head(list, nskb);

This introduces TCP reordering.

You should use some kind of skb_queue_splice()



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: sk == 0xffffffff fix - not for commit

[Eric Dumazet]

On Thu, 2014-01-16 at 16:21 +0100, Andrzej Pietrasiewicz wrote:
 W dniu 10.12.2013 15:25, Eric Dumazet pisze:
  On Tue, 2013-12-10 at 07:55 +0100, Andrzej Pietrasiewicz wrote:
  W dniu 09.12.2013 16:31, Eric Dumazet pisze:
  On Mon, 2013-12-09 at 12:47 +0100, Andrzej Pietrasiewicz wrote:
  NOT FOR COMMITTING TO MAINLINE.
 
  With g_ether loaded the sk occasionally becomes 0x.
  It happens usually after transferring few hundreds of kilobytes to few
  tens of megabytes. If sk is 0x then dereferencing it causes
  kernel panic.
 
  This is a *workaround*. I don't know enough net code to understand the 
  core
  of the problem. However, with this patch applied the problems are gone,
  or at least pushed farther away.
 
  Is it happening on SMP or UP ?
 
  UP build, S5PC110
 
  OK
 
  I believe you need additional debugging to track the exact moment
  0x is fed to 'sk'
 
  It looks like a very strange bug, involving a problem in some assembly
  helper, register save/restore, compiler bug or stack corruption or
  something.
 
 
 I started with adding WARN_ON(sk == 0x); just before return in
 __inet_lookup_established(), and the problem was gone. So this looks
 very strange, like a toolchain problem.

Or a timing issue. Adding a WARN_ON() adds extra instructions and might
really change the assembly output.

 
 I used gcc-linaro-arm-linux-gnueabihf-4.8-2013.05.
 
 If I change the toolchain to
 
 gcc-linaro-arm-linux-gnueabihf-4.7-2013.04-20130415
 
 the problem seems to have gone away.

Its totally possible some barrier was not properly handled by the
compiler. You could disassemble the function on both toolchains and
try to spot the issue.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: sk == 0xffffffff fix - not for commit

[Eric Dumazet]

On Tue, 2013-12-10 at 07:55 +0100, Andrzej Pietrasiewicz wrote:
 W dniu 09.12.2013 16:31, Eric Dumazet pisze:
  On Mon, 2013-12-09 at 12:47 +0100, Andrzej Pietrasiewicz wrote:
  NOT FOR COMMITTING TO MAINLINE.
 
  With g_ether loaded the sk occasionally becomes 0x.
  It happens usually after transferring few hundreds of kilobytes to few
  tens of megabytes. If sk is 0x then dereferencing it causes
  kernel panic.
 
  This is a *workaround*. I don't know enough net code to understand the core
  of the problem. However, with this patch applied the problems are gone,
  or at least pushed farther away.
 
  Is it happening on SMP or UP ?
 
 UP build, S5PC110

OK

I believe you need additional debugging to track the exact moment
0x is fed to 'sk'

It looks like a very strange bug, involving a problem in some assembly
helper, register save/restore, compiler bug or stack corruption or
something.

You should not have more than 150 instructions to decode, including
__inet_lookup_established()

Since __inet_lookup_established() dereferences the socket pointer, I do
not see why it would crash ~20 instructions _later_



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: sk == 0xffffffff fix - not for commit

[Eric Dumazet]

On Mon, 2013-12-09 at 12:47 +0100, Andrzej Pietrasiewicz wrote:
 NOT FOR COMMITTING TO MAINLINE.
 
 With g_ether loaded the sk occasionally becomes 0x.
 It happens usually after transferring few hundreds of kilobytes to few
 tens of megabytes. If sk is 0x then dereferencing it causes
 kernel panic.
 
 This is a *workaround*. I don't know enough net code to understand the core
 of the problem. However, with this patch applied the problems are gone,
 or at least pushed farther away.

Is it happening on SMP or UP ?

Crash should happen earlier in __inet_lookup_established()


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.

[Eric Dumazet]

On Wed, 2013-11-20 at 09:36 +, David Laight wrote:

 Ben said the largest number of fragments from the current network
 stack will be 17, and that none of them will cross 32k boundaries.
 So the network stack won't send down long SG lists.

Please note that skb-head itself _might_ cross a 32K or 64K boundary :

skb-head is kmalloc() provided, and SLUB can be tweaked
(slub_max_order) to use very high order pages.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] net: asix: Move declaration of ax88172a_info to shared header

[Eric Dumazet]

On Fri, 2013-08-09 at 10:40 -0700, Stephen Hemminger wrote:
 On Fri, 9 Aug 2013 14:39:06 -0300
 Fabio Estevam feste...@gmail.com wrote:
 
  On Fri, Aug 9, 2013 at 2:31 PM, Mark Brown broo...@kernel.org wrote:
   From: Mark Brown broo...@linaro.org
  
   Ensure that the definition of ax88172a_info matches the declaration seen
   by users and silence sparse warnings about symbols without declarations
   in the global namespace by moving the declaration into the shared header
   asix.h.
  
   Signed-off-by: Mark Brown broo...@linaro.org
   ---
drivers/net/usb/asix.h | 2 ++
drivers/net/usb/asix_devices.c | 2 --
2 files changed, 2 insertions(+), 2 deletions(-)
  
   diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
   index 346c032..bdaa12d 100644
   --- a/drivers/net/usb/asix.h
   +++ b/drivers/net/usb/asix.h
   @@ -178,6 +178,8 @@ struct asix_common_private {
   struct asix_rx_fixup_info rx_fixup_info;
};
  
   +extern const struct driver_info ax88172a_info;
  
  You could drop the 'extern' here.
  
  All other function prototypes in this header file do not use 'extern'.
 
 That is data, not function prototype, so yes extern is needed.


And this kind of contradictions show why extern declarations make sense
in include files, for text or/and data.

Some compiler folk decided 'extern' were not mandatory for code, but its
really adding confusion and endless discussions.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma

[Eric Dumazet]

On Thu, 2013-08-08 at 21:48 +0800, Ming Lei wrote:
 This patch enables 'can_dma_sg' flag for ax88179_178a device
 if the attached host controller supports building packet from
 discontinuous buffers(DMA SG is possible), so TSO can be enabled
 and skb fragment buffers can be passed to usb stack via urb-sg
 directly.
 
 With the patch, system CPU utilization decreased ~50% and throughput
 increased by ~10% when doing iperf client test on one ARM A15 dual
 core board.
 
 Cc: Eric Dumazet eric.duma...@gmail.com
 Cc: Ben Hutchings bhutchi...@solarflare.com
 Cc: Grant Grundler grund...@google.com
 Cc: Oliver Neukum oneu...@suse.de
 Cc: Alan Stern st...@rowland.harvard.edu
 Cc: Freddy Xin fre...@asix.com.tw
 Signed-off-by: Ming Lei ming@canonical.com
 ---
  drivers/net/usb/ax88179_178a.c |8 
  1 file changed, 8 insertions(+)

Acked-by: Eric Dumazet eduma...@gmail.com

Thanks for doing this !


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] USBNET: support DMA SG

[Eric Dumazet]

On Thu, 2013-08-08 at 21:48 +0800, Ming Lei wrote:
 This patch introduces support of DMA SG if the USB host controller
 which usbnet device is attached to is capable of building packet from
 discontinuous buffers.


 diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
 index 8fbc008..9cb2fe8 100644
 --- a/include/linux/usb/usbnet.h
 +++ b/include/linux/usb/usbnet.h
 @@ -35,6 +35,7 @@ struct usbnet {
   unsigned char   suspend_count;
   unsigned char   pkt_cnt, pkt_err;
   unsigned short  rx_qlen, tx_qlen;
 + unsignedcan_dma_sg:1;

We try to use unsigned int instead of plain unsigned, but its a
minor point and should not block your patches.

Apart from this :

Reviewed-by: Eric Dumazet eduma...@google.com


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma

[Eric Dumazet]

On Tue, 2013-08-06 at 08:52 +0800, Ming Lei wrote:
 This patch enables 'can_dma_sg' flag for ax88179_178a device
 if the attached host controller supports building packet from
 discontinuous buffers(DMA SG is possible), so TSO can be enabled
 and skb fragment buffers can be passed to usb stack via urb-sg
 directly.
 
 With the patch, system CPU utilization decreased ~50% and throughput
 increased by ~10% when doing iperf client test on one ARM A15 dual
 core board.
 

Nice ;)

  AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
 @@ -1310,6 +1318,10 @@ static int ax88179_reset(struct usbnet *dev)
  
   dev-net-hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
NETIF_F_RXCSUM;
 + if (dev-can_dma_sg) {
 + dev-net-features |= NETIF_F_SG | NETIF_F_TSO;
 + dev-net-hw_features |= NETIF_F_SG | NETIF_F_TSO;
 + }
  

My concern with setting TSO on reset() is the following :

Admin can disable TSO with

ethtool -K ethX tso off


Then, one hour later, or one month later, a reset happens, and this code
magically re-enables TSO

So, I really think this part should be removed from your patch.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Eric Dumazet]

On Thu, 2013-08-01 at 16:10 +0800, Ming Lei wrote:
 On Thu, Aug 1, 2013 at 1:04 PM, Eric Dumazet eric.duma...@gmail.com wrote:
  On Thu, 2013-08-01 at 12:41 +0800, Ming Lei wrote:
 
  From my trace result, lots of linear SKBs are cloned or header-cloned, so
  it needs skb copy too.
 
  Is it normal in xmit path to see cloned SKBs for driver? If not, I can add 
  check
  to avoid allocation of 8 bytes header for non-cloned skb.
 
  Existing code is not very friendly and very complex.
 
  Sure TCP stack does a clone for every skb from socket write queue,
  but header should be available for pushing 8 bytes.
 
  The !skb_cloned(skb) test should be removed if the memmove() is not
  needed.
 
  Could you try following patch ?
 
 Tested-by: Ming Lei ming@canonical.com
 
 This patch does work, and it will make the patch 4/4 become very
 simple, :-)
 
 I will rewrite this one against your patch.

OK I'll post an official patch then. I presume you got performance
increase, since we no longer do a copy of the TCP packets.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next] ax88179_178a: avoid copy of tx tcp packets

[Eric Dumazet]

From: Eric Dumazet eduma...@google.com

ax88179_tx_fixup() has quite complex code trying to push 8 bytes
of control data (len/mss), but fails to do it properly for TCP packets,
incurring an extra copy and point of memory allocation failure.

Lets use the simple and approved way.

dev-needed_headroom being 8, all frames should have 8 bytes of
headroom, so the extra copy should be unlikely anyway.

This patch should improve performance for TCP xmits.

Reported-by: Ming Lei ming@canonical.com
Tested-by: Ming Lei ming@canonical.com
Signed-off-by: Eric Dumazet eduma...@google.com
---
 drivers/net/usb/ax88179_178a.c |   21 -
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 2bc87e3..e2120d6 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1166,31 +1166,18 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff 
*skb, gfp_t flags)
int frame_size = dev-maxpacket;
int mss = skb_shinfo(skb)-gso_size;
int headroom;
-   int tailroom;
 
tx_hdr1 = skb-len;
tx_hdr2 = mss;
if (((skb-len + 8) % frame_size) == 0)
tx_hdr2 |= 0x80008000;  /* Enable padding */
 
-   headroom = skb_headroom(skb);
-   tailroom = skb_tailroom(skb);
+   headroom = skb_headroom(skb) - 8;
 
-   if (!skb_header_cloned(skb) 
-   !skb_cloned(skb) 
-   (headroom + tailroom) = 8) {
-   if (headroom  8) {
-   skb-data = memmove(skb-head + 8, skb-data, skb-len);
-   skb_set_tail_pointer(skb, skb-len);
-   }
-   } else {
-   struct sk_buff *skb2;
-
-   skb2 = skb_copy_expand(skb, 8, 0, flags);
+   if ((skb_header_cloned(skb) || headroom  0) 
+   pskb_expand_head(skb, headroom  0 ? 8 : 0, 0, GFP_ATOMIC)) {
dev_kfree_skb_any(skb);
-   skb = skb2;
-   if (!skb)
-   return NULL;
+   return NULL;
}
 
skb_push(skb, 4);


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] ax88179_178a: avoid copy of tx tcp packets

[Eric Dumazet]

On Thu, 2013-08-01 at 06:49 -0700, Eric Dumazet wrote:
 From: Eric Dumazet eduma...@google.com
 
 ax88179_tx_fixup() has quite complex code trying to push 8 bytes
 of control data (len/mss), but fails to do it properly for TCP packets,
 incurring an extra copy and point of memory allocation failure.

I forgot to say this patch is for net-next, but depends on following fix
from net tree being applied.

commit 20f0170377264e8449b6987041f0bcc4d746d3ed
(usbnet: do not pretend to support SG/TSO)



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Eric Dumazet]

On Thu, 2013-08-01 at 08:30 -0700, Grant Grundler wrote:

 http://lxr.free-electrons.com/source/include/linux/byteorder/generic.h#L111
 http://lxr.free-electrons.com/ident?i=__cpu_to_le32s
 
 IIRC, cpu_to_leXX() macros return the endian corrected value.
 In other words, they need to be assigned to something, no?
 

Nope, this in in-place byte swapping (for Big Endian only)


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] USBNET: ax88179_178a: enable tso if host supports sg dma

[Eric Dumazet]

On Wed, 2013-07-31 at 16:02 +0200, Oliver Neukum wrote:
 On Wed, 2013-07-31 at 21:50 +0800, Ming Lei wrote:
 
  In the usbnet case, the driver already supports non-sg well. Actually,
  all current drivers should support non-sg well because urb-sg wasn't
  introduced for very long time. We can think it as a new feature or DMA
  enhancement for xHCI controller.
  
  If you mean buffer debounce for dma sg support on ehci/uhci/ohci/..,
  maybe we need to discuss and evaluate further.
 
 We cannot lie to the networking layer. Either we can do sg in hardware
 or we cannot. This is unfortunately determined by the HC not the device
 on the bus. How else but from the host driver would we get the
 information?
 

Hmm, I would rather make sure SG is really supported before adding TSO
support.

TCP stack can build skb with fragments of any size, not multiple
of 512 or 1024 bytes.

commit 20f0170377264e8449b6987041f0bcc4d746d3ed

usbnet: do not pretend to support SG/TSO

usbnet doesn't support yet SG, so drivers should not advertise SG or TSO
capabilities, as they allow TCP stack to build large TSO packets that
need to be linearized and might use order-5 pages.

This adds an extra copy overhead and possible allocation failures.

Current code ignore skb_linearize() return code so crashes are even
possible.

Best is to not pretend SG/TSO is supported, and add this again when/if
usbnet really supports SG for devices who could get a performance gain.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] USB USBNET: loose DMA SG check and support usbnet DMA SG

[Eric Dumazet]

On Wed, 2013-07-31 at 18:51 +0800, Ming Lei wrote:
 Hi,
 
 This patchset allows drivers to pass sg buffers which size can't be divided
 by max packet size of endpoint if the host controllers support this kind
 of sg buffers.
 
 Previously we add check[1] on the situation and don't allow these sg buffers
 passed to USB HCD, looks the check is too strict because some 
 applications(such
 as network stack) can't provide this sg buffers which size can be divided by
 max packet size, so this patch looses the check in case that the host 
 controllers
 support it.
 
 Also patch 3/4 implements DMA SG on usbnet driver, and patch 4/4
 enables it on ax88179_178a USB3 NIC, so CPU utilization can be
 decreased much with usbnet DMA SG when the USB3 NIC is attached to
 xHCI controller.

This looks very promising to me, thanks a lot for doing this Ming.


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Thu, 2013-07-25 at 13:25 +0800, Ming Lei wrote:
 On Thu, Jul 25, 2013 at 1:10 PM, Eric Dumazet eric.duma...@gmail.com wrote:
  On Thu, 2013-07-25 at 10:28 +0800, Ming Lei wrote:
 
 
  It depends if size of sg buffer(except for last one) in the sg list can be
  divided by usb endpoint's max packet size(512 or 1024), at least there
  is the constraint:
 
  http://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-nextid=10e232c597ac757e7f8600649f7e872e86de190f
 
  I am wondering if network stack can meet that.  If not, it might be a
  bit difficult
  because lots of USB host controller don't support that, and driver may have
  to support SG and non-SG at the same time for working well on all HCs.
 
  I do not see the problem.
 
  If one skb has 2 fragments of 32KB, couldn't they be split into 64 1K
  segments by the device driver ?
 
 OK, if length of fragments of all SKBs from network stack can always guarantee
 to be divided by 1024, that is fine,  seems I worry about too much, :-)

Unfortunately, there is no such guarantee. TSO permits sendfile() zero
copy operation, so the frags can be of any size, any offset...

In this mode, the first element (skb-head) will typically contains the
headers, and there are way below 512 bytes.

So even with lowering netdev-gso_max_size under PAGE_SIZE, most of the
packets will need to be copied into a single segment.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Thu, 2013-07-25 at 22:52 +0800, Ming Lei wrote:

 Maybe need to try it with TSO enabled, in my test on ax88179_178a NIC after
 applying your disabling TSO patch, tx throughput is less than 600Mbps, but rx
 is close to 900Mbps.

It looks like TCP stack could for this case allocate linear skbs
(GFP_KERNEL context), using order-3 pages, and not adding frags on them,
to avoid the skb_linearize() hazard (in GFP_ATOMIC)

In case of retransmits, skb are small (one MSS) so the skb_linearize()
should succeed most of the time.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Thu, 2013-07-25 at 10:28 +0800, Ming Lei wrote:

 
 It depends if size of sg buffer(except for last one) in the sg list can be
 divided by usb endpoint's max packet size(512 or 1024), at least there
 is the constraint:
 
 http://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-nextid=10e232c597ac757e7f8600649f7e872e86de190f
 
 I am wondering if network stack can meet that.  If not, it might be a
 bit difficult
 because lots of USB host controller don't support that, and driver may have
 to support SG and non-SG at the same time for working well on all HCs.

I do not see the problem.

If one skb has 2 fragments of 32KB, couldn't they be split into 64 1K
segments by the device driver ?



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Mon, 2013-07-22 at 20:47 +0100, Ben Hutchings wrote:
 On Mon, 2013-07-22 at 11:47 -0700, Eric Dumazet wrote:
  On Mon, 2013-07-22 at 19:38 +0100, Ben Hutchings wrote:
   On Mon, 2013-07-22 at 11:29 -0700, Grant Grundler wrote:
On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet eric.duma...@gmail.com 
wrote:
...
 I guess that if a driver does not advertise NETIF_F_SG, this
 skb_linearize() call is not needed : All frames reaching your xmit
 function should already be linear

As Ben Hutchings pointed out, hw_features is still setting this...but
I'm not sure how that matters.

ax88179_set_features() doesn't allow setting SG or TSO features.  But
I expect it would be not too difficult to add such that ethtool
could set those features after boot.
   [...]
   
   It already can.  That's what putting feature flags in hw_features does.
  
  My original concern, that inspired this patch, was to remove SG support,
  as this driver does not have SG support at all.
  
  Linearize a full TSO packet needs order-5 allocations, thats likely to
  fail and lead to very slow TCP performance, because it will only rely on
  retransmits.
 
 The driver could set gso_max_size to reduce that problem.  But I rather
 doubt that TSO followed by skb_linearize() significantly improves
 throughput or CPU-efficiency.  (If the device has a 1G link but is
 connected to the host through a USB 2.0 port, then USB is the bottleneck
 and TSO could improve throughput a few percent.  But that's a silly
 configuration.)
 
 The real solution would be for someone to add SG support to the usbnet
 core.  Trying to support 1GbE with only linear skbs is not a great
 idea... and it can only be a matter of time before there is USB ultra
 speed (or whatever comes after 'super') with 10GbE devices...
 

This sounds a good idea.

Is anybody working on adding SG to usbnet ?



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Tue, 2013-07-23 at 16:46 -0700, David Miller wrote:
 From: Eric Dumazet eric.duma...@gmail.com
 Date: Mon, 22 Jul 2013 23:10:27 -0700
 
  On Mon, 2013-07-22 at 20:47 +0100, Ben Hutchings wrote:
  The real solution would be for someone to add SG support to the usbnet
  core.  Trying to support 1GbE with only linear skbs is not a great
  idea... and it can only be a matter of time before there is USB ultra
  speed (or whatever comes after 'super') with 10GbE devices...
  
  
  This sounds a good idea.
  
  Is anybody working on adding SG to usbnet ?
 
 This is a good long-term plan, but right now we have to do something
 reasonable.
 
 A quick scan shows that smsc75xx, smsc95xx, and ax88179_178a all have
 this problem.
 
 Instead of the patch starting this thread, I'd like to see one that
 hits all three drivers and removes all SG and TSO features bits from
 both the -features _and_ -hw_features settings.
 
 Could someone toss that together quickly?

Yes, I can prepare this patch.


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Tue, 2013-07-23 at 16:56 -0700, Eric Dumazet wrote:

  A quick scan shows that smsc75xx, smsc95xx, and ax88179_178a all have
  this problem.
  
  Instead of the patch starting this thread, I'd like to see one that
  hits all three drivers and removes all SG and TSO features bits from
  both the -features _and_ -hw_features settings.
  
  Could someone toss that together quickly?
 
 Yes, I can prepare this patch.

Looks like only ax88179_178a  smsc75xx are impacted.


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbnet: do not pretend to support SG/TSO

[Eric Dumazet]

From: Eric Dumazet eduma...@google.com

usbnet doesn't support yet SG, so drivers should not advertise SG or TSO
capabilities, as they allow TCP stack to build large TSO packets that 
need to be linearized and might use order-5 pages.

This adds an extra copy overhead and possible allocation failures.

Current code ignore skb_linearize() return code so crashes are even
possible.

Best is to not pretend SG/TSO is supported, and add this again when/if
usbnet really supports SG for devices who could get a performance gain.

Based on a prior patch from Freddy Xin fre...@asix.com.tw

Signed-off-by: Eric Dumazet eduma...@google.com
---
 drivers/net/usb/ax88179_178a.c |9 -
 drivers/net/usb/smsc75xx.c |   12 +++-
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 1e3c302..2bc87e3 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1029,10 +1029,10 @@ static int ax88179_bind(struct usbnet *dev, struct 
usb_interface *intf)
dev-mii.supports_gmii = 1;
 
dev-net-features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
- NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
+ NETIF_F_RXCSUM;
 
dev-net-hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
+NETIF_F_RXCSUM;
 
/* Enable checksum offload */
*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
@@ -1173,7 +1173,6 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, 
gfp_t flags)
if (((skb-len + 8) % frame_size) == 0)
tx_hdr2 |= 0x80008000;  /* Enable padding */
 
-   skb_linearize(skb);
headroom = skb_headroom(skb);
tailroom = skb_tailroom(skb);
 
@@ -1317,10 +1316,10 @@ static int ax88179_reset(struct usbnet *dev)
  1, 1, tmp);
 
dev-net-features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
- NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
+ NETIF_F_RXCSUM;
 
dev-net-hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
+NETIF_F_RXCSUM;
 
/* Enable checksum offload */
*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 7540974..66ebbac 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -45,7 +45,6 @@
 #define EEPROM_MAC_OFFSET  (0x01)
 #define DEFAULT_TX_CSUM_ENABLE (true)
 #define DEFAULT_RX_CSUM_ENABLE (true)
-#define DEFAULT_TSO_ENABLE (true)
 #define SMSC75XX_INTERNAL_PHY_ID   (1)
 #define SMSC75XX_TX_OVERHEAD   (8)
 #define MAX_RX_FIFO_SIZE   (20 * 1024)
@@ -1410,17 +1409,14 @@ static int smsc75xx_bind(struct usbnet *dev, struct 
usb_interface *intf)
 
INIT_WORK(pdata-set_multicast, smsc75xx_deferred_multicast_write);
 
-   if (DEFAULT_TX_CSUM_ENABLE) {
+   if (DEFAULT_TX_CSUM_ENABLE)
dev-net-features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-   if (DEFAULT_TSO_ENABLE)
-   dev-net-features |= NETIF_F_SG |
-   NETIF_F_TSO | NETIF_F_TSO6;
-   }
+
if (DEFAULT_RX_CSUM_ENABLE)
dev-net-features |= NETIF_F_RXCSUM;
 
dev-net-hw_features = NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-   NETIF_F_SG | NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_RXCSUM;
+   NETIF_F_RXCSUM;
 
ret = smsc75xx_wait_ready(dev, 0);
if (ret  0) {
@@ -2200,8 +2196,6 @@ static struct sk_buff *smsc75xx_tx_fixup(struct usbnet 
*dev,
 {
u32 tx_cmd_a, tx_cmd_b;
 
-   skb_linearize(skb);
-
if (skb_headroom(skb)  SMSC75XX_TX_OVERHEAD) {
struct sk_buff *skb2 =
skb_copy_expand(skb, SMSC75XX_TX_OVERHEAD, 0, flags);


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Sat, 2013-07-20 at 17:16 +0800, fre...@asix.com.tw wrote:
 From: Freddy Xin fre...@asix.com.tw
 
 Disable TSO and SG network features in reset() and bind() functions,
 and check the return value of skb_linearize() in tx_fixup() to prevent
 TX throttling.
 
 Signed-off-by: Freddy Xin fre...@asix.com.tw
 ---
  drivers/net/usb/ax88179_178a.c | 8 +---
  1 file changed, 5 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
 index 1e3c302..eb71331 100644
 --- a/drivers/net/usb/ax88179_178a.c
 +++ b/drivers/net/usb/ax88179_178a.c
 @@ -1029,7 +1029,7 @@ static int ax88179_bind(struct usbnet *dev, struct 
 usb_interface *intf)
   dev-mii.supports_gmii = 1;
  
   dev-net-features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 -   NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
 +   NETIF_F_RXCSUM;
  
   dev-net-hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
 @@ -1173,7 +1173,9 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff 
 *skb, gfp_t flags)
   if (((skb-len + 8) % frame_size) == 0)
   tx_hdr2 |= 0x80008000;  /* Enable padding */
  
 - skb_linearize(skb);
 + if (skb_linearize(skb))
 + return NULL;
 +
   

I guess that if a driver does not advertise NETIF_F_SG, this
skb_linearize() call is not needed : All frames reaching your xmit
function should already be linear

Thanks


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Mon, 2013-07-22 at 19:38 +0100, Ben Hutchings wrote:
 On Mon, 2013-07-22 at 11:29 -0700, Grant Grundler wrote:
  On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet eric.duma...@gmail.com 
  wrote:
  ...
   I guess that if a driver does not advertise NETIF_F_SG, this
   skb_linearize() call is not needed : All frames reaching your xmit
   function should already be linear
  
  As Ben Hutchings pointed out, hw_features is still setting this...but
  I'm not sure how that matters.
  
  ax88179_set_features() doesn't allow setting SG or TSO features.  But
  I expect it would be not too difficult to add such that ethtool
  could set those features after boot.
 [...]
 
 It already can.  That's what putting feature flags in hw_features does.

My original concern, that inspired this patch, was to remove SG support,
as this driver does not have SG support at all.

Linearize a full TSO packet needs order-5 allocations, thats likely to
fail and lead to very slow TCP performance, because it will only rely on
retransmits.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: changing usbnet's API to better deal with cdc-ncm

[Eric Dumazet]

On Thu, 2012-09-06 at 10:50 +0200, Oliver Neukum wrote: 
 On Thursday 06 September 2012 10:13:01 Alexey ORISHKO wrote:

  Rx path:
  4. IP packets are cloned to separate skb and length of actual data
 set to eth packet size, while skb size is still the same as skb
 containing full NTB frame. This causes problems with TCP stack when
 throughput is high because of flow control in the stack, if too much
 data allocated.
  Someone suggested a patch for it, which was rejected. Anyway, I
 don't think copy data to a new skb would be a nice solution, but
 keeping several clones with size equal to incoming skb in not good
 either. 
 
 Perhaps the problem is using an skb for aggregate reception at all.
 Possibly enough buffers of fixed size should be allocated on open and
 reused,
 not freed.

Really skb_clone() use should be removed from cdc_ncm_rx_fixup()

Unless you expect 10Gbit speed from this driver, skb_clone() is the
worst possible strategy.

Allocating fresh skbs of the right size permits better memory use and
allows TCP coalescing as well.

The use of skb_clone() forces some parts of the stack to perform a full
copy anyway.

It's still unclear to me with we use up to 32KB blocks in USB drivers...


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: changing usbnet's API to better deal with cdc-ncm

[Eric Dumazet]

On Thu, 2012-09-06 at 11:11 +0200, Eric Dumazet wrote:

 Really skb_clone() use should be removed from cdc_ncm_rx_fixup()
 
 Unless you expect 10Gbit speed from this driver, skb_clone() is the
 worst possible strategy.
 
 Allocating fresh skbs of the right size permits better memory use and
 allows TCP coalescing as well.
 
 The use of skb_clone() forces some parts of the stack to perform a full
 copy anyway.
 
 It's still unclear to me with we use up to 32KB blocks in USB drivers...
 

So I advise testing this patch :

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 4cd582a..c0821f7 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1052,12 +1052,11 @@ static int cdc_ncm_rx_fixup(struct usbnet *dev, struct 
sk_buff *skb_in)
break;
 
} else {
-   skb = skb_clone(skb_in, GFP_ATOMIC);
+   skb = netdev_alloc_skb_ip_align(dev-net, len);
if (!skb)
goto error;
-   skb-len = len;
-   skb-data = ((u8 *)skb_in-data) + offset;
-   skb_set_tail_pointer(skb, len);
+   skb_put(skb, len);
+   memcpy(skb-data, skb_in-data + offset, len);
usbnet_skb_return(dev, skb);
}
}


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html