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]

Date:	Fri, 8 Apr 2016 19:26:18 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Petri Gynther <pgynther@...gle.com>,
	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, Apr 8, 2016 at 6:56 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> 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.

For a bit of history on why you might want to do the reset on clean-up
instead of init you might take a look at commit dad8a3b3eaa0 ("igb,
ixgbe: netdev_tx_reset_queue incorrectly called from tx init path").

Basically you want to make certain you flush the queues after bringing
the interface down so that you don't possibly trigger any false hangs
for having stalled queues.  Basically the rule with the BQL stuff is
you need to leave the Tx queue in the state you found it in instead of
just wiping it and making use of it and putting it away dirty.

- Alex