| Message ID | 20240326144646.2078893-7-saeed@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,01/10] net/mlx5: E-switch, store eswitch pointer before registering devlink_param | expand |
On Tue, 26 Mar 2024 07:46:42 -0700 Saeed Mahameed wrote: > Changing the channels number after configuring the receive > flow hash indirection table may alter the RSS table size. > The previous configuration may no longer be compatible with > the new receive flow hash indirection table. > > Block changing the channels number when RXFH is configured. Do I understand correctly that this will block all set_channels calls after indir table changes? This may be a little too risky for a fix. Perhaps okay for net-next, but not a fix. I'd think that setting indir table and then increasing the number of channels is a pretty legit maneuver, or even best practice to allocate a queue outside of RSS. Is it possible to make a narrower change, only rejecting the truly problematic transitions?
On 29/03/2024 8:31, Jakub Kicinski wrote: > On Tue, 26 Mar 2024 07:46:42 -0700 Saeed Mahameed wrote: >> Changing the channels number after configuring the receive >> flow hash indirection table may alter the RSS table size. >> The previous configuration may no longer be compatible with >> the new receive flow hash indirection table. >> >> Block changing the channels number when RXFH is configured. > > Do I understand correctly that this will block all set_channels > calls after indir table changes? Right. > This may be a little too risky > for a fix. Perhaps okay for net-next, but not a fix. > This fixes an issue introduced only in v6.7, not before that. > I'd think that setting indir table and then increasing the number > of channels is a pretty legit maneuver, or even best practice > to allocate a queue outside of RSS. > > Is it possible to make a narrower change, only rejecting the truly > problematic transitions? > The rationale of having a "single flow" or "single "logic" is to make it simple, and achieve a fine user experience. Otherwise, users would, for example, question why increasing the number of channels (after setting the indir table) from 24 channels to 120 works, but doesn't work when trying with 130 channels, although max num channels is much higher. The required order looks pretty natural: first set the desired num of channels, and only then set your indirection table. At the end, there are pros and cons for each solution. If you still strongly prefer narrowing it down only to the truly problematic transitions, then we'll have no big issue in changing this.
On Mon, 1 Apr 2024 09:54:26 +0300 Tariq Toukan wrote: > The rationale of having a "single flow" or "single "logic" is to make it > simple, and achieve a fine user experience. > > Otherwise, users would, for example, question why increasing the number > of channels (after setting the indir table) from 24 channels to 120 > works, but doesn't work when trying with 130 channels, although max num > channels is much higher. Any way to preserve the indir table in case it has to grow? If it increases by pow2 maybe we can "duplicate" the old table? 90% of the time when user changes the settings it's to exclude a queue from RSS, the remaining 10% to change the balance. In both cases "mirroring" the settings would be fine. > The required order looks pretty natural: first set the desired num of > channels, and only then set your indirection table. Say user allocated 16 queues for RSS and 4 for flow rules and/or other RSS context. Now they want to bump the 4 to 8. Resetting RSS and to be able to allocate new queues may not be an option, as traffic from the two "domains" would start mixing. Admittedly a bit contrived but not impossible, so my vote would be to only nak the cases we really can't reliably support :( > At the end, there are pros and cons for each solution. > If you still strongly prefer narrowing it down only to the truly > problematic transitions, then we'll have no big issue in changing this.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c index cc51ce16df14..c5a203b5119d 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c @@ -451,6 +451,17 @@ int mlx5e_ethtool_set_channels(struct mlx5e_priv *priv, mutex_lock(&priv->state_lock); + /* Don't allow changing the number of channels if RXFH was previously configured. + * Changing the channels number after configuring the RXFH may alter the RSS table size, + * the previous configuration may no longer be compatible with the new RSS table. + */ + if (netif_is_rxfh_configured(priv->netdev)) { + err = -EINVAL; + netdev_err(priv->netdev, "%s: RXFH is configured, cannot change the number of channels\n", + __func__); + goto out; + } + /* Don't allow changing the number of channels if HTB offload is active, * because the numeration of the QoS SQs will change, while per-queue * qdiscs are attached.