[PATCH] net/net_failover: fix queue exceeding warning

[PATCH] net/net_failover: fix queue exceeding warning

[Faicker Mo]

If the primary device queue number is bigger than the default 16,
there is a warning about the queue exceeding when tx from the
net_failover device.

Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn>
---
 drivers/net/net_failover.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/net_failover.c b/drivers/net/net_failover.c
index 7a28e082436e..d0c916a53d7c 100644
--- a/drivers/net/net_failover.c
+++ b/drivers/net/net_failover.c
@@ -130,14 +130,10 @@ static u16 net_failover_select_queue(struct net_device *dev,
 			txq = ops->ndo_select_queue(primary_dev, skb, sb_dev);
 		else
 			txq = netdev_pick_tx(primary_dev, skb, NULL);
-
-		qdisc_skb_cb(skb)->slave_dev_queue_mapping = skb->queue_mapping;
-
-		return txq;
+	} else {
+		txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
 	}
 
-	txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
-
 	/* Save the original txq to restore before passing to the driver */
 	qdisc_skb_cb(skb)->slave_dev_queue_mapping = skb->queue_mapping;
 
-- 
2.39.1

Re: [PATCH] net/net_failover: fix queue exceeding warning

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

On Tue, Mar 21, 2023 at 8:15 AM Faicker Mo <faicker.mo@ucloud.cn> wrote:
>
> If the primary device queue number is bigger than the default 16,
> there is a warning about the queue exceeding when tx from the
> net_failover device.
>

Can you describe the issue more? If the net device has not implemented
its own selection then netdev_pick_tx should take care of the
real_num_tx_queues.
Is that not happening?

> Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn>
> ---
>  drivers/net/net_failover.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/net_failover.c b/drivers/net/net_failover.c
> index 7a28e082436e..d0c916a53d7c 100644
> --- a/drivers/net/net_failover.c
> +++ b/drivers/net/net_failover.c
> @@ -130,14 +130,10 @@ static u16 net_failover_select_queue(struct net_device *dev,
>                         txq = ops->ndo_select_queue(primary_dev, skb, sb_dev);
>                 else
>                         txq = netdev_pick_tx(primary_dev, skb, NULL);
> -
> -               qdisc_skb_cb(skb)->slave_dev_queue_mapping = skb->queue_mapping;
> -
> -               return txq;
> +       } else {
> +               txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
>         }
>
> -       txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
> -
>         /* Save the original txq to restore before passing to the driver */
>         qdisc_skb_cb(skb)->slave_dev_queue_mapping = skb->queue_mapping;
>
> --
> 2.39.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: Re: [PATCH] net/net_failover: fix queue exceeding warning

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 2802 bytes --]

On Tue, Mar 21, 2023 at 11:17 AM Faicker Mo <faicker.mo@ucloud.cn> wrote:
>
> When tx from the net_failover device, the actual tx queue number is the salve device.

Then why is primary OK..

> The ndo_select_queue of net_failover device returns the txq which is the primary device txq
> if the primary device is OK.

This is what is done in all the functions. I don't think there is a problem.
Not sure if there is an issue I am not getting, at least with the description.
I will let the maintainer take the call. Thanks.

> This number may be bigger than the default 16 of the net_failover device.
>  A warning will be reported in netdev_cap_txqueue which device is the net_failover.
>
>
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
>  Date: 2023-03-21 13:11:52
> To:Faicker Mo <faicker.mo@ucloud.cn>
>  cc: Sridhar Samudrala <sridhar.samudrala@intel.com>,"David S. Miller" <davem@davemloft.net>,Eric Dumazet <edumazet@google.com>,Jakub Kicinski <kuba@kernel.org>,Paolo Abeni <pabeni@redhat.com>,netdev@vger.kernel.org,linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] net/net_failover: fix queue exceeding warning>On Tue, Mar 21, 2023 at 8:15 AM Faicker Mo <faicker.mo@ucloud.cn> wrote:
> >>
> >> If the primary device queue number is bigger than the default 16,
> >> there is a warning about the queue exceeding when tx from the
> >> net_failover device.
> >>
> >
> >Can you describe the issue more? If the net device has not implemented
> >its own selection then netdev_pick_tx should take care of the
> >real_num_tx_queues.
> >Is that not happening?
> >
> >> Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn>
> >> ---
> >>  drivers/net/net_failover.c | 8 ++------
> >>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/net/net_failover.c b/drivers/net/net_failover.c
> >> index 7a28e082436e..d0c916a53d7c 100644
> >> --- a/drivers/net/net_failover.c
> >> +++ b/drivers/net/net_failover.c
> >> @@ -130,14 +130,10 @@ static u16 net_failover_select_queue(struct net_device *dev,
> >>                         txq = ops->ndo_select_queue(primary_dev, skb, sb_dev);
> >>                 else
> >>                         txq = netdev_pick_tx(primary_dev, skb, NULL);
> >> -
> >> -               qdisc_skb_cb(skb)->slave_dev_queue_mapping = skb->queue_mapping;
> >> -
> >> -               return txq;
> >> +       } else {
> >> +               txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
> >>         }
> >>
> >> -       txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
> >> -
> >>         /* Save the original txq to restore before passing to the driver */
> >>         qdisc_skb_cb(skb)->slave_dev_queue_mapping = skb->queue_mapping;
> >>
> >> --
> >> 2.39.1
> >>
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH] net/net_failover: fix queue exceeding warning

[Paolo Abeni]

On Tue, 2023-03-21 at 10:29 +0800, Faicker Mo wrote:
> If the primary device queue number is bigger than the default 16,
> there is a warning about the queue exceeding when tx from the
> net_failover device.
> 
> Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn>

This looks like a fixes, so it should include at least a fixes tag.

More importantly a longer/clearer description of the issue is needed,
including the warning backtrace.

I think this warning:

https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L3542

should not be ignored/silenced: it's telling that the running
configuration is not using a number of the available tx queues, which
is possibly not the thing you want.

Instead the failover device could use an higher number of tx queues and
eventually set real_num_tx_queues equal to the primary_dev when the
latter is enslaved.

Thanks,

Paolo

Re:Re: [PATCH] net/net_failover: fix queue exceeding warning

[Faicker Mo]

Thanks. I will send the v2 fix later.

Yes, the better method is to let the failover device folllows the primary dev
and remove the warning, but more work need to be done.


From: Paolo Abeni <pabeni@redhat.com>
Date: 2023-03-22 19:40:44
To:  Faicker Mo <faicker.mo@ucloud.cn>
Cc:  Sridhar Samudrala <sridhar.samudrala@intel.com>,"David S. Miller" <davem@davemloft.net>,Eric Dumazet <edumazet@google.com>,Jakub Kicinski <kuba@kernel.org>,netdev@vger.kernel.org,linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net/net_failover: fix queue exceeding warning>On Tue, 2023-03-21 at 10:29 +0800, Faicker Mo wrote:
>> If the primary device queue number is bigger than the default 16,
>> there is a warning about the queue exceeding when tx from the
>> net_failover device.
>> 
>> Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn>
>
>This looks like a fixes, so it should include at least a fixes tag.
>
>More importantly a longer/clearer description of the issue is needed,
>including the warning backtrace.
>
>I think this warning:
>
>https://elixir.bootlin.com/linux/latest/source/include/linux/netdevice.h#L3542
>
>should not be ignored/silenced: it's telling that the running
>configuration is not using a number of the available tx queues, which
>is possibly not the thing you want.
>
>Instead the failover device could use an higher number of tx queues and
>eventually set real_num_tx_queues equal to the primary_dev when the
>latter is enslaved.
>
>Thanks,
>
>Paolo
>
>
>