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

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

[Petri Gynther]

Add Byte Queue Limits (BQL) support to bcmgenet driver.

Signed-off-by: Petri Gynther <pgynther@google.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index f7b42b9..9fcd514 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1221,8 +1221,10 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
 	dev->stats.tx_packets += pkts_compl;
 	dev->stats.tx_bytes += bytes_compl;
 
+	txq = netdev_get_tx_queue(dev, ring->queue);
+	netdev_tx_completed_queue(txq, pkts_compl, bytes_compl);
+
 	if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
-		txq = netdev_get_tx_queue(dev, ring->queue);
 		if (netif_tx_queue_stopped(txq))
 			netif_tx_wake_queue(txq);
 	}
@@ -1516,6 +1518,8 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	ring->prod_index += nr_frags + 1;
 	ring->prod_index &= DMA_P_INDEX_MASK;
 
+	netdev_tx_sent_queue(txq, GENET_CB(skb)->bytes_sent);
+
 	if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
 		netif_tx_stop_queue(txq);
 
@@ -2809,6 +2813,8 @@ static void bcmgenet_hfb_init(struct bcmgenet_priv *priv)
 static void bcmgenet_netif_start(struct net_device *dev)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
+	int i;
+	struct netdev_queue *txq;
 
 	/* Start the network engine */
 	bcmgenet_enable_rx_napi(priv);
@@ -2816,6 +2822,14 @@ static void bcmgenet_netif_start(struct net_device *dev)
 
 	umac_enable_set(priv, CMD_TX_EN | CMD_RX_EN, true);
 
+	for (i = 0; i < priv->hw_params->tx_queues; i++) {
+		txq = netdev_get_tx_queue(dev, priv->tx_rings[i].queue);
+		netdev_tx_reset_queue(txq);
+	}
+
+	txq = netdev_get_tx_queue(dev, priv->tx_rings[DESC_INDEX].queue);
+	netdev_tx_reset_queue(txq);
+
 	netif_tx_start_all_queues(dev);
 
 	/* Monitor link interrupts now */
-- 
2.8.0.rc3.226.g39d4020

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

[Florian Fainelli]

2016-04-05 17:50 GMT-07:00 Petri Gynther <pgynther@google.com>:
> Add Byte Queue Limits (BQL) support to bcmgenet driver.
>
> Signed-off-by: Petri Gynther <pgynther@google.com>

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!
-- 
Florian

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

[Petri Gynther]

On Wed, Apr 6, 2016 at 1:25 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> 2016-04-05 17:50 GMT-07:00 Petri Gynther <pgynther@google.com>:
> > Add Byte Queue Limits (BQL) support to bcmgenet driver.
> >
> > Signed-off-by: Petri Gynther <pgynther@google.com>
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
> Thanks!
> --
> Florian

Any further comments?

Notable difference from some other drivers --
netdev_tx_reset_queue(txq) is called for all queues in
bcmgenet_netif_start(), just before netif_tx_start_all_queues(dev).
This is to ensure that BQL is reset before the interface becomes
operational.

I think that is the right place for these calls.

Some other drivers call it from the "interface down" path.

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

[Eric Dumazet]

On Fri, 2016-04-08 at 09:54 -0700, Petri Gynther wrote:
> On Wed, Apr 6, 2016 at 1:25 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> > 2016-04-05 17:50 GMT-07:00 Petri Gynther <pgynther@google.com>:
> > > Add Byte Queue Limits (BQL) support to bcmgenet driver.
> > >
> > > Signed-off-by: Petri Gynther <pgynther@google.com>
> >
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >
> > Thanks!
> > --
> > Florian
> 
> Any further comments?
> 
> Notable difference from some other drivers --
> netdev_tx_reset_queue(txq) is called for all queues in
> bcmgenet_netif_start(), just before netif_tx_start_all_queues(dev).
> This is to ensure that BQL is reset before the interface becomes
> operational.
> 
> I think that is the right place for these calls.
> 
> Some other drivers call it from the "interface down" path.

BQL is ready to go at device setup :

__QUEUE_STATE_STACK_XOFF is not set

dql_reset() was called from dql_init(), called from
netdev_init_one_queue()

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

[David Miller]

From: Petri Gynther <pgynther@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 <pgynther@google.com>

As Eric Dumazet indicated, your ->ndo_init() code to reset the queues is
probably not necessary at all.

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

[Petri Gynther]

On Fri, Apr 8, 2016 at 1:36 PM, David Miller <davem@davemloft.net> wrote:
> From: Petri Gynther <pgynther@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 <pgynther@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.

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

[Eric Dumazet]

On Fri, 2016-04-08 at 18:39 -0700, Petri Gynther wrote:
> On Fri, Apr 8, 2016 at 1:36 PM, David Miller <davem@davemloft.net> wrote:
> > From: Petri Gynther <pgynther@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 <pgynther@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.

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.

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

[Alexander Duyck]

On Fri, Apr 8, 2016 at 6:56 PM, Eric Dumazet <eric.dumazet@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 <davem@davemloft.net> wrote:
>> > From: Petri Gynther <pgynther@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 <pgynther@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.
>
> 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

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

[Petri Gynther]

On Fri, Apr 8, 2016 at 6:56 PM, Eric Dumazet <eric.dumazet@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 <davem@davemloft.net> wrote:
>> > From: Petri Gynther <pgynther@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 <pgynther@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.
>
>

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

[Eric Dumazet]

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.