| Message ID | 20230415054855.9293-3-jdamato@fastly.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ixgbe: Multiple RSS bugfixes | expand |
On 4/15/2023 12:48 AM, Joe Damato wrote: > ethtool uses `ETHTOOL_GRXRINGS` to compute how many queues are supported > by RSS. The driver should return the smaller of either: > - The maximum number of RSS queues the device supports, OR > - The number of RX queues configured > > Prior to this change, running `ethtool -X $iface default` fails if the > number of queues configured is larger than the number supported by RSS, > even though changing the queue count correctly resets the flowhash to > use all supported queues. > > Other drivers (for example, i40e) will succeed but the flow hash will > reset to support the maximum number of queues supported by RSS, even if > that amount is smaller than the configured amount. > > Prior to this change: > > $ sudo ethtool -L eth1 combined 20 > $ sudo ethtool -x eth1 > RX flow hash indirection table for eth1 with 20 RX ring(s): > 0: 0 1 2 3 4 5 6 7 > 8: 8 9 10 11 12 13 14 15 > 16: 0 1 2 3 4 5 6 7 > 24: 8 9 10 11 12 13 14 15 > 32: 0 1 2 3 4 5 6 7 > ... > > You can see that the flowhash was correctly set to use the maximum > number of queues supported by the driver (16). > > However, asking the NIC to reset to "default" fails: > > $ sudo ethtool -X eth1 default > Cannot set RX flow hash configuration: Invalid argument > > After this change, the flowhash can be reset to default which will use > all of the available RSS queues (16) or the configured queue count, > whichever is smaller. > > Starting with eth1 which has 10 queues and a flowhash distributing to > all 10 queues: > > $ sudo ethtool -x eth1 > RX flow hash indirection table for eth1 with 10 RX ring(s): > 0: 0 1 2 3 4 5 6 7 > 8: 8 9 0 1 2 3 4 5 > 16: 6 7 8 9 0 1 2 3 > ... > > Increasing the queue count to 48 resets the flowhash to distribute to 16 > queues, as it did before this patch: > > $ sudo ethtool -L eth1 combined 48 > $ sudo ethtool -x eth1 > RX flow hash indirection table for eth1 with 16 RX ring(s): > 0: 0 1 2 3 4 5 6 7 > 8: 8 9 10 11 12 13 14 15 > 16: 0 1 2 3 4 5 6 7 > ... > > Due to the other bugfix in this series, the flowhash can be set to use > queues 0-5: > > $ sudo ethtool -X eth1 equal 5 > $ sudo ethtool -x eth1 > RX flow hash indirection table for eth1 with 16 RX ring(s): > 0: 0 1 2 3 4 0 1 2 > 8: 3 4 0 1 2 3 4 0 > 16: 1 2 3 4 0 1 2 3 > ... > > Due to this bugfix, the flowhash can be reset to default and use 16 > queues: > > $ sudo ethtool -X eth1 default > $ sudo ethtool -x eth1 > RX flow hash indirection table for eth1 with 16 RX ring(s): > 0: 0 1 2 3 4 5 6 7 > 8: 8 9 10 11 12 13 14 15 > 16: 0 1 2 3 4 5 6 7 > ... > > Signed-off-by: Joe Damato <jdamato@fastly.com> Thanks for the detailed commit message and steps to reproduce and validate the issue. Would suggest changing the title to indicate that this fix is enabling setting the RSS indirection table to default value. Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com> > --- > .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > index 821dfd323fa9..0bbad4a5cc2f 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > @@ -2665,6 +2665,14 @@ static int ixgbe_get_rss_hash_opts(struct ixgbe_adapter *adapter, > return 0; > } > > +static int ixgbe_rss_indir_tbl_max(struct ixgbe_adapter *adapter) > +{ > + if (adapter->hw.mac.type < ixgbe_mac_X550) > + return 16; > + else > + return 64; > +} > + > static int ixgbe_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd, > u32 *rule_locs) > { > @@ -2673,7 +2681,8 @@ static int ixgbe_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd, > > switch (cmd->cmd) { > case ETHTOOL_GRXRINGS: > - cmd->data = adapter->num_rx_queues; > + cmd->data = min_t(int, adapter->num_rx_queues, > + ixgbe_rss_indir_tbl_max(adapter)); > ret = 0; > break; > case ETHTOOL_GRXCLSRLCNT: > @@ -3075,14 +3084,6 @@ static int ixgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd) > return ret; > } > > -static int ixgbe_rss_indir_tbl_max(struct ixgbe_adapter *adapter) > -{ > - if (adapter->hw.mac.type < ixgbe_mac_X550) > - return 16; > - else > - return 64; > -} > - > static u32 ixgbe_get_rxfh_key_size(struct net_device *netdev) > { > return IXGBE_RSS_KEY_SIZE;
On Sun, Apr 16, 2023 at 10:29:36AM -0500, Samudrala, Sridhar wrote: > > > On 4/15/2023 12:48 AM, Joe Damato wrote: > >ethtool uses `ETHTOOL_GRXRINGS` to compute how many queues are supported > >by RSS. The driver should return the smaller of either: > > - The maximum number of RSS queues the device supports, OR > > - The number of RX queues configured > > > >Prior to this change, running `ethtool -X $iface default` fails if the > >number of queues configured is larger than the number supported by RSS, > >even though changing the queue count correctly resets the flowhash to > >use all supported queues. > > > >Other drivers (for example, i40e) will succeed but the flow hash will > >reset to support the maximum number of queues supported by RSS, even if > >that amount is smaller than the configured amount. > > > >Prior to this change: > > > >$ sudo ethtool -L eth1 combined 20 > >$ sudo ethtool -x eth1 > >RX flow hash indirection table for eth1 with 20 RX ring(s): > > 0: 0 1 2 3 4 5 6 7 > > 8: 8 9 10 11 12 13 14 15 > > 16: 0 1 2 3 4 5 6 7 > > 24: 8 9 10 11 12 13 14 15 > > 32: 0 1 2 3 4 5 6 7 > >... > > > >You can see that the flowhash was correctly set to use the maximum > >number of queues supported by the driver (16). > > > >However, asking the NIC to reset to "default" fails: > > > >$ sudo ethtool -X eth1 default > >Cannot set RX flow hash configuration: Invalid argument > > > >After this change, the flowhash can be reset to default which will use > >all of the available RSS queues (16) or the configured queue count, > >whichever is smaller. > > > >Starting with eth1 which has 10 queues and a flowhash distributing to > >all 10 queues: > > > >$ sudo ethtool -x eth1 > >RX flow hash indirection table for eth1 with 10 RX ring(s): > > 0: 0 1 2 3 4 5 6 7 > > 8: 8 9 0 1 2 3 4 5 > > 16: 6 7 8 9 0 1 2 3 > >... > > > >Increasing the queue count to 48 resets the flowhash to distribute to 16 > >queues, as it did before this patch: > > > >$ sudo ethtool -L eth1 combined 48 > >$ sudo ethtool -x eth1 > >RX flow hash indirection table for eth1 with 16 RX ring(s): > > 0: 0 1 2 3 4 5 6 7 > > 8: 8 9 10 11 12 13 14 15 > > 16: 0 1 2 3 4 5 6 7 > >... > > > >Due to the other bugfix in this series, the flowhash can be set to use > >queues 0-5: > > > >$ sudo ethtool -X eth1 equal 5 > >$ sudo ethtool -x eth1 > >RX flow hash indirection table for eth1 with 16 RX ring(s): > > 0: 0 1 2 3 4 0 1 2 > > 8: 3 4 0 1 2 3 4 0 > > 16: 1 2 3 4 0 1 2 3 > >... > > > >Due to this bugfix, the flowhash can be reset to default and use 16 > >queues: > > > >$ sudo ethtool -X eth1 default > >$ sudo ethtool -x eth1 > >RX flow hash indirection table for eth1 with 16 RX ring(s): > > 0: 0 1 2 3 4 5 6 7 > > 8: 8 9 10 11 12 13 14 15 > > 16: 0 1 2 3 4 5 6 7 > >... > > > >Signed-off-by: Joe Damato <jdamato@fastly.com> > > Thanks for the detailed commit message and steps to reproduce > and validate the issue. No worries. Thanks for the review. > Would suggest changing the title to indicate that this fix is enabling > setting the RSS indirection table to default value. OK, sure. I can send a v2 that includes your reviewed tags and changes the title of this commit (but makes no other changes). I'll make the new title of this commit ixgbe: Enable setting RSS table to default values. Hope that's ok; will send out the v2 shortly. > Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com> > > >--- > > .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 19 ++++++++++--------- > > 1 file changed, 10 insertions(+), 9 deletions(-) > > > >diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > >index 821dfd323fa9..0bbad4a5cc2f 100644 > >--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > >+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > >@@ -2665,6 +2665,14 @@ static int ixgbe_get_rss_hash_opts(struct ixgbe_adapter *adapter, > > return 0; > > } > >+static int ixgbe_rss_indir_tbl_max(struct ixgbe_adapter *adapter) > >+{ > >+ if (adapter->hw.mac.type < ixgbe_mac_X550) > >+ return 16; > >+ else > >+ return 64; > >+} > >+ > > static int ixgbe_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd, > > u32 *rule_locs) > > { > >@@ -2673,7 +2681,8 @@ static int ixgbe_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd, > > switch (cmd->cmd) { > > case ETHTOOL_GRXRINGS: > >- cmd->data = adapter->num_rx_queues; > >+ cmd->data = min_t(int, adapter->num_rx_queues, > >+ ixgbe_rss_indir_tbl_max(adapter)); > > ret = 0; > > break; > > case ETHTOOL_GRXCLSRLCNT: > >@@ -3075,14 +3084,6 @@ static int ixgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd) > > return ret; > > } > >-static int ixgbe_rss_indir_tbl_max(struct ixgbe_adapter *adapter) > >-{ > >- if (adapter->hw.mac.type < ixgbe_mac_X550) > >- return 16; > >- else > >- return 64; > >-} > >- > > static u32 ixgbe_get_rxfh_key_size(struct net_device *netdev) > > { > > return IXGBE_RSS_KEY_SIZE;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c index 821dfd323fa9..0bbad4a5cc2f 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c @@ -2665,6 +2665,14 @@ static int ixgbe_get_rss_hash_opts(struct ixgbe_adapter *adapter, return 0; } +static int ixgbe_rss_indir_tbl_max(struct ixgbe_adapter *adapter) +{ + if (adapter->hw.mac.type < ixgbe_mac_X550) + return 16; + else + return 64; +} + static int ixgbe_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd, u32 *rule_locs) { @@ -2673,7 +2681,8 @@ static int ixgbe_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd, switch (cmd->cmd) { case ETHTOOL_GRXRINGS: - cmd->data = adapter->num_rx_queues; + cmd->data = min_t(int, adapter->num_rx_queues, + ixgbe_rss_indir_tbl_max(adapter)); ret = 0; break; case ETHTOOL_GRXCLSRLCNT: @@ -3075,14 +3084,6 @@ static int ixgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd) return ret; } -static int ixgbe_rss_indir_tbl_max(struct ixgbe_adapter *adapter) -{ - if (adapter->hw.mac.type < ixgbe_mac_X550) - return 16; - else - return 64; -} - static u32 ixgbe_get_rxfh_key_size(struct net_device *netdev) { return IXGBE_RSS_KEY_SIZE;
ethtool uses `ETHTOOL_GRXRINGS` to compute how many queues are supported by RSS. The driver should return the smaller of either: - The maximum number of RSS queues the device supports, OR - The number of RX queues configured Prior to this change, running `ethtool -X $iface default` fails if the number of queues configured is larger than the number supported by RSS, even though changing the queue count correctly resets the flowhash to use all supported queues. Other drivers (for example, i40e) will succeed but the flow hash will reset to support the maximum number of queues supported by RSS, even if that amount is smaller than the configured amount. Prior to this change: $ sudo ethtool -L eth1 combined 20 $ sudo ethtool -x eth1 RX flow hash indirection table for eth1 with 20 RX ring(s): 0: 0 1 2 3 4 5 6 7 8: 8 9 10 11 12 13 14 15 16: 0 1 2 3 4 5 6 7 24: 8 9 10 11 12 13 14 15 32: 0 1 2 3 4 5 6 7 ... You can see that the flowhash was correctly set to use the maximum number of queues supported by the driver (16). However, asking the NIC to reset to "default" fails: $ sudo ethtool -X eth1 default Cannot set RX flow hash configuration: Invalid argument After this change, the flowhash can be reset to default which will use all of the available RSS queues (16) or the configured queue count, whichever is smaller. Starting with eth1 which has 10 queues and a flowhash distributing to all 10 queues: $ sudo ethtool -x eth1 RX flow hash indirection table for eth1 with 10 RX ring(s): 0: 0 1 2 3 4 5 6 7 8: 8 9 0 1 2 3 4 5 16: 6 7 8 9 0 1 2 3 ... Increasing the queue count to 48 resets the flowhash to distribute to 16 queues, as it did before this patch: $ sudo ethtool -L eth1 combined 48 $ sudo ethtool -x eth1 RX flow hash indirection table for eth1 with 16 RX ring(s): 0: 0 1 2 3 4 5 6 7 8: 8 9 10 11 12 13 14 15 16: 0 1 2 3 4 5 6 7 ... Due to the other bugfix in this series, the flowhash can be set to use queues 0-5: $ sudo ethtool -X eth1 equal 5 $ sudo ethtool -x eth1 RX flow hash indirection table for eth1 with 16 RX ring(s): 0: 0 1 2 3 4 0 1 2 8: 3 4 0 1 2 3 4 0 16: 1 2 3 4 0 1 2 3 ... Due to this bugfix, the flowhash can be reset to default and use 16 queues: $ sudo ethtool -X eth1 default $ sudo ethtool -x eth1 RX flow hash indirection table for eth1 with 16 RX ring(s): 0: 0 1 2 3 4 5 6 7 8: 8 9 10 11 12 13 14 15 16: 0 1 2 3 4 5 6 7 ... Signed-off-by: Joe Damato <jdamato@fastly.com> --- .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)