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 21:13:39 -0700
From:	Petri Gynther <pgynther@...gle.com>
To:	Eric Dumazet <eric.dumazet@...il.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, 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.
>

What values does the networking core program into BQL dynamic limits
that my code in netdev->ndo_open() would wipe out?

You mentioned the queue init path:
netdev_init_one_queue() -> dql_init() -> dql_reset()

that is called when the netdev is created and Tx queues allocated.

But, does the networking core somewhere set *different* values for BQL
dynamic limits than what dql_reset() did, before opening the device?

> 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().
>

Looking at the tg3 driver, it calls:
tg3_stop()
  tg3_free_rings()
    netdev_tx_reset_queue()

netdev_tx_reset_queue() is called unconditionally, as long as the Tx
ring exists. So "ip link set dev eth<x> down" would cause it to be
called.

Why is it OK to call netdev_tx_reset_queue() from the
netdev->ndo_stop() path, but not from netdev->ndo_open() path?

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