rndis gadget: Inconsistent locking

rndis gadget: Inconsistent locking

[Neil Jones]

Hi,

Im getting another lockdep warning when using the RNDIS gadget:

 WARNING: at kernel/softirq.c:98 ___local_bh_disable+0xc4/0xd0()
 Modules linked in: g_ether

 Call trace:
 [<40003bf8>] _show_stack+0x68/0x7c
 [<40003c20>] _dump_stack+0x14/0x28
 [<40013c3c>] _warn_slowpath_common+0x5c/0x7c
 [<40013c74>] _warn_slowpath_null+0x18/0x2c
 [<4001b17c>] ___local_bh_disable+0xc0/0xd0
 [<4001b1a0>] _local_bh_disable+0x14/0x28
 [<402e57f8>] __raw_spin_lock_bh+0x18/0x54
 [<40257f4c>] _dev_txq_stats_fold+0x7c/0x13c
 [<402580c4>] _dev_get_stats+0xb8/0xc0
 [<781d4e60>] _rndis_msg_parser+0x288/0xa04 [g_ether]
 [<781d5600>] _rndis_command_complete+0x24/0x70 [g_ether]
 [<401d66fc>] _dwc_otg_request_done+0xd8/0x220
 [<401d928c>] _ep0_complete_request+0x3f4/0x578
 [<401d95bc>] _handle_ep0+0x1ac/0x146c
 [<401daf7c>] _dwc_otg_pcd_handle_in_ep_intr+0x1c0/0x8bc
 [<401db8dc>] _dwc_otg_pcd_handle_intr+0x264/0x294
 [<401d6288>] _dwc_otg_pcd_irq+0x10/0x30
 [<40054cf4>] _handle_IRQ_event+0x4c/0x184
 [<40057b4c>] _handle_level_irq+0xac/0x15c
 [<4000b204>] _metag_soc_irq_demux+0xac/0xb4
 [<40002dd4>] _do_IRQ+0x4c/0x78
 [<40004000>] _trigger_handler+0x38/0xac
 [<40000b18>] ___TBIBoingVec+0xc/0x10
 [<40003588>] _cpu_idle+0x54/0x78

 no locks held by swapper/0.
 ---[ end trace 77ac3cfee0ae5b25 ]---

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

Re: rndis gadget: Inconsistent locking

[Neil Jones]

Sorry, stupid gmail...

Hi,

Im getting another lockdep warning when using the RNDIS gadget:

 WARNING: at kernel/softirq.c:98 ___local_bh_disable+0xc4/0xd0()
 Modules linked in: g_ether

 Call trace:
 [<40003bf8>] _show_stack+0x68/0x7c
 [<40003c20>] _dump_stack+0x14/0x28
 [<40013c3c>] _warn_slowpath_common+0x5c/0x7c
 [<40013c74>] _warn_slowpath_null+0x18/0x2c
 [<4001b17c>] ___local_bh_disable+0xc0/0xd0
 [<4001b1a0>] _local_bh_disable+0x14/0x28
 [<402e57f8>] __raw_spin_lock_bh+0x18/0x54
 [<40257f4c>] _dev_txq_stats_fold+0x7c/0x13c
 [<402580c4>] _dev_get_stats+0xb8/0xc0
 [<781d4e60>] _rndis_msg_parser+0x288/0xa04 [g_ether]
 [<781d5600>] _rndis_command_complete+0x24/0x70 [g_ether]
 [<401d66fc>] _dwc_otg_request_done+0xd8/0x220
 [<401d928c>] _ep0_complete_request+0x3f4/0x578
 [<401d95bc>] _handle_ep0+0x1ac/0x146c
 [<401daf7c>] _dwc_otg_pcd_handle_in_ep_intr+0x1c0/0x8bc
 [<401db8dc>] _dwc_otg_pcd_handle_intr+0x264/0x294
 [<401d6288>] _dwc_otg_pcd_irq+0x10/0x30
 [<40054cf4>] _handle_IRQ_event+0x4c/0x184
 [<40057b4c>] _handle_level_irq+0xac/0x15c
 [<4000b204>] _metag_soc_irq_demux+0xac/0xb4
 [<40002dd4>] _do_IRQ+0x4c/0x78
 [<40004000>] _trigger_handler+0x38/0xac
 [<40000b18>] ___TBIBoingVec+0xc/0x10
 [<40003588>] _cpu_idle+0x54/0x78

 no locks held by swapper/0.
 ---[ end trace 77ac3cfee0ae5b25 ]---

It looks like we are calling spin_lock_bh in the completion function
which is running in hard_irq, I think the driver should defer handling
this msg (and maybe all requests) to a workqueue?

Cheers

Neil



On Wed, Dec 8, 2010 at 3:03 PM, Neil Jones <neiljay@gmail.com> wrote:
> Hi,
>
> Im getting another lockdep warning when using the RNDIS gadget:
>
>  WARNING: at kernel/softirq.c:98 ___local_bh_disable+0xc4/0xd0()
>  Modules linked in: g_ether
>
>  Call trace:
>  [<40003bf8>] _show_stack+0x68/0x7c
>  [<40003c20>] _dump_stack+0x14/0x28
>  [<40013c3c>] _warn_slowpath_common+0x5c/0x7c
>  [<40013c74>] _warn_slowpath_null+0x18/0x2c
>  [<4001b17c>] ___local_bh_disable+0xc0/0xd0
>  [<4001b1a0>] _local_bh_disable+0x14/0x28
>  [<402e57f8>] __raw_spin_lock_bh+0x18/0x54
>  [<40257f4c>] _dev_txq_stats_fold+0x7c/0x13c
>  [<402580c4>] _dev_get_stats+0xb8/0xc0
>  [<781d4e60>] _rndis_msg_parser+0x288/0xa04 [g_ether]
>  [<781d5600>] _rndis_command_complete+0x24/0x70 [g_ether]
>  [<401d66fc>] _dwc_otg_request_done+0xd8/0x220
>  [<401d928c>] _ep0_complete_request+0x3f4/0x578
>  [<401d95bc>] _handle_ep0+0x1ac/0x146c
>  [<401daf7c>] _dwc_otg_pcd_handle_in_ep_intr+0x1c0/0x8bc
>  [<401db8dc>] _dwc_otg_pcd_handle_intr+0x264/0x294
>  [<401d6288>] _dwc_otg_pcd_irq+0x10/0x30
>  [<40054cf4>] _handle_IRQ_event+0x4c/0x184
>  [<40057b4c>] _handle_level_irq+0xac/0x15c
>  [<4000b204>] _metag_soc_irq_demux+0xac/0xb4
>  [<40002dd4>] _do_IRQ+0x4c/0x78
>  [<40004000>] _trigger_handler+0x38/0xac
>  [<40000b18>] ___TBIBoingVec+0xc/0x10
>  [<40003588>] _cpu_idle+0x54/0x78
>
>  no locks held by swapper/0.
>  ---[ end trace 77ac3cfee0ae5b25 ]---
>
> It
>

Re: rndis gadget: Inconsistent locking

[Michał Nazarewicz]

On Wed, 08 Dec 2010 16:11:30 +0100, Neil Jones <neiljay@gmail.com> wrote:
> Im getting another lockdep warning when using the RNDIS gadget:
>
>  WARNING: at kernel/softirq.c:98 ___local_bh_disable+0xc4/0xd0()
>  Modules linked in: g_ether
>
>  Call trace:
>  [<40003bf8>] _show_stack+0x68/0x7c
>  [<40003c20>] _dump_stack+0x14/0x28
>  [<40013c3c>] _warn_slowpath_common+0x5c/0x7c
>  [<40013c74>] _warn_slowpath_null+0x18/0x2c
>  [<4001b17c>] ___local_bh_disable+0xc0/0xd0
>  [<4001b1a0>] _local_bh_disable+0x14/0x28
>  [<402e57f8>] __raw_spin_lock_bh+0x18/0x54
>  [<40257f4c>] _dev_txq_stats_fold+0x7c/0x13c
>  [<402580c4>] _dev_get_stats+0xb8/0xc0
>  [<781d4e60>] _rndis_msg_parser+0x288/0xa04 [g_ether]
>  [<781d5600>] _rndis_command_complete+0x24/0x70 [g_ether]
>  [<401d66fc>] _dwc_otg_request_done+0xd8/0x220
>  [<401d928c>] _ep0_complete_request+0x3f4/0x578
>  [<401d95bc>] _handle_ep0+0x1ac/0x146c
>  [<401daf7c>] _dwc_otg_pcd_handle_in_ep_intr+0x1c0/0x8bc
>  [<401db8dc>] _dwc_otg_pcd_handle_intr+0x264/0x294
>  [<401d6288>] _dwc_otg_pcd_irq+0x10/0x30
>  [<40054cf4>] _handle_IRQ_event+0x4c/0x184
>  [<40057b4c>] _handle_level_irq+0xac/0x15c
>  [<4000b204>] _metag_soc_irq_demux+0xac/0xb4
>  [<40002dd4>] _do_IRQ+0x4c/0x78
>  [<40004000>] _trigger_handler+0x38/0xac
>  [<40000b18>] ___TBIBoingVec+0xc/0x10
>  [<40003588>] _cpu_idle+0x54/0x78
>
>  no locks held by swapper/0.
>  ---[ end trace 77ac3cfee0ae5b25 ]---

Known problem.

> It looks like we are calling spin_lock_bh in the completion function
> which is running in hard_irq, I think the driver should defer handling
> this msg (and maybe all requests) to a workqueue?

Does this help: <https://patchwork.kernel.org/patch/195562/>?

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: rndis gadget: Inconsistent locking

[Neil Jones]

> Does this help: <https://patchwork.kernel.org/patch/195562/>?

Yes cheers, warning gone and driver seems fine so far.

has this been accepted upstream ?

Neil

2010/12/8 Michał Nazarewicz <m.nazarewicz@samsung.com>:
> On Wed, 08 Dec 2010 16:11:30 +0100, Neil Jones <neiljay@gmail.com> wrote:
>>
>> Im getting another lockdep warning when using the RNDIS gadget:
>>
>>  WARNING: at kernel/softirq.c:98 ___local_bh_disable+0xc4/0xd0()
>>  Modules linked in: g_ether
>>
>>  Call trace:
>>  [<40003bf8>] _show_stack+0x68/0x7c
>>  [<40003c20>] _dump_stack+0x14/0x28
>>  [<40013c3c>] _warn_slowpath_common+0x5c/0x7c
>>  [<40013c74>] _warn_slowpath_null+0x18/0x2c
>>  [<4001b17c>] ___local_bh_disable+0xc0/0xd0
>>  [<4001b1a0>] _local_bh_disable+0x14/0x28
>>  [<402e57f8>] __raw_spin_lock_bh+0x18/0x54
>>  [<40257f4c>] _dev_txq_stats_fold+0x7c/0x13c
>>  [<402580c4>] _dev_get_stats+0xb8/0xc0
>>  [<781d4e60>] _rndis_msg_parser+0x288/0xa04 [g_ether]
>>  [<781d5600>] _rndis_command_complete+0x24/0x70 [g_ether]
>>  [<401d66fc>] _dwc_otg_request_done+0xd8/0x220
>>  [<401d928c>] _ep0_complete_request+0x3f4/0x578
>>  [<401d95bc>] _handle_ep0+0x1ac/0x146c
>>  [<401daf7c>] _dwc_otg_pcd_handle_in_ep_intr+0x1c0/0x8bc
>>  [<401db8dc>] _dwc_otg_pcd_handle_intr+0x264/0x294
>>  [<401d6288>] _dwc_otg_pcd_irq+0x10/0x30
>>  [<40054cf4>] _handle_IRQ_event+0x4c/0x184
>>  [<40057b4c>] _handle_level_irq+0xac/0x15c
>>  [<4000b204>] _metag_soc_irq_demux+0xac/0xb4
>>  [<40002dd4>] _do_IRQ+0x4c/0x78
>>  [<40004000>] _trigger_handler+0x38/0xac
>>  [<40000b18>] ___TBIBoingVec+0xc/0x10
>>  [<40003588>] _cpu_idle+0x54/0x78
>>
>>  no locks held by swapper/0.
>>  ---[ end trace 77ac3cfee0ae5b25 ]---
>
> Known problem.
>
>> It looks like we are calling spin_lock_bh in the completion function
>> which is running in hard_irq, I think the driver should defer handling
>> this msg (and maybe all requests) to a workqueue?
>
> Does this help: <https://patchwork.kernel.org/patch/195562/>?
>
> --
> Best regards,                                        _     _
> | Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
> | Computer Science,  Michał "mina86" Nazarewicz       (o o)
> +----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
>

Re: rndis gadget: Inconsistent locking

[Michał Nazarewicz]

On Thu, 09 Dec 2010 18:00:55 +0100, Neil Jones <neiljay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

>> Does this help: <https://patchwork.kernel.org/patch/195562/>?
>
> Yes cheers, warning gone and driver seems fine so far.
>
> has this been accepted upstream ?

No, it hasn't.  There were some concerns that this patch is invalid and may
not work in some situations -- it uses an "incorrect" way of getting network
interface statistics which, however, happens to work with g_ether.  No one
got to implementing the worker solution, so as of my knowledge, this is the
only working patch.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rndis gadget: Inconsistent locking

[David Brownell]

On Wed, 2010-12-08 at 15:11 +0000, Neil Jones wrote:
> 
> Im getting another lockdep warning when using the RNDIS gadget:



Probably this is either


 (a) a recent regression, maybe caused by a change ISTR in the network
layer stats handling ... which broke another USB + NET driver in much
the same way, wish I remembered details of which driver, the fix
there was simple and maybe a good model for fixing this one.

or (b)  ... maybe a
DWC-specific USB device controller driver oddity.  (Seemed less likely
to me when I glanced at the stackdumps below.

Has this shown up with other USB device controller drivers, or just the
DWC (DesignWare)? UDC driver?

I'll say I'm not keen on adding a thread to the driver.  It's worked
fine without one for many years, even running lockdep.  Whatever change
(network stack or using DWC) caused the locking issue can be fixed
without a new thread.

- Dave



The first thing I noticed was that very little of the dumped stack
context was part of the RNDIS gadget ... often a sign that the issue
is in the call down to the code dumping its stack (or its context).

(Or if my recollection is right ... that there was an incompatible
change in a network statistics call, and whoever changed that didn't
update all affected callers.  (ergo breakage here and in another driver.
> 
>  WARNING: at kernel/softirq.c:98 ___local_bh_disable+0xc4/0xd0()
>  Modules linked in: g_ether
> 
>  Call trace:
>  [<40003bf8>] _show_stack+0x68/0x7c
>  [<40003c20>] _dump_stack+0x14/0x28
>  [<40013c3c>] _warn_slowpath_common+0x5c/0x7c
>  [<40013c74>] _warn_slowpath_null+0x18/0x2c
>  [<4001b17c>] ___local_bh_disable+0xc0/0xd0
>  [<4001b1a0>] _local_bh_disable+0x14/0x28
>  [<402e57f8>] __raw_spin_lock_bh+0x18/0x54
>  [<40257f4c>] _dev_txq_stats_fold+0x7c/0x13c
>  [<402580c4>] _dev_get_stats+0xb8/0xc0
>  [<781d4e60>] _rndis_msg_parser+0x288/0xa04 [g_ether]
>  [<781d5600>] _rndis_command_complete+0x24/0x70 [g_ether]
>  [<401d66fc>] _dwc_otg_request_done+0xd8/0x220
>  [<401d928c>] _ep0_complete_request+0x3f4/0x578
>  [<401d95bc>] _handle_ep0+0x1ac/0x146c
>  [<401daf7c>] _dwc_otg_pcd_handle_in_ep_intr+0x1c0/0x8bc
>  [<401db8dc>] _dwc_otg_pcd_handle_intr+0x264/0x294
>  [<401d6288>] _dwc_otg_pcd_irq+0x10/0x30
>  [<40054cf4>] _handle_IRQ_event+0x4c/0x184
>  [<40057b4c>] _handle_level_irq+0xac/0x15c
>  [<4000b204>] _metag_soc_irq_demux+0xac/0xb4
>  [<40002dd4>] _do_IRQ+0x4c/0x78
>  [<40004000>] _trigger_handler+0x38/0xac
>  [<40000b18>] ___TBIBoingVec+0xc/0x10
>  [<40003588>] _cpu_idle+0x54/0x78
> 
>  no locks held by swapper/0.
>  ---[ end trace 77ac3cfee0ae5b25 ]---
> 
> It looks like we are calling spin_lock_bh in the completion function
> which is running in hard_irq, I think the driver should defer handling
> this msg (and maybe all requests) to a workqueue?
> 
> Cheers
> 
> Neil
> 
> 
> 
> On Wed, Dec 8, 2010 at 3:03 PM, Neil Jones <neiljay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Hi,
> >
> > Im getting another lockdep warning when using the RNDIS gadget:
> >
> >  WARNING: at kernel/softirq.c:98 ___local_bh_disable+0xc4/0xd0()
> >  Modules linked in: g_ether
> >
> >  Call trace:
> >  [<40003bf8>] _show_stack+0x68/0x7c
> >  [<40003c20>] _dump_stack+0x14/0x28
> >  [<40013c3c>] _warn_slowpath_common+0x5c/0x7c
> >  [<40013c74>] _warn_slowpath_null+0x18/0x2c
> >  [<4001b17c>] ___local_bh_disable+0xc0/0xd0
> >  [<4001b1a0>] _local_bh_disable+0x14/0x28
> >  [<402e57f8>] __raw_spin_lock_bh+0x18/0x54
> >  [<40257f4c>] _dev_txq_stats_fold+0x7c/0x13c
> >  [<402580c4>] _dev_get_stats+0xb8/0xc0
> >  [<781d4e60>] _rndis_msg_parser+0x288/0xa04 [g_ether]
> >  [<781d5600>] _rndis_command_complete+0x24/0x70 [g_ether]
> >  [<401d66fc>] _dwc_otg_request_done+0xd8/0x220
> >  [<401d928c>] _ep0_complete_request+0x3f4/0x578
> >  [<401d95bc>] _handle_ep0+0x1ac/0x146c
> >  [<401daf7c>] _dwc_otg_pcd_handle_in_ep_intr+0x1c0/0x8bc
> >  [<401db8dc>] _dwc_otg_pcd_handle_intr+0x264/0x294
> >  [<401d6288>] _dwc_otg_pcd_irq+0x10/0x30
> >  [<40054cf4>] _handle_IRQ_event+0x4c/0x184
> >  [<40057b4c>] _handle_level_irq+0xac/0x15c
> >  [<4000b204>] _metag_soc_irq_demux+0xac/0xb4
> >  [<40002dd4>] _do_IRQ+0x4c/0x78
> >  [<40004000>] _trigger_handler+0x38/0xac
> >  [<40000b18>] ___TBIBoingVec+0xc/0x10
> >  [<40003588>] _cpu_idle+0x54/0x78
> >
> >  no locks held by swapper/0.
> >  ---[ end trace 77ac3cfee0ae5b25 ]---
> >
> > It
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

Re: rndis gadget: Inconsistent locking

[Neil Jones]

>Has this shown up with other USB device controller drivers, or just the
>DWC (DesignWare)? UDC driver?

Yes Michal Nazarewicz has seen this on a S3C UDC, heres his stack trace:

 WARNING: at kernel/softirq.c:143 local_bh_enable_ip+0x44/0xc0()
[...]
[<c0049024>] (local_bh_enable_ip+0x44/0xc0) from [<c025ef18>]
(dev_txq_stats_fold+0xac/0x108)
[<c025ef18>] (dev_txq_stats_fold+0xac/0x108) from [<c025f018>]
(dev_get_stats+0xa4/0xac)
[<c025f018>] (dev_get_stats+0xa4/0xac) from [<c01f72bc>]
(gen_ndis_query_resp+0x4c/0x43c)
[<c01f72bc>] (gen_ndis_query_resp+0x4c/0x43c) from [<c01f784c>]
(rndis_msg_parser+0x1a0/0x32c)
[<c01f784c>] (rndis_msg_parser+0x1a0/0x32c) from [<c01f79f8>]
(rndis_command_complete+0x20/0x4c)
[<c01f79f8>] (rndis_command_complete+0x20/0x4c) from [<c01f3278>]
(done+0x5c/0x70)
[<c01f3278>] (done+0x5c/0x70) from [<c01f3c60>] (complete_tx+0xf0/0x1a8)
[<c01f3c60>] (complete_tx+0xf0/0x1a8) from [<c01f3d8c>]
(process_ep_in_intr+0x74/0x14c)
[<c01f3d8c>] (process_ep_in_intr+0x74/0x14c) from [<c01f470c>]
(s3c_udc_irq+0x2c8/0x3f4)
[<c01f470c>] (s3c_udc_irq+0x2c8/0x3f4) from [<c006dfd0>]
(handle_IRQ_event+0x24/0xe4)
[<c006dfd0>] (handle_IRQ_event+0x24/0xe4) from [<c006fe40>]
(handle_level_irq+0xb0/0x12c)
[<c006fe40>] (handle_level_irq+0xb0/0x12c) from [<c0027074>]
(asm_do_IRQ+0x74/0x98)
[<c0027074>] (asm_do_IRQ+0x74/0x98) from [<c0027ca4>] (__irq_usr+0x44/0xc0)

(taken from Michal's original patch here:
<https://patchwork.kernel.org/patch/195562/>)

Neil




On Sun, Dec 26, 2010 at 11:49 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Wed, 2010-12-08 at 15:11 +0000, Neil Jones wrote:
>>
>> Im getting another lockdep warning when using the RNDIS gadget:
>
>
>
> Probably this is either
>
>
>  (a) a recent regression, maybe caused by a change ISTR in the network
> layer stats handling ... which broke another USB + NET driver in much
> the same way, wish I remembered details of which driver, the fix
> there was simple and maybe a good model for fixing this one.
>
> or (b)  ... maybe a
> DWC-specific USB device controller driver oddity.  (Seemed less likely
> to me when I glanced at the stackdumps below.
>
> Has this shown up with other USB device controller drivers, or just the
> DWC (DesignWare)? UDC driver?
>
> I'll say I'm not keen on adding a thread to the driver.  It's worked
> fine without one for many years, even running lockdep.  Whatever change
> (network stack or using DWC) caused the locking issue can be fixed
> without a new thread.
>
> - Dave
>
>
>
> The first thing I noticed was that very little of the dumped stack
> context was part of the RNDIS gadget ... often a sign that the issue
> is in the call down to the code dumping its stack (or its context).
>
> (Or if my recollection is right ... that there was an incompatible
> change in a network statistics call, and whoever changed that didn't
> update all affected callers.  (ergo breakage here and in another driver.
>>
>>  WARNING: at kernel/softirq.c:98 ___local_bh_disable+0xc4/0xd0()
>>  Modules linked in: g_ether
>>
>>  Call trace:
>>  [<40003bf8>] _show_stack+0x68/0x7c
>>  [<40003c20>] _dump_stack+0x14/0x28
>>  [<40013c3c>] _warn_slowpath_common+0x5c/0x7c
>>  [<40013c74>] _warn_slowpath_null+0x18/0x2c
>>  [<4001b17c>] ___local_bh_disable+0xc0/0xd0
>>  [<4001b1a0>] _local_bh_disable+0x14/0x28
>>  [<402e57f8>] __raw_spin_lock_bh+0x18/0x54
>>  [<40257f4c>] _dev_txq_stats_fold+0x7c/0x13c
>>  [<402580c4>] _dev_get_stats+0xb8/0xc0
>>  [<781d4e60>] _rndis_msg_parser+0x288/0xa04 [g_ether]
>>  [<781d5600>] _rndis_command_complete+0x24/0x70 [g_ether]
>>  [<401d66fc>] _dwc_otg_request_done+0xd8/0x220
>>  [<401d928c>] _ep0_complete_request+0x3f4/0x578
>>  [<401d95bc>] _handle_ep0+0x1ac/0x146c
>>  [<401daf7c>] _dwc_otg_pcd_handle_in_ep_intr+0x1c0/0x8bc
>>  [<401db8dc>] _dwc_otg_pcd_handle_intr+0x264/0x294
>>  [<401d6288>] _dwc_otg_pcd_irq+0x10/0x30
>>  [<40054cf4>] _handle_IRQ_event+0x4c/0x184
>>  [<40057b4c>] _handle_level_irq+0xac/0x15c
>>  [<4000b204>] _metag_soc_irq_demux+0xac/0xb4
>>  [<40002dd4>] _do_IRQ+0x4c/0x78
>>  [<40004000>] _trigger_handler+0x38/0xac
>>  [<40000b18>] ___TBIBoingVec+0xc/0x10
>>  [<40003588>] _cpu_idle+0x54/0x78
>>
>>  no locks held by swapper/0.
>>  ---[ end trace 77ac3cfee0ae5b25 ]---
>>
>> It looks like we are calling spin_lock_bh in the completion function
>> which is running in hard_irq, I think the driver should defer handling
>> this msg (and maybe all requests) to a workqueue?
>>
>> Cheers
>>
>> Neil
>>
>>
>>
>> On Wed, Dec 8, 2010 at 3:03 PM, Neil Jones <neiljay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > Hi,
>> >
>> > Im getting another lockdep warning when using the RNDIS gadget:
>> >
>> >  WARNING: at kernel/softirq.c:98 ___local_bh_disable+0xc4/0xd0()
>> >  Modules linked in: g_ether
>> >
>> >  Call trace:
>> >  [<40003bf8>] _show_stack+0x68/0x7c
>> >  [<40003c20>] _dump_stack+0x14/0x28
>> >  [<40013c3c>] _warn_slowpath_common+0x5c/0x7c
>> >  [<40013c74>] _warn_slowpath_null+0x18/0x2c
>> >  [<4001b17c>] ___local_bh_disable+0xc0/0xd0
>> >  [<4001b1a0>] _local_bh_disable+0x14/0x28
>> >  [<402e57f8>] __raw_spin_lock_bh+0x18/0x54
>> >  [<40257f4c>] _dev_txq_stats_fold+0x7c/0x13c
>> >  [<402580c4>] _dev_get_stats+0xb8/0xc0
>> >  [<781d4e60>] _rndis_msg_parser+0x288/0xa04 [g_ether]
>> >  [<781d5600>] _rndis_command_complete+0x24/0x70 [g_ether]
>> >  [<401d66fc>] _dwc_otg_request_done+0xd8/0x220
>> >  [<401d928c>] _ep0_complete_request+0x3f4/0x578
>> >  [<401d95bc>] _handle_ep0+0x1ac/0x146c
>> >  [<401daf7c>] _dwc_otg_pcd_handle_in_ep_intr+0x1c0/0x8bc
>> >  [<401db8dc>] _dwc_otg_pcd_handle_intr+0x264/0x294
>> >  [<401d6288>] _dwc_otg_pcd_irq+0x10/0x30
>> >  [<40054cf4>] _handle_IRQ_event+0x4c/0x184
>> >  [<40057b4c>] _handle_level_irq+0xac/0x15c
>> >  [<4000b204>] _metag_soc_irq_demux+0xac/0xb4
>> >  [<40002dd4>] _do_IRQ+0x4c/0x78
>> >  [<40004000>] _trigger_handler+0x38/0xac
>> >  [<40000b18>] ___TBIBoingVec+0xc/0x10
>> >  [<40003588>] _cpu_idle+0x54/0x78
>> >
>> >  no locks held by swapper/0.
>> >  ---[ end trace 77ac3cfee0ae5b25 ]---
>> >
>> > It
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rndis gadget: Inconsistent locking

[David Brownell]

> Yes Michal Nazarewicz has seen this on a S3C UDC, 

Good to confirm that.  As I mentioned, other folk have seen much the same bug on other hardware; we
know it's neither a new nor a HW-specific issue.

Correction to an earlier comment of mine: 

https://patchwork.kernel.org/patch/195562/
does correctly identify the commit which
caused this bug in multiple drivers.

However, that patch won't apply to the latest
tree from Linus.

Michal, can you update and resubmit, so I can
at least ack your fix and xpedite its merge?

It'd be good  to have RNDIS work again, a fair
number of folk rely on it, and this seems to be
the main obstacle to continuing to do that.


- Dave

Re: rndis gadget: Inconsistent locking

[Neil Jones]

> Michal, can you update and resubmit, so I can
> at least ack your fix and xpedite its merge?

I have just retested Michals patch but I have found another lockdep failure.

It looks like rndis_msg_parser() can call dev_get_stats too:

------------[ cut here ]------------
WARNING: at kernel/softirq.c:98 ___local_bh_disable+0xc4/0xd0()
Modules linked in: g_ether [last unloaded: g_zero]

Call trace:
[<40003d78>] _show_stack+0x68/0x7c
[<40003da0>] _dump_stack+0x14/0x28
[<40016d68>] _warn_slowpath_common+0x5c/0x7c
[<40016da0>] _warn_slowpath_null+0x18/0x2c
[<4001e330>] ___local_bh_disable+0xc0/0xd0
[<4001e354>] _local_bh_disable+0x14/0x28
[<402bd9e0>] __raw_spin_lock_bh+0x18/0x54
[<4023010c>] _dev_txq_stats_fold+0x7c/0x13c
[<40230284>] _dev_get_stats+0xb8/0xc0
[<78056eac>] _rndis_msg_parser+0x288/0xa04 [g_ether]
[<7805764c>] _rndis_command_complete+0x24/0x70 [g_ether]
[<401c39b8>] _dwc_otg_request_done+0xd4/0x210
[<401c6b48>] _ep0_complete_request+0x1dc/0x220
[<401c6cac>] _handle_ep0+0x120/0x850
[<401c79a4>] _dwc_otg_pcd_handle_in_ep_intr+0x188/0x780
[<401c8200>] _dwc_otg_pcd_handle_intr+0x264/0x294
[<401c36d4>] _dwc_otg_pcd_irq+0x10/0x30
[<4005a60c>] _handle_IRQ_event+0x4c/0x184
[<4005d464>] _handle_level_irq+0xac/0x15c
[<4000c154>] _metag_soc_irq_demux+0xac/0xb4
[<40002f54>] _do_IRQ+0x4c/0x78
[<40004180>] _trigger_handler+0x38/0xac
[<40000c98>] ___TBIBoingVec+0xc/0x10
[<40003708>] _cpu_idle+0x54/0x78

no locks held by swapper/0.
---[ end trace 74bf431fcc326847 ]---
=================================
[ INFO: inconsistent lock state ]
2.6.37-rc7+ #1833
---------------------------------
inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
swapper/0 [HC1[1]:SC0[2]:HE0:SE0] takes:
 (_xmit_ETHER){?.....}, at: [<40230110>] _dev_txq_stats_fold+0x80/0x13c
{HARDIRQ-ON-W} state was registered at:
  [<4004ddd0>] _lock_acquire+0x98/0xc0
  [<402bda00>] __raw_spin_lock_bh+0x38/0x54
  [<4023010c>] _dev_txq_stats_fold+0x7c/0x13c
  [<40230284>] _dev_get_stats+0xb8/0xc0
  [<4024461c>] _rtnl_fill_ifinfo+0x330/0x708
  [<40245b38>] _rtmsg_ifinfo+0x68/0xf4
  [<402342b8>] _register_netdevice+0x3f8/0x498
  [<40234398>] _register_netdev+0x40/0x60
  [<78056358>] _gether_setup+0x1d0/0x264 [g_ether]
  [<7806042c>] 0x7806042c
  [<78059c00>] _composite_bind+0x110/0x380 [g_ether]
  [<401c27f0>] _usb_gadget_probe_driver+0x9c/0xcc
  [<78057c90>] _usb_composite_probe+0x98/0xbc [g_ether]
  [<78060380>] 0x78060380
  [<4000210c>] _do_one_initcall+0x178/0x1d0
  [<40057ecc>] _sys_init_module+0xd8/0x208
  [<40004860>] _switch_handler+0x110/0x170
  [<40000c98>] ___TBIBoingVec+0xc/0x10
irq event stamp: 476219
hardirqs last  enabled at (476215): [<40003ec4>] _tail_end+0x110/0x1f8
hardirqs last disabled at (476216): [<40004178>] _trigger_handler+0x30/0xac
softirqs last  enabled at (476218): [<4001e790>] __local_bh_enable+0x14/0x24
softirqs last disabled at (476219): [<402bd9e4>] __raw_spin_lock_bh+0x1c/0x54

other info that might help us debug this:
no locks held by swapper/0.
--------------------------------------------------------------

So the patch may need some more work,

Neil.

On Fri, Jan 7, 2011 at 11:20 AM, David Brownell <david-b@pacbell.net> wrote:
>
>
>> Yes Michal Nazarewicz has seen this on a S3C UDC,
>
> Good to confirm that.  As I mentioned, other folk have seen much the same bug on other hardware; we
> know it's neither a new nor a HW-specific issue.
>
> Correction to an earlier comment of mine:
>
> https://patchwork.kernel.org/patch/195562/
> does correctly identify the commit which
> caused this bug in multiple drivers.
>
> However, that patch won't apply to the latest
> tree from Linus.
>
> Michal, can you update and resubmit, so I can
> at least ack your fix and xpedite its merge?
>
> It'd be good  to have RNDIS work again, a fair
> number of folk rely on it, and this seems to be
> the main obstacle to continuing to do that.
>
>
> - Dave
>
>
>
>
>
>

Re: rndis gadget: Inconsistent locking

[David Brownell]

> I have just retested Michals patch but I have found another
> lockdep failure.
> 
> It looks like rndis_msg_parser() can call dev_get_stats> too:

Can someone provide a fully working patch then?
Or maybe just revert the one that caused all the
breakage??

Rememeber that this driver was working fine for
years before netdev changes added multiple bugs
because (at least) they didn't update all callers.

Re: rndis gadget: Inconsistent locking

[Jarek Poplawski]

On 2011-01-10 13:14, David Brownell wrote:
> 
> 
>> I have just retested Michals patch but I have found another
>> lockdep failure.
>>
>> It looks like rndis_msg_parser() can call dev_get_stats> too:
> 
> Can someone provide a fully working patch then?
> Or maybe just revert the one that caused all the
> breakage??
> 
> Rememeber that this driver was working fine for
> years before netdev changes added multiple bugs
> because (at least) they didn't update all callers.

Maybe I miss something, but if it's like Michal described something
like this should be enough (not tested nor compiled).

Jarek P.
---

 drivers/usb/gadget/f_phonet.c |    6 ++++++
 drivers/usb/gadget/u_ether.c  |    6 ++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/f_phonet.c b/drivers/usb/gadget/f_phonet.c
index 3c6e1a0..bef4622 100644
--- a/drivers/usb/gadget/f_phonet.c
+++ b/drivers/usb/gadget/f_phonet.c
@@ -207,6 +207,11 @@ static int pn_net_close(struct net_device *dev)
 	return 0;
 }
 
+static struct net_device_stats *pn_net_stats(struct net_device *dev)
+{
+	return &dev->stats;
+}
+
 static void pn_tx_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	struct f_phonet *fp = ep->driver_data;
@@ -279,6 +284,7 @@ static int pn_net_mtu(struct net_device *dev, int new_mtu)
 static const struct net_device_ops pn_netdev_ops = {
 	.ndo_open	= pn_net_open,
 	.ndo_stop	= pn_net_close,
+	.ndo_get_stats	= pn_net_stats,
 	.ndo_start_xmit	= pn_net_xmit,
 	.ndo_change_mtu	= pn_net_mtu,
 };
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index 1eda968..f21520f 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -702,6 +702,11 @@ static int eth_stop(struct net_device *net)
 	return 0;
 }
 
+static struct net_device_stats *eth_get_stats(struct net_device *net)
+{
+	return &net->stats;
+}
+
 /*-------------------------------------------------------------------------*/
 
 /* initial value, changed by "ifconfig usb0 hw ether xx:xx:xx:xx:xx:xx" */
@@ -740,6 +745,7 @@ static struct eth_dev *the_dev;
 static const struct net_device_ops eth_netdev_ops = {
 	.ndo_open		= eth_open,
 	.ndo_stop		= eth_stop,
+	.ndo_get_stats		= eth_get_stats,
 	.ndo_start_xmit		= eth_start_xmit,
 	.ndo_change_mtu		= ueth_change_mtu,
 	.ndo_set_mac_address 	= eth_mac_addr,

Re: rndis gadget: Inconsistent locking

[Eric Dumazet]

Le mercredi 12 janvier 2011 à 12:28 +0000, Jarek Poplawski a écrit :
> On 2011-01-10 13:14, David Brownell wrote:
> > 
> > 
> >> I have just retested Michals patch but I have found another
> >> lockdep failure.
> >>
> >> It looks like rndis_msg_parser() can call dev_get_stats> too:
> > 
> > Can someone provide a fully working patch then?
> > Or maybe just revert the one that caused all the
> > breakage??
> > 
> > Rememeber that this driver was working fine for
> > years before netdev changes added multiple bugs
> > because (at least) they didn't update all callers.
> 
> Maybe I miss something, but if it's like Michal described something
> like this should be enough (not tested nor compiled).
> 
> Jarek P.
> ---
> 
>  drivers/usb/gadget/f_phonet.c |    6 ++++++
>  drivers/usb/gadget/u_ether.c  |    6 ++++++
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_phonet.c b/drivers/usb/gadget/f_phonet.c
> index 3c6e1a0..bef4622 100644
> --- a/drivers/usb/gadget/f_phonet.c
> +++ b/drivers/usb/gadget/f_phonet.c
> @@ -207,6 +207,11 @@ static int pn_net_close(struct net_device *dev)
>  	return 0;
>  }
>  
> +static struct net_device_stats *pn_net_stats(struct net_device *dev)
> +{
> +	return &dev->stats;
> +}
> +
>  static void pn_tx_complete(struct usb_ep *ep, struct usb_request *req)
>  {
>  	struct f_phonet *fp = ep->driver_data;
> @@ -279,6 +284,7 @@ static int pn_net_mtu(struct net_device *dev, int new_mtu)
>  static const struct net_device_ops pn_netdev_ops = {
>  	.ndo_open	= pn_net_open,
>  	.ndo_stop	= pn_net_close,
> +	.ndo_get_stats	= pn_net_stats,
>  	.ndo_start_xmit	= pn_net_xmit,
>  	.ndo_change_mtu	= pn_net_mtu,
>  };
> diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
> index 1eda968..f21520f 100644
> --- a/drivers/usb/gadget/u_ether.c
> +++ b/drivers/usb/gadget/u_ether.c
> @@ -702,6 +702,11 @@ static int eth_stop(struct net_device *net)
>  	return 0;
>  }
>  
> +static struct net_device_stats *eth_get_stats(struct net_device *net)
> +{
> +	return &net->stats;
> +}
> +
>  /*-------------------------------------------------------------------------*/
>  
>  /* initial value, changed by "ifconfig usb0 hw ether xx:xx:xx:xx:xx:xx" */
> @@ -740,6 +745,7 @@ static struct eth_dev *the_dev;
>  static const struct net_device_ops eth_netdev_ops = {
>  	.ndo_open		= eth_open,
>  	.ndo_stop		= eth_stop,
> +	.ndo_get_stats		= eth_get_stats,
>  	.ndo_start_xmit		= eth_start_xmit,
>  	.ndo_change_mtu		= ueth_change_mtu,
>  	.ndo_set_mac_address 	= eth_mac_addr,
> --

Hmm... 

So all net devices in gen_ndis_query_resp() should have a
ndo_get_stats() or ndo_get_stats64() method, not allowed to use
spin_lock_bh() / spin_unlock_bh()

If yes, we should add big fat comments to pn_net_stats()/eth_get_stats() so that nobody
tries to revert your patch ;)



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

Re: rndis gadget: Inconsistent locking

[Jarek Poplawski]

On Wed, Jan 12, 2011 at 02:07:12PM +0100, Eric Dumazet wrote:
...
> 
> Hmm... 
> 
> So all net devices in gen_ndis_query_resp() should have a
> ndo_get_stats() or ndo_get_stats64() method, not allowed to use
> spin_lock_bh() / spin_unlock_bh()
> 
> If yes, we should add big fat comments to pn_net_stats()/eth_get_stats() so that nobody
> tries to revert your patch ;)

I'm not sure I got your point: my patch could be replaced with
ndo_get_stats64() implementing irq safe locking or by changing
gen_ndis_query_resp() calling context. It's intended as a fast
(compatible) fix.

Jarek P.

Re: rndis gadget: Inconsistent locking

[Eric Dumazet]

Le mercredi 12 janvier 2011 à 13:23 +0000, Jarek Poplawski a écrit :
> On Wed, Jan 12, 2011 at 02:07:12PM +0100, Eric Dumazet wrote:
> ...
> > 
> > Hmm... 
> > 
> > So all net devices in gen_ndis_query_resp() should have a
> > ndo_get_stats() or ndo_get_stats64() method, not allowed to use
> > spin_lock_bh() / spin_unlock_bh()
> > 
> > If yes, we should add big fat comments to pn_net_stats()/eth_get_stats() so that nobody
> > tries to revert your patch ;)
> 
> I'm not sure I got your point: my patch could be replaced with
> ndo_get_stats64() implementing irq safe locking or by changing
> gen_ndis_query_resp() calling context. It's intended as a fast
> (compatible) fix.

I was mentioning that we tried in past months to remove useless
ndo_get_stats() methods that were only doing :

return &net->stats;

random commit : b27d50a9ff5cf2775b7a4daf5

Another possibility would be to use u64_stats_sync.h for these txq
counters.

(no locking needed to read counters, only a seqcount fetch/retry)

As a bonus, no overhead on 64bit arches.

Re: rndis gadget: Inconsistent locking

[Jarek Poplawski]

On Wed, Jan 12, 2011 at 02:32:20PM +0100, Eric Dumazet wrote:
> Le mercredi 12 janvier 2011 ?? 13:23 +0000, Jarek Poplawski a écrit :
> > On Wed, Jan 12, 2011 at 02:07:12PM +0100, Eric Dumazet wrote:
> > ...
> > > 
> > > Hmm... 
> > > 
> > > So all net devices in gen_ndis_query_resp() should have a
> > > ndo_get_stats() or ndo_get_stats64() method, not allowed to use
> > > spin_lock_bh() / spin_unlock_bh()
> > > 
> > > If yes, we should add big fat comments to pn_net_stats()/eth_get_stats() so that nobody
> > > tries to revert your patch ;)
> > 
> > I'm not sure I got your point: my patch could be replaced with
> > ndo_get_stats64() implementing irq safe locking or by changing
> > gen_ndis_query_resp() calling context. It's intended as a fast
> > (compatible) fix.
> 
> I was mentioning that we tried in past months to remove useless
> ndo_get_stats() methods that were only doing :
> 
> return &net->stats;
> 
> random commit : b27d50a9ff5cf2775b7a4daf5
> 
> Another possibility would be to use u64_stats_sync.h for these txq
> counters.
> 
> (no locking needed to read counters, only a seqcount fetch/retry)
> 
> As a bonus, no overhead on 64bit arches.

Well, I'm OK with anything, but this thread gets older and older ;-)

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

Re: rndis gadget: Inconsistent locking

[Eric Dumazet]

Le mercredi 12 janvier 2011 à 14:32 +0100, Eric Dumazet a écrit :
> Le mercredi 12 janvier 2011 à 13:23 +0000, Jarek Poplawski a écrit :
> > On Wed, Jan 12, 2011 at 02:07:12PM +0100, Eric Dumazet wrote:
> > ...
> > > 
> > > Hmm... 
> > > 
> > > So all net devices in gen_ndis_query_resp() should have a
> > > ndo_get_stats() or ndo_get_stats64() method, not allowed to use
> > > spin_lock_bh() / spin_unlock_bh()
> > > 
> > > If yes, we should add big fat comments to pn_net_stats()/eth_get_stats() so that nobody
> > > tries to revert your patch ;)
> > 
> > I'm not sure I got your point: my patch could be replaced with
> > ndo_get_stats64() implementing irq safe locking or by changing
> > gen_ndis_query_resp() calling context. It's intended as a fast
> > (compatible) fix.
> 
> I was mentioning that we tried in past months to remove useless
> ndo_get_stats() methods that were only doing :
> 
> return &net->stats;
> 
> random commit : b27d50a9ff5cf2775b7a4daf5
> 
> Another possibility would be to use u64_stats_sync.h for these txq
> counters.
> 
> (no locking needed to read counters, only a seqcount fetch/retry)
> 
> As a bonus, no overhead on 64bit arches.
> 

Or even better, remove these counters since there is no users left but
ixgbe.

(vlans, tunnels, ... now use percpu stats)



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

[PATCH] net: remove dev_txq_stats_fold()

[Eric Dumazet]

Le mercredi 12 janvier 2011 à 14:52 +0100, Eric Dumazet a écrit :

> Or even better, remove these counters since there is no users left but
> ixgbe.
> 
> (vlans, tunnels, ... now use percpu stats)
> 
> 

Thanks Jarek for the reminder :)

[PATCH] net: remove dev_txq_stats_fold()

After recent changes, (percpu stats on vlan/tunnels...), we dont need
anymore per struct netdev_queue tx_bytes/tx_packets/tx_dropped counters.

Only remaining users are ixgbe, sch_teql & macvlan :

1) ixgbe can be converted to use existing tx_ring counters.

2) macvlan incremented txq->tx_dropped, it can use the
dev->stats.tx_dropped counter.

3) sch_teql : almost revert ab35cd4b8f42 (Use net_device internal stats)
    Now we have ndo_get_stats64(), use it.

This removes a lockdep warning (and possible lockup) in rndis gadget,
calling dev_get_stats() from hard IRQ context.

Ref: http://www.spinics.net/lists/netdev/msg149202.html

Reported-by: Neil Jones <neiljay@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Jarek Poplawski <jarkao2@gmail.com>
CC: Alexander Duyck <alexander.h.duyck@intel.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ixgbe/ixgbe_main.c |   23 ++++++++++++++++-------
 drivers/net/macvtap.c          |    2 +-
 include/linux/netdevice.h      |    5 -----
 net/core/dev.c                 |   29 -----------------------------
 net/sched/sch_teql.c           |   26 +++++++++++++++++++++-----
 5 files changed, 38 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index a060610..602078b 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -6667,8 +6667,6 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 			  struct ixgbe_adapter *adapter,
 			  struct ixgbe_ring *tx_ring)
 {
-	struct net_device *netdev = tx_ring->netdev;
-	struct netdev_queue *txq;
 	unsigned int first;
 	unsigned int tx_flags = 0;
 	u8 hdr_len = 0;
@@ -6765,9 +6763,6 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 		/* add the ATR filter if ATR is on */
 		if (test_bit(__IXGBE_TX_FDIR_INIT_DONE, &tx_ring->state))
 			ixgbe_atr(tx_ring, skb, tx_flags, protocol);
-		txq = netdev_get_tx_queue(netdev, tx_ring->queue_index);
-		txq->tx_bytes += skb->len;
-		txq->tx_packets++;
 		ixgbe_tx_queue(tx_ring, tx_flags, count, skb->len, hdr_len);
 		ixgbe_maybe_stop_tx(tx_ring, DESC_NEEDED);
 
@@ -6925,8 +6920,6 @@ static struct rtnl_link_stats64 *ixgbe_get_stats64(struct net_device *netdev,
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	int i;
 
-	/* accurate rx/tx bytes/packets stats */
-	dev_txq_stats_fold(netdev, stats);
 	rcu_read_lock();
 	for (i = 0; i < adapter->num_rx_queues; i++) {
 		struct ixgbe_ring *ring = ACCESS_ONCE(adapter->rx_ring[i]);
@@ -6943,6 +6936,22 @@ static struct rtnl_link_stats64 *ixgbe_get_stats64(struct net_device *netdev,
 			stats->rx_bytes   += bytes;
 		}
 	}
+
+	for (i = 0; i < adapter->num_tx_queues; i++) {
+		struct ixgbe_ring *ring = ACCESS_ONCE(adapter->tx_ring[i]);
+		u64 bytes, packets;
+		unsigned int start;
+
+		if (ring) {
+			do {
+				start = u64_stats_fetch_begin_bh(&ring->syncp);
+				packets = ring->stats.packets;
+				bytes   = ring->stats.bytes;
+			} while (u64_stats_fetch_retry_bh(&ring->syncp, start));
+			stats->tx_packets += packets;
+			stats->tx_bytes   += bytes;
+		}
+	}
 	rcu_read_unlock();
 	/* following stats updated by ixgbe_watchdog_task() */
 	stats->multicast	= netdev->stats.multicast;
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 21845af..5933621 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -585,7 +585,7 @@ err:
 	rcu_read_lock_bh();
 	vlan = rcu_dereference(q->vlan);
 	if (vlan)
-		netdev_get_tx_queue(vlan->dev, 0)->tx_dropped++;
+		vlan->dev->stats.tx_dropped++;
 	rcu_read_unlock_bh();
 
 	return err;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index be4957c..d971346 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -520,9 +520,6 @@ struct netdev_queue {
 	 * please use this field instead of dev->trans_start
 	 */
 	unsigned long		trans_start;
-	u64			tx_bytes;
-	u64			tx_packets;
-	u64			tx_dropped;
 } ____cacheline_aligned_in_smp;
 
 static inline int netdev_queue_numa_node_read(const struct netdev_queue *q)
@@ -2265,8 +2262,6 @@ extern void		dev_load(struct net *net, const char *name);
 extern void		dev_mcast_init(void);
 extern struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 					       struct rtnl_link_stats64 *storage);
-extern void		dev_txq_stats_fold(const struct net_device *dev,
-					   struct rtnl_link_stats64 *stats);
 
 extern int		netdev_max_backlog;
 extern int		netdev_tstamp_prequeue;
diff --git a/net/core/dev.c b/net/core/dev.c
index a3ef808..83507c2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5523,34 +5523,6 @@ void netdev_run_todo(void)
 	}
 }
 
-/**
- *	dev_txq_stats_fold - fold tx_queues stats
- *	@dev: device to get statistics from
- *	@stats: struct rtnl_link_stats64 to hold results
- */
-void dev_txq_stats_fold(const struct net_device *dev,
-			struct rtnl_link_stats64 *stats)
-{
-	u64 tx_bytes = 0, tx_packets = 0, tx_dropped = 0;
-	unsigned int i;
-	struct netdev_queue *txq;
-
-	for (i = 0; i < dev->num_tx_queues; i++) {
-		txq = netdev_get_tx_queue(dev, i);
-		spin_lock_bh(&txq->_xmit_lock);
-		tx_bytes   += txq->tx_bytes;
-		tx_packets += txq->tx_packets;
-		tx_dropped += txq->tx_dropped;
-		spin_unlock_bh(&txq->_xmit_lock);
-	}
-	if (tx_bytes || tx_packets || tx_dropped) {
-		stats->tx_bytes   = tx_bytes;
-		stats->tx_packets = tx_packets;
-		stats->tx_dropped = tx_dropped;
-	}
-}
-EXPORT_SYMBOL(dev_txq_stats_fold);
-
 /* Convert net_device_stats to rtnl_link_stats64.  They have the same
  * fields in the same order, with only the type differing.
  */
@@ -5594,7 +5566,6 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 		netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev));
 	} else {
 		netdev_stats_to_stats64(storage, &dev->stats);
-		dev_txq_stats_fold(dev, storage);
 	}
 	storage->rx_dropped += atomic_long_read(&dev->rx_dropped);
 	return storage;
diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index af9360d..8b82c13 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -59,6 +59,10 @@ struct teql_master
 	struct net_device *dev;
 	struct Qdisc *slaves;
 	struct list_head master_list;
+	u64	tx_bytes;
+	u64	tx_packets;
+	u64	tx_errors;
+	u64	tx_dropped;
 };
 
 struct teql_sched_data
@@ -274,7 +278,6 @@ static inline int teql_resolve(struct sk_buff *skb,
 static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct teql_master *master = netdev_priv(dev);
-	struct netdev_queue *txq = netdev_get_tx_queue(dev, 0);
 	struct Qdisc *start, *q;
 	int busy;
 	int nores;
@@ -314,8 +317,8 @@ restart:
 					__netif_tx_unlock(slave_txq);
 					master->slaves = NEXT_SLAVE(q);
 					netif_wake_queue(dev);
-					txq->tx_packets++;
-					txq->tx_bytes += length;
+ 					master->tx_packets++;
+ 					master->tx_bytes += length;
 					return NETDEV_TX_OK;
 				}
 				__netif_tx_unlock(slave_txq);
@@ -342,10 +345,10 @@ restart:
 		netif_stop_queue(dev);
 		return NETDEV_TX_BUSY;
 	}
-	dev->stats.tx_errors++;
+	master->tx_errors++;
 
 drop:
-	txq->tx_dropped++;
+	master->tx_dropped++;
 	dev_kfree_skb(skb);
 	return NETDEV_TX_OK;
 }
@@ -398,6 +401,18 @@ static int teql_master_close(struct net_device *dev)
 	return 0;
 }
 
+static struct rtnl_link_stats64 *teql_master_stats64(struct net_device *dev,
+						     struct rtnl_link_stats64 *stats)
+{
+	struct teql_master *m = netdev_priv(dev);
+
+	stats->tx_packets	= m->tx_packets;
+	stats->tx_bytes		= m->tx_bytes;
+	stats->tx_errors	= m->tx_errors;
+	stats->tx_dropped	= m->tx_dropped;
+	return stats;
+}
+
 static int teql_master_mtu(struct net_device *dev, int new_mtu)
 {
 	struct teql_master *m = netdev_priv(dev);
@@ -422,6 +437,7 @@ static const struct net_device_ops teql_netdev_ops = {
 	.ndo_open	= teql_master_open,
 	.ndo_stop	= teql_master_close,
 	.ndo_start_xmit	= teql_master_xmit,
+	.ndo_get_stats64 = teql_master_stats64,
 	.ndo_change_mtu	= teql_master_mtu,
 };
 

Re: [PATCH] net: remove dev_txq_stats_fold()

[Jarek Poplawski]

On Wed, Jan 12, 2011 at 04:02:00PM +0100, Eric Dumazet wrote:
> Le mercredi 12 janvier 2011 ?? 14:52 +0100, Eric Dumazet a écrit :
> 
> > Or even better, remove these counters since there is no users left but
> > ixgbe.
> > 
> > (vlans, tunnels, ... now use percpu stats)
> > 
> > 
> 
> Thanks Jarek for the reminder :)
> 
> [PATCH] net: remove dev_txq_stats_fold()
> 
> After recent changes, (percpu stats on vlan/tunnels...), we dont need
> anymore per struct netdev_queue tx_bytes/tx_packets/tx_dropped counters.
> 
> Only remaining users are ixgbe, sch_teql & macvlan :

And gianfar (not counting staging/bcm) but I'm not sure if that's all.

> 
> 1) ixgbe can be converted to use existing tx_ring counters.
> 
> 2) macvlan incremented txq->tx_dropped, it can use the
> dev->stats.tx_dropped counter.
> 
> 3) sch_teql : almost revert ab35cd4b8f42 (Use net_device internal stats)
>     Now we have ndo_get_stats64(), use it.

Btw, why doesn't sch_teql need locking (for 32-bits)?

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

Re: [PATCH] net: remove dev_txq_stats_fold()

[Jarek Poplawski]

Btw, I updated Michal's email (according to linux-kernel).

Jarek P.

On Wed, Jan 12, 2011 at 10:05:18PM +0100, Jarek Poplawski wrote:
> On Wed, Jan 12, 2011 at 04:02:00PM +0100, Eric Dumazet wrote:
> > Le mercredi 12 janvier 2011 ?? 14:52 +0100, Eric Dumazet a écrit :
> > 
> > > Or even better, remove these counters since there is no users left but
> > > ixgbe.
> > > 
> > > (vlans, tunnels, ... now use percpu stats)
> > > 
> > > 
> > 
> > Thanks Jarek for the reminder :)
> > 
> > [PATCH] net: remove dev_txq_stats_fold()
> > 
> > After recent changes, (percpu stats on vlan/tunnels...), we dont need
> > anymore per struct netdev_queue tx_bytes/tx_packets/tx_dropped counters.
> > 
> > Only remaining users are ixgbe, sch_teql & macvlan :
> 
> And gianfar (not counting staging/bcm) but I'm not sure if that's all.
> 
> > 
> > 1) ixgbe can be converted to use existing tx_ring counters.
> > 
> > 2) macvlan incremented txq->tx_dropped, it can use the
> > dev->stats.tx_dropped counter.
> > 
> > 3) sch_teql : almost revert ab35cd4b8f42 (Use net_device internal stats)
> >     Now we have ndo_get_stats64(), use it.
> 
> Btw, why doesn't sch_teql need locking (for 32-bits)?
> 
> Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: remove dev_txq_stats_fold()

[Eric Dumazet]

Le mercredi 12 janvier 2011 à 22:18 +0100, Jarek Poplawski a écrit :
> Btw, I updated Michal's email (according to linux-kernel).
> 
> Jarek P.
> 
> On Wed, Jan 12, 2011 at 10:05:18PM +0100, Jarek Poplawski wrote:
> > On Wed, Jan 12, 2011 at 04:02:00PM +0100, Eric Dumazet wrote:
> > > Le mercredi 12 janvier 2011 ?? 14:52 +0100, Eric Dumazet a écrit :
> > > 
> > > > Or even better, remove these counters since there is no users left but
> > > > ixgbe.
> > > > 
> > > > (vlans, tunnels, ... now use percpu stats)
> > > > 
> > > > 
> > > 
> > > Thanks Jarek for the reminder :)
> > > 
> > > [PATCH] net: remove dev_txq_stats_fold()
> > > 
> > > After recent changes, (percpu stats on vlan/tunnels...), we dont need
> > > anymore per struct netdev_queue tx_bytes/tx_packets/tx_dropped counters.
> > > 
> > > Only remaining users are ixgbe, sch_teql & macvlan :
> > 
> > And gianfar (not counting staging/bcm) but I'm not sure if that's all.
> > 

Hmm, I did a "allyesconfig", but on x86_64 ;)

> > > 
> > > 1) ixgbe can be converted to use existing tx_ring counters.
> > > 
> > > 2) macvlan incremented txq->tx_dropped, it can use the
> > > dev->stats.tx_dropped counter.
> > > 
> > > 3) sch_teql : almost revert ab35cd4b8f42 (Use net_device internal stats)
> > >     Now we have ndo_get_stats64(), use it.
> > 
> > Btw, why doesn't sch_teql need locking (for 32-bits)?

Yes you're right, thanks !

[PATCH v2] net: remove dev_txq_stats_fold()

After recent changes, (percpu stats on vlan/tunnels...), we dont need
anymore per struct netdev_queue tx_bytes/tx_packets/tx_dropped counters.

Only remaining users are ixgbe, sch_teql, gianfar & macvlan :

1) ixgbe can be converted to use existing tx_ring counters.

2) macvlan incremented txq->tx_dropped, it can use the
dev->stats.tx_dropped counter.

3) sch_teql : almost revert ab35cd4b8f42 (Use net_device internal stats)
    Now we have ndo_get_stats64(), use it, even for "unsigned long"
fields (No need to bring back a struct net_device_stats)

4) gianfar adds a stats structure per tx queue to hold
tx_bytes/tx_packets

This removes a lockdep warning (and possible lockup) in rndis gadget,
calling dev_get_stats() from hard IRQ context.

Ref: http://www.spinics.net/lists/netdev/msg149202.html

Reported-by: Neil Jones <neiljay@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Jarek Poplawski <jarkao2@gmail.com>
CC: Alexander Duyck <alexander.h.duyck@intel.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
CC: Sandeep Gopalpet <sandeep.kumar@freescale.com>
CC: Michal Nazarewicz <mina86@mina86.com>
---
v2: added gianfar, removed u64 fields from sch_teql to avoid extra
locking

 drivers/net/gianfar.c          |   10 ++++------
 drivers/net/gianfar.h          |   10 ++++++++++
 drivers/net/ixgbe/ixgbe_main.c |   23 ++++++++++++++++-------
 drivers/net/macvtap.c          |    2 +-
 include/linux/netdevice.h      |    5 -----
 net/core/dev.c                 |   29 -----------------------------
 net/sched/sch_teql.c           |   26 +++++++++++++++++++++-----
 7 files changed, 52 insertions(+), 53 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 6de4675..119aa20 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -434,7 +434,6 @@ static void gfar_init_mac(struct net_device *ndev)
 static struct net_device_stats *gfar_get_stats(struct net_device *dev)
 {
 	struct gfar_private *priv = netdev_priv(dev);
-	struct netdev_queue *txq;
 	unsigned long rx_packets = 0, rx_bytes = 0, rx_dropped = 0;
 	unsigned long tx_packets = 0, tx_bytes = 0;
 	int i = 0;
@@ -450,9 +449,8 @@ static struct net_device_stats *gfar_get_stats(struct net_device *dev)
 	dev->stats.rx_dropped = rx_dropped;
 
 	for (i = 0; i < priv->num_tx_queues; i++) {
-		txq = netdev_get_tx_queue(dev, i);
-		tx_bytes += txq->tx_bytes;
-		tx_packets += txq->tx_packets;
+		tx_bytes += priv->tx_queue[i]->stats.tx_bytes;
+		tx_packets += priv->tx_queue[i]->stats.tx_packets;
 	}
 
 	dev->stats.tx_bytes = tx_bytes;
@@ -2109,8 +2107,8 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	/* Update transmit stats */
-	txq->tx_bytes += skb->len;
-	txq->tx_packets ++;
+	tx_queue->stats.tx_bytes += skb->len;
+	tx_queue->stats.tx_packets++;
 
 	txbdp = txbdp_start = tx_queue->cur_tx;
 	lstatus = txbdp->lstatus;
diff --git a/drivers/net/gianfar.h b/drivers/net/gianfar.h
index 68984eb..54de413 100644
--- a/drivers/net/gianfar.h
+++ b/drivers/net/gianfar.h
@@ -907,12 +907,21 @@ enum {
 	MQ_MG_MODE
 };
 
+/*
+ * Per TX queue stats
+ */
+struct tx_q_stats {
+	unsigned long tx_packets;
+	unsigned long tx_bytes;
+};
+
 /**
  *	struct gfar_priv_tx_q - per tx queue structure
  *	@txlock: per queue tx spin lock
  *	@tx_skbuff:skb pointers
  *	@skb_curtx: to be used skb pointer
  *	@skb_dirtytx:the last used skb pointer
+ *	@stats: bytes/packets stats
  *	@qindex: index of this queue
  *	@dev: back pointer to the dev structure
  *	@grp: back pointer to the group to which this queue belongs
@@ -934,6 +943,7 @@ struct gfar_priv_tx_q {
 	struct	txbd8 *tx_bd_base;
 	struct	txbd8 *cur_tx;
 	struct	txbd8 *dirty_tx;
+	struct tx_q_stats stats;
 	struct	net_device *dev;
 	struct gfar_priv_grp *grp;
 	u16	skb_curtx;
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index a060610..602078b 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -6667,8 +6667,6 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 			  struct ixgbe_adapter *adapter,
 			  struct ixgbe_ring *tx_ring)
 {
-	struct net_device *netdev = tx_ring->netdev;
-	struct netdev_queue *txq;
 	unsigned int first;
 	unsigned int tx_flags = 0;
 	u8 hdr_len = 0;
@@ -6765,9 +6763,6 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 		/* add the ATR filter if ATR is on */
 		if (test_bit(__IXGBE_TX_FDIR_INIT_DONE, &tx_ring->state))
 			ixgbe_atr(tx_ring, skb, tx_flags, protocol);
-		txq = netdev_get_tx_queue(netdev, tx_ring->queue_index);
-		txq->tx_bytes += skb->len;
-		txq->tx_packets++;
 		ixgbe_tx_queue(tx_ring, tx_flags, count, skb->len, hdr_len);
 		ixgbe_maybe_stop_tx(tx_ring, DESC_NEEDED);
 
@@ -6925,8 +6920,6 @@ static struct rtnl_link_stats64 *ixgbe_get_stats64(struct net_device *netdev,
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	int i;
 
-	/* accurate rx/tx bytes/packets stats */
-	dev_txq_stats_fold(netdev, stats);
 	rcu_read_lock();
 	for (i = 0; i < adapter->num_rx_queues; i++) {
 		struct ixgbe_ring *ring = ACCESS_ONCE(adapter->rx_ring[i]);
@@ -6943,6 +6936,22 @@ static struct rtnl_link_stats64 *ixgbe_get_stats64(struct net_device *netdev,
 			stats->rx_bytes   += bytes;
 		}
 	}
+
+	for (i = 0; i < adapter->num_tx_queues; i++) {
+		struct ixgbe_ring *ring = ACCESS_ONCE(adapter->tx_ring[i]);
+		u64 bytes, packets;
+		unsigned int start;
+
+		if (ring) {
+			do {
+				start = u64_stats_fetch_begin_bh(&ring->syncp);
+				packets = ring->stats.packets;
+				bytes   = ring->stats.bytes;
+			} while (u64_stats_fetch_retry_bh(&ring->syncp, start));
+			stats->tx_packets += packets;
+			stats->tx_bytes   += bytes;
+		}
+	}
 	rcu_read_unlock();
 	/* following stats updated by ixgbe_watchdog_task() */
 	stats->multicast	= netdev->stats.multicast;
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 21845af..5933621 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -585,7 +585,7 @@ err:
 	rcu_read_lock_bh();
 	vlan = rcu_dereference(q->vlan);
 	if (vlan)
-		netdev_get_tx_queue(vlan->dev, 0)->tx_dropped++;
+		vlan->dev->stats.tx_dropped++;
 	rcu_read_unlock_bh();
 
 	return err;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index be4957c..d971346 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -520,9 +520,6 @@ struct netdev_queue {
 	 * please use this field instead of dev->trans_start
 	 */
 	unsigned long		trans_start;
-	u64			tx_bytes;
-	u64			tx_packets;
-	u64			tx_dropped;
 } ____cacheline_aligned_in_smp;
 
 static inline int netdev_queue_numa_node_read(const struct netdev_queue *q)
@@ -2265,8 +2262,6 @@ extern void		dev_load(struct net *net, const char *name);
 extern void		dev_mcast_init(void);
 extern struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 					       struct rtnl_link_stats64 *storage);
-extern void		dev_txq_stats_fold(const struct net_device *dev,
-					   struct rtnl_link_stats64 *stats);
 
 extern int		netdev_max_backlog;
 extern int		netdev_tstamp_prequeue;
diff --git a/net/core/dev.c b/net/core/dev.c
index a3ef808..83507c2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5523,34 +5523,6 @@ void netdev_run_todo(void)
 	}
 }
 
-/**
- *	dev_txq_stats_fold - fold tx_queues stats
- *	@dev: device to get statistics from
- *	@stats: struct rtnl_link_stats64 to hold results
- */
-void dev_txq_stats_fold(const struct net_device *dev,
-			struct rtnl_link_stats64 *stats)
-{
-	u64 tx_bytes = 0, tx_packets = 0, tx_dropped = 0;
-	unsigned int i;
-	struct netdev_queue *txq;
-
-	for (i = 0; i < dev->num_tx_queues; i++) {
-		txq = netdev_get_tx_queue(dev, i);
-		spin_lock_bh(&txq->_xmit_lock);
-		tx_bytes   += txq->tx_bytes;
-		tx_packets += txq->tx_packets;
-		tx_dropped += txq->tx_dropped;
-		spin_unlock_bh(&txq->_xmit_lock);
-	}
-	if (tx_bytes || tx_packets || tx_dropped) {
-		stats->tx_bytes   = tx_bytes;
-		stats->tx_packets = tx_packets;
-		stats->tx_dropped = tx_dropped;
-	}
-}
-EXPORT_SYMBOL(dev_txq_stats_fold);
-
 /* Convert net_device_stats to rtnl_link_stats64.  They have the same
  * fields in the same order, with only the type differing.
  */
@@ -5594,7 +5566,6 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 		netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev));
 	} else {
 		netdev_stats_to_stats64(storage, &dev->stats);
-		dev_txq_stats_fold(dev, storage);
 	}
 	storage->rx_dropped += atomic_long_read(&dev->rx_dropped);
 	return storage;
diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index af9360d..8ab66a4 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -59,6 +59,10 @@ struct teql_master
 	struct net_device *dev;
 	struct Qdisc *slaves;
 	struct list_head master_list;
+	unsigned long	tx_bytes;
+	unsigned long	tx_packets;
+	unsigned long	tx_errors;
+	unsigned long	tx_dropped;
 };
 
 struct teql_sched_data
@@ -274,7 +278,6 @@ static inline int teql_resolve(struct sk_buff *skb,
 static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct teql_master *master = netdev_priv(dev);
-	struct netdev_queue *txq = netdev_get_tx_queue(dev, 0);
 	struct Qdisc *start, *q;
 	int busy;
 	int nores;
@@ -314,8 +317,8 @@ restart:
 					__netif_tx_unlock(slave_txq);
 					master->slaves = NEXT_SLAVE(q);
 					netif_wake_queue(dev);
-					txq->tx_packets++;
-					txq->tx_bytes += length;
+ 					master->tx_packets++;
+ 					master->tx_bytes += length;
 					return NETDEV_TX_OK;
 				}
 				__netif_tx_unlock(slave_txq);
@@ -342,10 +345,10 @@ restart:
 		netif_stop_queue(dev);
 		return NETDEV_TX_BUSY;
 	}
-	dev->stats.tx_errors++;
+	master->tx_errors++;
 
 drop:
-	txq->tx_dropped++;
+	master->tx_dropped++;
 	dev_kfree_skb(skb);
 	return NETDEV_TX_OK;
 }
@@ -398,6 +401,18 @@ static int teql_master_close(struct net_device *dev)
 	return 0;
 }
 
+static struct rtnl_link_stats64 *teql_master_stats64(struct net_device *dev,
+						     struct rtnl_link_stats64 *stats)
+{
+	struct teql_master *m = netdev_priv(dev);
+
+	stats->tx_packets	= m->tx_packets;
+	stats->tx_bytes		= m->tx_bytes;
+	stats->tx_errors	= m->tx_errors;
+	stats->tx_dropped	= m->tx_dropped;
+	return stats;
+}
+
 static int teql_master_mtu(struct net_device *dev, int new_mtu)
 {
 	struct teql_master *m = netdev_priv(dev);
@@ -422,6 +437,7 @@ static const struct net_device_ops teql_netdev_ops = {
 	.ndo_open	= teql_master_open,
 	.ndo_stop	= teql_master_close,
 	.ndo_start_xmit	= teql_master_xmit,
+	.ndo_get_stats64 = teql_master_stats64,
 	.ndo_change_mtu	= teql_master_mtu,
 };
 

Re: [PATCH] net: remove dev_txq_stats_fold()

[David Miller]

From: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Wed, 12 Jan 2011 23:13:14 +0100

> [PATCH v2] net: remove dev_txq_stats_fold()
> 
> After recent changes, (percpu stats on vlan/tunnels...), we dont need
> anymore per struct netdev_queue tx_bytes/tx_packets/tx_dropped counters.
> 
> Only remaining users are ixgbe, sch_teql, gianfar & macvlan :
> 
> 1) ixgbe can be converted to use existing tx_ring counters.
> 
> 2) macvlan incremented txq->tx_dropped, it can use the
> dev->stats.tx_dropped counter.
> 
> 3) sch_teql : almost revert ab35cd4b8f42 (Use net_device internal stats)
>     Now we have ndo_get_stats64(), use it, even for "unsigned long"
> fields (No need to bring back a struct net_device_stats)
> 
> 4) gianfar adds a stats structure per tx queue to hold
> tx_bytes/tx_packets
> 
> This removes a lockdep warning (and possible lockup) in rndis gadget,
> calling dev_get_stats() from hard IRQ context.
> 
> Ref: http://www.spinics.net/lists/netdev/msg149202.html
> 
> Reported-by: Neil Jones <neiljay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> CC: Jarek Poplawski <jarkao2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> CC: Alexander Duyck <alexander.h.duyck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> CC: Jeff Kirsher <jeffrey.t.kirsher-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> CC: Sandeep Gopalpet <sandeep.kumar-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> CC: Michal Nazarewicz <mina86-deATy8a+UHjQT0dZR+AlfA@public.gmane.org>

Applied, thanks everyone.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html