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] [day] [month] [year] [list]

Message-ID: <1460177608.6473.463.camel@edumazet-glaptop3.roam.corp.google.com>
Date:	Fri, 08 Apr 2016 21:53:28 -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 21:13 -0700, Petri Gynther wrote:

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

0 and 0

Clearing again these values by 0 and 0 is defensive programming.

As I said, no BQL enabled driver does that, and we do not want various
drivers implementing BQL in various ways.

Having the same logic is easier for code review and maintenance.

This was proven to work for many years.

> 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?

Because we properly init BQL state when a device is created in core
networking stack. So that we do not have to copy the same code over and
over in 100 drivers. This is called code factorization.


Put these calls in bcmgenet_fini_dma(), to follow the BQL model used in
all other drivers.

Thanks.