[PATCH] bnxt_en: Fix RSS logic in __bnxt_reserve_rings()

[PATCH] bnxt_en: Fix RSS logic in __bnxt_reserve_rings()

[Michael Chan]

From: Pavan Chebbi <pavan.chebbi@broadcom.com>

In __bnxt_reserve_rings(), the existing code unconditionally sets the
default RSS indirection table to default if netif_is_rxfh_configured()
returns false.  This used to be correct before we added RSS contexts
support.  For example, if the user is changing the number of ethtool
channels, we will enter this path to reserve the new number of rings.
We will then set the RSS indirection table to default to cover the new
number of rings if netif_is_rxfh_configured() is false.

Now, with RSS contexts support, if the user has added or deleted RSS
contexts, we may now enter this path to reserve the new number of VNICs.
However, netif_is_rxfh_configured() will not return the correct state if
we are still in the middle of set_rxfh().  So the existing code may
set the indirection table of the default RSS context to default by
mistake.

Fix it to check if the reservation of the RX rings is changing.  Only
check netif_is_rxfh_configured() if it is changing.  RX rings will not
change in the middle of set_rxfh() and this will fix the issue.

Fixes: b3d0083caf9a ("bnxt_en: Support RSS contexts in ethtool .{get|set}_rxfh()")
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index bb3be33c1bbd..f788f114e430 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7648,8 +7648,8 @@ static int bnxt_get_avail_msix(struct bnxt *bp, int num);
 static int __bnxt_reserve_rings(struct bnxt *bp)
 {
 	struct bnxt_hw_rings hwr = {0};
+	int rx_rings, old_rx_rings, rc;
 	int cp = bp->cp_nr_rings;
-	int rx_rings, rc;
 	int ulp_msix = 0;
 	bool sh = false;
 	int tx_cp;
@@ -7683,6 +7683,7 @@ static int __bnxt_reserve_rings(struct bnxt *bp)
 	hwr.grp = bp->rx_nr_rings;
 	hwr.rss_ctx = bnxt_get_total_rss_ctxs(bp, &hwr);
 	hwr.stat = bnxt_get_func_stat_ctxs(bp);
+	old_rx_rings = bp->hw_resc.resv_rx_rings;
 
 	rc = bnxt_hwrm_reserve_rings(bp, &hwr);
 	if (rc)
@@ -7737,7 +7738,8 @@ static int __bnxt_reserve_rings(struct bnxt *bp)
 	if (!bnxt_rings_ok(bp, &hwr))
 		return -ENOMEM;
 
-	if (!netif_is_rxfh_configured(bp->dev))
+	if (old_rx_rings != bp->hw_resc.resv_rx_rings &&
+	    !netif_is_rxfh_configured(bp->dev))
 		bnxt_set_dflt_rss_indir_tbl(bp, NULL);
 
 	if (!bnxt_ulp_registered(bp->edev) && BNXT_NEW_RM(bp)) {
-- 
2.30.1

Re: [PATCH] bnxt_en: Fix RSS logic in __bnxt_reserve_rings()

[Jakub Kicinski]

On Wed, 24 Jul 2024 15:21:06 -0700 Michael Chan wrote:
> Now, with RSS contexts support, if the user has added or deleted RSS
> contexts, we may now enter this path to reserve the new number of VNICs.
> However, netif_is_rxfh_configured() will not return the correct state if
> we are still in the middle of set_rxfh().  So the existing code may
> set the indirection table of the default RSS context to default by
> mistake.

I feel like my explanation was more clear :S

The key point is that ethtool::set_rxfh() calls the "reload" functions
and expects the scope of the "reload" to be quite narrow, because only
the RSS table has changed. Unfortunately the add / delete of additional
contexts de-sync the resource counts, so ethtool::set_rxfh() now ends
up "reloading" more than it intended. The "more than intended" includes
going down the RSS indir reset path, which calls netif_is_rxfh_configured().
Return value from netif_is_rxfh_configured() during ethtool::set_rxfh()
is undefined.

Reported tag would have been nice too..

Re: [PATCH] bnxt_en: Fix RSS logic in __bnxt_reserve_rings()

[Jakub Kicinski]

On Wed, 24 Jul 2024 17:25:36 -0700 Jakub Kicinski wrote:
> On Wed, 24 Jul 2024 15:21:06 -0700 Michael Chan wrote:
> > Now, with RSS contexts support, if the user has added or deleted RSS
> > contexts, we may now enter this path to reserve the new number of VNICs.
> > However, netif_is_rxfh_configured() will not return the correct state if
> > we are still in the middle of set_rxfh().  So the existing code may
> > set the indirection table of the default RSS context to default by
> > mistake.  
> 
> I feel like my explanation was more clear :S
> 
> The key point is that ethtool::set_rxfh() calls the "reload" functions
> and expects the scope of the "reload" to be quite narrow, because only
> the RSS table has changed. Unfortunately the add / delete of additional
> contexts de-sync the resource counts, so ethtool::set_rxfh() now ends
> up "reloading" more than it intended. The "more than intended" includes
> going down the RSS indir reset path, which calls netif_is_rxfh_configured().
> Return value from netif_is_rxfh_configured() during ethtool::set_rxfh()
> is undefined.
> 
> Reported tag would have been nice too..

Reported-and-tested-by: Jakub Kicinski <kuba@kernel.org>
Link: https://lore.kernel.org/20240625010210.2002310-1-kuba@kernel.org

There's one more problem. It looks like changing queue count discards
existing ntuple filters:

# Check| At /root/./ksft/drivers/net/hw/rss_ctx.py, line 387, in test_rss_context_queue_reconfigure:
# Check|     test_rss_queue_reconfigure(cfg, main_ctx=False)
# Check| At /root/./ksft/drivers/net/hw/rss_ctx.py, line 230, in test_rss_queue_reconfigure:
# Check|     _send_traffic_check(cfg, port, ctx_ref, { 'target': (0, 3),
# Check| At /root/./ksft/drivers/net/hw/rss_ctx.py, line 92, in _send_traffic_check:
# Check|     ksft_lt(sum(cnts[i] for i in params['noise']), directed / 2,
# Check failed 1045235 >= 405823.5 traffic on other queues (context 1)':[460068, 351995, 565970, 351579, 127270]
# Exception while handling defer / cleanup (callback 1 of 3)!
# Defer Exception| Traceback (most recent call last):
# Defer Exception|   File "/root/ksft/net/lib/py/ksft.py", line 129, in ksft_flush_defer
# Defer Exception|     entry.exec_only()
# Defer Exception|   File "/root/ksft/net/lib/py/utils.py", line 93, in exec_only
# Defer Exception|     self.func(*self.args, **self.kwargs)
# Defer Exception|   File "/root/ksft/net/lib/py/utils.py", line 121, in ethtool
# Defer Exception|     return tool('ethtool', args, json=json, ns=ns, host=host)
# Defer Exception|   File "/root/ksft/net/lib/py/utils.py", line 108, in tool
# Defer Exception|     cmd_obj = cmd(cmd_str, ns=ns, host=host)
# Defer Exception|   File "/root/ksft/net/lib/py/utils.py", line 32, in __init__
# Defer Exception|     self.process(terminate=False, fail=fail, timeout=timeout)
# Defer Exception|   File "/root/ksft/net/lib/py/utils.py", line 50, in process
# Defer Exception|     raise CmdExitFailure("Command failed: %s\nSTDOUT: %s\nSTDERR: %s" %
# Defer Exception| net.lib.py.utils.CmdExitFailure: Command failed: ethtool -N eth0 delete 0
# Defer Exception| STDOUT: b''
# Defer Exception| STDERR: b'rmgr: Cannot delete RX class rule: No such file or directory\nCannot delete classification rule\n'
not ok 8 rss_ctx.test_rss_context_queue_reconfigure

This is from the following chunk of the test:

   225      # We should be able to increase queues, but table should be left untouched
   226      ethtool(f"-L {cfg.ifname} combined 5")
   227      data = get_rss(cfg, context=ctx_id)
   228      ksft_eq({0, 3}, set(data['rss-indirection-table']))
   229  
   230      _send_traffic_check(cfg, port, ctx_ref, { 'target': (0, 3),
   231                                                other_key: (1, 2, 4) })

The Check failure tells us the traffic was sprayed.
The Defer Exception, well, self-explanatory: 
  "Cannot delete RX class rule: No such file or directory"

Re: [PATCH] bnxt_en: Fix RSS logic in __bnxt_reserve_rings()

[Andy Gospodarek]

On Thu, Jul 25, 2024 at 11:19:12AM -0700, Jakub Kicinski wrote:
> On Wed, 24 Jul 2024 17:25:36 -0700 Jakub Kicinski wrote:
> > On Wed, 24 Jul 2024 15:21:06 -0700 Michael Chan wrote:
> > > Now, with RSS contexts support, if the user has added or deleted RSS
> > > contexts, we may now enter this path to reserve the new number of VNICs.
> > > However, netif_is_rxfh_configured() will not return the correct state if
> > > we are still in the middle of set_rxfh().  So the existing code may
> > > set the indirection table of the default RSS context to default by
> > > mistake.  
> > 
> > I feel like my explanation was more clear :S
> > 
> > The key point is that ethtool::set_rxfh() calls the "reload" functions
> > and expects the scope of the "reload" to be quite narrow, because only
> > the RSS table has changed. Unfortunately the add / delete of additional
> > contexts de-sync the resource counts, so ethtool::set_rxfh() now ends
> > up "reloading" more than it intended. The "more than intended" includes
> > going down the RSS indir reset path, which calls netif_is_rxfh_configured().
> > Return value from netif_is_rxfh_configured() during ethtool::set_rxfh()
> > is undefined.
> > 
> > Reported tag would have been nice too..
> 

Agreed.  Sorry that was missed.

> Reported-and-tested-by: Jakub Kicinski <kuba@kernel.org>
> Link: https://lore.kernel.org/20240625010210.2002310-1-kuba@kernel.org
> 
> There's one more problem. It looks like changing queue count discards
> existing ntuple filters:
> 
> # Check| At /root/./ksft/drivers/net/hw/rss_ctx.py, line 387, in test_rss_context_queue_reconfigure:
> # Check|     test_rss_queue_reconfigure(cfg, main_ctx=False)
> # Check| At /root/./ksft/drivers/net/hw/rss_ctx.py, line 230, in test_rss_queue_reconfigure:
> # Check|     _send_traffic_check(cfg, port, ctx_ref, { 'target': (0, 3),
> # Check| At /root/./ksft/drivers/net/hw/rss_ctx.py, line 92, in _send_traffic_check:
> # Check|     ksft_lt(sum(cnts[i] for i in params['noise']), directed / 2,
> # Check failed 1045235 >= 405823.5 traffic on other queues (context 1)':[460068, 351995, 565970, 351579, 127270]
> # Exception while handling defer / cleanup (callback 1 of 3)!
> # Defer Exception| Traceback (most recent call last):
> # Defer Exception|   File "/root/ksft/net/lib/py/ksft.py", line 129, in ksft_flush_defer
> # Defer Exception|     entry.exec_only()
> # Defer Exception|   File "/root/ksft/net/lib/py/utils.py", line 93, in exec_only
> # Defer Exception|     self.func(*self.args, **self.kwargs)
> # Defer Exception|   File "/root/ksft/net/lib/py/utils.py", line 121, in ethtool
> # Defer Exception|     return tool('ethtool', args, json=json, ns=ns, host=host)
> # Defer Exception|   File "/root/ksft/net/lib/py/utils.py", line 108, in tool
> # Defer Exception|     cmd_obj = cmd(cmd_str, ns=ns, host=host)
> # Defer Exception|   File "/root/ksft/net/lib/py/utils.py", line 32, in __init__
> # Defer Exception|     self.process(terminate=False, fail=fail, timeout=timeout)
> # Defer Exception|   File "/root/ksft/net/lib/py/utils.py", line 50, in process
> # Defer Exception|     raise CmdExitFailure("Command failed: %s\nSTDOUT: %s\nSTDERR: %s" %
> # Defer Exception| net.lib.py.utils.CmdExitFailure: Command failed: ethtool -N eth0 delete 0
> # Defer Exception| STDOUT: b''
> # Defer Exception| STDERR: b'rmgr: Cannot delete RX class rule: No such file or directory\nCannot delete classification rule\n'
> not ok 8 rss_ctx.test_rss_context_queue_reconfigure
> 
> This is from the following chunk of the test:
> 
>    225      # We should be able to increase queues, but table should be left untouched
>    226      ethtool(f"-L {cfg.ifname} combined 5")
>    227      data = get_rss(cfg, context=ctx_id)
>    228      ksft_eq({0, 3}, set(data['rss-indirection-table']))
>    229  
>    230      _send_traffic_check(cfg, port, ctx_ref, { 'target': (0, 3),
>    231                                                other_key: (1, 2, 4) })
> 
> The Check failure tells us the traffic was sprayed.
> The Defer Exception, well, self-explanatory: 
>   "Cannot delete RX class rule: No such file or directory"

We can take a look at that, but we currently do this on purpose.

Re: [PATCH] bnxt_en: Fix RSS logic in __bnxt_reserve_rings()

[Jakub Kicinski]

On Thu, 25 Jul 2024 17:33:10 -0400 Andy Gospodarek wrote:
> > The Check failure tells us the traffic was sprayed.
> > The Defer Exception, well, self-explanatory: 
> >   "Cannot delete RX class rule: No such file or directory"  
> 
> We can take a look at that, but we currently do this on purpose.

Hm, I thought the rules may get lost if someone ifconfig down's
the entire device. Losing rules on a config change is much more
of a no-no, especially as long as the queue API remains all but 
a mirage.

Re: [PATCH] bnxt_en: Fix RSS logic in __bnxt_reserve_rings()

[Pavan Chebbi]

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

On Fri, Jul 26, 2024 at 4:02 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 25 Jul 2024 17:33:10 -0400 Andy Gospodarek wrote:
> > > The Check failure tells us the traffic was sprayed.
> > > The Defer Exception, well, self-explanatory:
> > >   "Cannot delete RX class rule: No such file or directory"
> >
> > We can take a look at that, but we currently do this on purpose.
>
> Hm, I thought the rules may get lost if someone ifconfig down's
> the entire device. Losing rules on a config change is much more
> of a no-no, especially as long as the queue API remains all but
> a mirage.

The rules won't be lost on ifconfig down. They will be lost in
firmware but driver will restore them on ifup.
I will work on the fix to not lose them on any non-impacting config
change. Thanks.

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

Re: [PATCH] bnxt_en: Fix RSS logic in __bnxt_reserve_rings()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 24 Jul 2024 15:21:06 -0700 you wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> In __bnxt_reserve_rings(), the existing code unconditionally sets the
> default RSS indirection table to default if netif_is_rxfh_configured()
> returns false.  This used to be correct before we added RSS contexts
> support.  For example, if the user is changing the number of ethtool
> channels, we will enter this path to reserve the new number of rings.
> We will then set the RSS indirection table to default to cover the new
> number of rings if netif_is_rxfh_configured() is false.
> 
> [...]

Here is the summary with links:
  - bnxt_en: Fix RSS logic in __bnxt_reserve_rings()
    https://git.kernel.org/netdev/net/c/98ba1d931f61

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html