On Fri, Apr 8, 2016 at 6:56 PM, Eric Dumazet <eric.duma...@gmail.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 <da...@davemloft.net> wrote:
>> > From: Petri Gynther <pgynt...@google.com>
>> > Date: Tue, 5 Apr 2016 17:50:01 -0700
>> >
>> >> Add Byte Queue Limits (BQL) support to bcmgenet driver.
>> >>
>> >> Signed-off-by: Petri Gynther <pgynt...@google.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.
>
>
