netdev - Re: [PATCH net-next] net: bcmgenet: add BQL support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1460166979.6473.451.camel@edumazet-glaptop3.roam.corp.google.com>
Date:	Fri, 08 Apr 2016 18:56:19 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Petri Gynther <pgynther@...gle.com>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Florian Fainelli <f.fainelli@...il.com>, opendmb@...il.com,
	Jaedon Shin <jaedon.shin@...il.com>
Subject: Re: [PATCH net-next] net: bcmgenet: add BQL support

On Fri, 2016-04-08 at 18:39 -0700, Petri Gynther wrote:
> On Fri, Apr 8, 2016 at 1:36 PM, David Miller <davem@...emloft.net> wrote:
> > From: Petri Gynther <pgynther@...gle.com>
> > Date: Tue,  5 Apr 2016 17:50:01 -0700
> >
> >> Add Byte Queue Limits (BQL) support to bcmgenet driver.
> >>
> >> Signed-off-by: Petri Gynther <pgynther@...gle.com>
> >
> > As Eric Dumazet indicated, your ->ndo_init() code to reset the queues is
> > probably not necessary at all.
> 
> I added the netdev_tx_reset_queue(txq) calls to ndo_open() path:
> netdev->ndo_open()
>   bcmgenet_open()
>     bcmgenet_netif_start()
>       for all Tx queues:
>         netdev_tx_reset_queue(txq)
>           clear __QUEUE_STATE_STACK_XOFF
>           dql_reset()
>       netif_tx_start_all_queues(dev)
>         for all Tx queues:
>           clear __QUEUE_STATE_DRV_XOFF
> 
> So, I think the call to netdev_tx_reset_queue(txq) is in the right
> place. It ensures that the Tx queue state is clean when the device is
> opened.


The netdev_tx_reset_queue(txq) calls are only needed in exceptional
conditions.

Not at device start, as the core networking layer init all txq
(including their BQL state) properly before giving them to drivers for
use.

For example, tg3 calls netdev_tx_reset_queue() only when freeing tx
rings, as it might have freed skb(s) not from normal TX complete path
and thus missed appropriate dql_completed().

If you believe BQL drivers need a fix, please elaborate ?

Thanks.