drivers/net/ethernet/google/gve/gve.h | 16 +++- drivers/net/ethernet/google/gve/gve_adminq.c | 64 ++++++++++--- drivers/net/ethernet/google/gve/gve_ethtool.c | 60 ++++++++++-- drivers/net/ethernet/google/gve/gve_main.c | 92 ++++++++++++++++++- 4 files changed, 209 insertions(+), 23 deletions(-)
From: Ziwei Xiao <ziweixiao@google.com>
Not all the devices have the capability for the driver to query for the
registered RSS configuration. The driver can discover this by checking
the relevant device option during setup. If it cannot, the driver needs
to store the RSS config cache and directly return such cache when
queried by the ethtool. RSS config is inited when driver probes. Also the
default RSS config will be adjusted when there is RX queue count change.
At this point, only keys of GVE_RSS_KEY_SIZE and indirection tables of
GVE_RSS_INDIR_SIZE are supported.
Signed-off-by: Ziwei Xiao <ziweixiao@google.com>
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Praveen Kaligineedi <pkaligineedi@google.com>
Signed-off-by: Jeroen de Borst <jeroend@google.com>
---
Changes in v2:
- Change to initialize RSS config when the driver is up instead of
doing that when the user setting the RSS.(Jakub Kicinski)
- Use NL_SET_ERR_MSG_MOD to log errors when there is extack
available.(Jakub Kicinski)
- Use ethtool_rxfh_indir_default to set default RSS indir
table.(Jakub Kicinski)
- Adjust the default RSS config when there is RX queue count change to
ensure the default RSS config is correct.
drivers/net/ethernet/google/gve/gve.h | 16 +++-
drivers/net/ethernet/google/gve/gve_adminq.c | 64 ++++++++++---
drivers/net/ethernet/google/gve/gve_ethtool.c | 60 ++++++++++--
drivers/net/ethernet/google/gve/gve_main.c | 92 ++++++++++++++++++-
4 files changed, 209 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 8167cc5fb0df..8e1e706c6f5e 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -68,6 +68,9 @@
#define GVE_FLOW_RULE_IDS_CACHE_SIZE \
(GVE_ADMINQ_BUFFER_SIZE / sizeof(((struct gve_adminq_queried_flow_rule *)0)->location))
+#define GVE_RSS_KEY_SIZE 40
+#define GVE_RSS_INDIR_SIZE 128
+
#define GVE_XDP_ACTIONS 5
#define GVE_GQ_TX_MIN_PKT_DESC_BYTES 182
@@ -672,6 +675,7 @@ struct gve_rx_alloc_rings_cfg {
u16 packet_buffer_size;
bool raw_addressing;
bool enable_header_split;
+ bool reset_rss;
/* Allocated resources are returned here */
struct gve_rx_ring *rx;
@@ -722,6 +726,11 @@ struct gve_flow_rules_cache {
u32 rule_ids_cache_num;
};
+struct gve_rss_config {
+ u8 *hash_key;
+ u32 *hash_lut;
+};
+
struct gve_priv {
struct net_device *dev;
struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
@@ -842,6 +851,8 @@ struct gve_priv {
u16 rss_key_size;
u16 rss_lut_size;
+ bool cache_rss_config;
+ struct gve_rss_config rss_config;
};
enum gve_service_task_flags_bit {
@@ -1210,13 +1221,16 @@ int gve_adjust_config(struct gve_priv *priv,
struct gve_rx_alloc_rings_cfg *rx_alloc_cfg);
int gve_adjust_queues(struct gve_priv *priv,
struct gve_queue_config new_rx_config,
- struct gve_queue_config new_tx_config);
+ struct gve_queue_config new_tx_config,
+ bool reset_rss);
/* flow steering rule */
int gve_get_flow_rule_entry(struct gve_priv *priv, struct ethtool_rxnfc *cmd);
int gve_get_flow_rule_ids(struct gve_priv *priv, struct ethtool_rxnfc *cmd, u32 *rule_locs);
int gve_add_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd);
int gve_del_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd);
int gve_flow_rules_reset(struct gve_priv *priv);
+/* RSS config */
+int gve_init_rss_config(struct gve_priv *priv, u16 num_queues);
/* report stats handling */
void gve_handle_report_stats(struct gve_priv *priv);
/* exported by ethtool.c */
diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index aa7d723011d0..a9ae094456cb 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -885,6 +885,15 @@ static void gve_set_default_desc_cnt(struct gve_priv *priv,
priv->min_rx_desc_cnt = priv->rx_desc_cnt;
}
+static void gve_set_default_rss_sizes(struct gve_priv *priv)
+{
+ if (!gve_is_gqi(priv)) {
+ priv->rss_key_size = GVE_RSS_KEY_SIZE;
+ priv->rss_lut_size = GVE_RSS_INDIR_SIZE;
+ priv->cache_rss_config = true;
+ }
+}
+
static void gve_enable_supported_features(struct gve_priv *priv,
u32 supported_features_mask,
const struct gve_device_option_jumbo_frames
@@ -968,6 +977,10 @@ static void gve_enable_supported_features(struct gve_priv *priv,
be16_to_cpu(dev_op_rss_config->hash_key_size);
priv->rss_lut_size =
be16_to_cpu(dev_op_rss_config->hash_lut_size);
+ priv->cache_rss_config = false;
+ dev_dbg(&priv->pdev->dev,
+ "RSS device option enabled with key size of %u, lut size of %u.\n",
+ priv->rss_key_size, priv->rss_lut_size);
}
}
@@ -1052,6 +1065,8 @@ int gve_adminq_describe_device(struct gve_priv *priv)
/* set default descriptor counts */
gve_set_default_desc_cnt(priv, descriptor);
+ gve_set_default_rss_sizes(priv);
+
/* DQO supports LRO. */
if (!gve_is_gqi(priv))
priv->dev->hw_features |= NETIF_F_LRO;
@@ -1276,8 +1291,9 @@ int gve_adminq_reset_flow_rules(struct gve_priv *priv)
int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *rxfh)
{
+ const u32 *hash_lut_to_config = NULL;
+ const u8 *hash_key_to_config = NULL;
dma_addr_t lut_bus = 0, key_bus = 0;
- u16 key_size = 0, lut_size = 0;
union gve_adminq_command cmd;
__be32 *lut = NULL;
u8 hash_alg = 0;
@@ -1287,7 +1303,7 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
switch (rxfh->hfunc) {
case ETH_RSS_HASH_NO_CHANGE:
- break;
+ fallthrough;
case ETH_RSS_HASH_TOP:
hash_alg = ETH_RSS_HASH_TOP;
break;
@@ -1296,27 +1312,44 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
}
if (rxfh->indir) {
- lut_size = priv->rss_lut_size;
+ if (rxfh->indir_size != priv->rss_lut_size)
+ return -EINVAL;
+
+ hash_lut_to_config = rxfh->indir;
+ } else if (priv->cache_rss_config) {
+ hash_lut_to_config = priv->rss_config.hash_lut;
+ }
+
+ if (hash_lut_to_config) {
lut = dma_alloc_coherent(&priv->pdev->dev,
- lut_size * sizeof(*lut),
+ priv->rss_lut_size * sizeof(*lut),
&lut_bus, GFP_KERNEL);
if (!lut)
return -ENOMEM;
for (i = 0; i < priv->rss_lut_size; i++)
- lut[i] = cpu_to_be32(rxfh->indir[i]);
+ lut[i] = cpu_to_be32(hash_lut_to_config[i]);
}
if (rxfh->key) {
- key_size = priv->rss_key_size;
+ if (rxfh->key_size != priv->rss_key_size)
+ return -EINVAL;
+
+ hash_key_to_config = rxfh->key;
+ } else if (priv->cache_rss_config) {
+ hash_key_to_config = priv->rss_config.hash_key;
+ }
+
+ if (hash_key_to_config) {
key = dma_alloc_coherent(&priv->pdev->dev,
- key_size, &key_bus, GFP_KERNEL);
+ priv->rss_key_size,
+ &key_bus, GFP_KERNEL);
if (!key) {
err = -ENOMEM;
goto out;
}
- memcpy(key, rxfh->key, key_size);
+ memcpy(key, hash_key_to_config, priv->rss_key_size);
}
/* Zero-valued fields in the cmd.configure_rss instruct the device to
@@ -1330,8 +1363,10 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
BIT(GVE_RSS_HASH_TCPV6) |
BIT(GVE_RSS_HASH_UDPV6)),
.hash_alg = hash_alg,
- .hash_key_size = cpu_to_be16(key_size),
- .hash_lut_size = cpu_to_be16(lut_size),
+ .hash_key_size =
+ cpu_to_be16((key_bus) ? priv->rss_key_size : 0),
+ .hash_lut_size =
+ cpu_to_be16((lut_bus) ? priv->rss_lut_size : 0),
.hash_key_addr = cpu_to_be64(key_bus),
.hash_lut_addr = cpu_to_be64(lut_bus),
};
@@ -1341,11 +1376,11 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
out:
if (lut)
dma_free_coherent(&priv->pdev->dev,
- lut_size * sizeof(*lut),
+ priv->rss_lut_size * sizeof(*lut),
lut, lut_bus);
if (key)
dma_free_coherent(&priv->pdev->dev,
- key_size, key, key_bus);
+ priv->rss_key_size, key, key_bus);
return err;
}
@@ -1449,12 +1484,15 @@ static int gve_adminq_process_rss_query(struct gve_priv *priv,
rxfh->hfunc = descriptor->hash_alg;
rss_info_addr = (void *)(descriptor + 1);
- if (rxfh->key)
+ if (rxfh->key) {
+ rxfh->key_size = priv->rss_key_size;
memcpy(rxfh->key, rss_info_addr, priv->rss_key_size);
+ }
rss_info_addr += priv->rss_key_size;
lut = (__be32 *)rss_info_addr;
if (rxfh->indir) {
+ rxfh->indir_size = priv->rss_lut_size;
for (i = 0; i < priv->rss_lut_size; i++)
rxfh->indir[i] = be32_to_cpu(lut[i]);
}
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index bdfc6e77b2af..efcafc607b2a 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -482,6 +482,7 @@ static int gve_set_channels(struct net_device *netdev,
struct ethtool_channels old_settings;
int new_tx = cmd->tx_count;
int new_rx = cmd->rx_count;
+ bool reset_rss;
gve_get_channels(netdev, &old_settings);
@@ -498,16 +499,14 @@ static int gve_set_channels(struct net_device *netdev,
return -EINVAL;
}
- if (!netif_running(netdev)) {
- priv->tx_cfg.num_queues = new_tx;
- priv->rx_cfg.num_queues = new_rx;
- return 0;
- }
+ if (new_rx != priv->rx_cfg.num_queues &&
+ priv->cache_rss_config && !netif_is_rxfh_configured(netdev))
+ reset_rss = true;
new_tx_cfg.num_queues = new_tx;
new_rx_cfg.num_queues = new_rx;
- return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg);
+ return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg, reset_rss);
}
static void gve_get_ringparam(struct net_device *netdev,
@@ -855,6 +854,25 @@ static u32 gve_get_rxfh_indir_size(struct net_device *netdev)
return priv->rss_lut_size;
}
+static void gve_get_rss_config_cache(struct gve_priv *priv,
+ struct ethtool_rxfh_param *rxfh)
+{
+ struct gve_rss_config *rss_config = &priv->rss_config;
+
+ rxfh->hfunc = ETH_RSS_HASH_TOP;
+
+ if (rxfh->key) {
+ rxfh->key_size = priv->rss_key_size;
+ memcpy(rxfh->key, rss_config->hash_key, priv->rss_key_size);
+ }
+
+ if (rxfh->indir) {
+ rxfh->indir_size = priv->rss_lut_size;
+ memcpy(rxfh->indir, rss_config->hash_lut,
+ priv->rss_lut_size * sizeof(*rxfh->indir));
+ }
+}
+
static int gve_get_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh)
{
struct gve_priv *priv = netdev_priv(netdev);
@@ -862,18 +880,46 @@ static int gve_get_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rx
if (!priv->rss_key_size || !priv->rss_lut_size)
return -EOPNOTSUPP;
+ if (priv->cache_rss_config) {
+ gve_get_rss_config_cache(priv, rxfh);
+ return 0;
+ }
+
return gve_adminq_query_rss_config(priv, rxfh);
}
+static void gve_set_rss_config_cache(struct gve_priv *priv,
+ struct ethtool_rxfh_param *rxfh)
+{
+ struct gve_rss_config *rss_config = &priv->rss_config;
+
+ if (rxfh->key)
+ memcpy(rss_config->hash_key, rxfh->key, priv->rss_key_size);
+
+ if (rxfh->indir)
+ memcpy(rss_config->hash_lut, rxfh->indir,
+ priv->rss_lut_size * sizeof(*rxfh->indir));
+}
+
static int gve_set_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh,
struct netlink_ext_ack *extack)
{
struct gve_priv *priv = netdev_priv(netdev);
+ int err;
if (!priv->rss_key_size || !priv->rss_lut_size)
return -EOPNOTSUPP;
- return gve_adminq_configure_rss(priv, rxfh);
+ err = gve_adminq_configure_rss(priv, rxfh);
+ if (err) {
+ NL_SET_ERR_MSG_MOD(extack, "Fail to configure RSS config\n");
+ return err;
+ }
+
+ if (priv->cache_rss_config)
+ gve_set_rss_config_cache(priv, rxfh);
+
+ return 0;
}
const struct ethtool_ops gve_ethtool_ops = {
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 533e659b15b3..d726a4d39b45 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -184,6 +184,49 @@ static void gve_free_flow_rule_caches(struct gve_priv *priv)
flow_rules_cache->rules_cache = NULL;
}
+static int gve_alloc_rss_config_cache(struct gve_priv *priv)
+{
+ struct gve_rss_config *rss_config = &priv->rss_config;
+ int err = 0;
+
+ if (!priv->cache_rss_config)
+ return 0;
+
+ rss_config->hash_key = kcalloc(priv->rss_key_size,
+ sizeof(rss_config->hash_key[0]),
+ GFP_KERNEL);
+ if (!rss_config->hash_key) {
+ dev_err(&priv->pdev->dev, "Cannot alloc rss key cache\n");
+ return -ENOMEM;
+ }
+
+ rss_config->hash_lut = kcalloc(priv->rss_lut_size,
+ sizeof(rss_config->hash_lut[0]),
+ GFP_KERNEL);
+ if (!rss_config->hash_lut) {
+ dev_err(&priv->pdev->dev, "Cannot alloc rss lut cache\n");
+ err = -ENOMEM;
+ goto free_rss_key_cache;
+ }
+
+ return 0;
+
+free_rss_key_cache:
+ kfree(rss_config->hash_key);
+ rss_config->hash_key = NULL;
+ return err;
+}
+
+static void gve_free_rss_config_cache(struct gve_priv *priv)
+{
+ struct gve_rss_config *rss_config = &priv->rss_config;
+
+ kfree(rss_config->hash_key);
+ kfree(rss_config->hash_lut);
+
+ memset(rss_config, 0, sizeof(*rss_config));
+}
+
static int gve_alloc_counter_array(struct gve_priv *priv)
{
priv->counter_array =
@@ -575,9 +618,12 @@ static int gve_setup_device_resources(struct gve_priv *priv)
err = gve_alloc_flow_rule_caches(priv);
if (err)
return err;
- err = gve_alloc_counter_array(priv);
+ err = gve_alloc_rss_config_cache(priv);
if (err)
goto abort_with_flow_rule_caches;
+ err = gve_alloc_counter_array(priv);
+ if (err)
+ goto abort_with_rss_config_cache;
err = gve_alloc_notify_blocks(priv);
if (err)
goto abort_with_counter;
@@ -611,6 +657,12 @@ static int gve_setup_device_resources(struct gve_priv *priv)
}
}
+ err = gve_init_rss_config(priv, priv->rx_cfg.num_queues);
+ if (err) {
+ dev_err(&priv->pdev->dev, "Failed to init RSS config");
+ goto abort_with_ptype_lut;
+ }
+
err = gve_adminq_report_stats(priv, priv->stats_report_len,
priv->stats_report_bus,
GVE_STATS_REPORT_TIMER_PERIOD);
@@ -629,6 +681,8 @@ static int gve_setup_device_resources(struct gve_priv *priv)
gve_free_notify_blocks(priv);
abort_with_counter:
gve_free_counter_array(priv);
+abort_with_rss_config_cache:
+ gve_free_rss_config_cache(priv);
abort_with_flow_rule_caches:
gve_free_flow_rule_caches(priv);
@@ -669,6 +723,7 @@ static void gve_teardown_device_resources(struct gve_priv *priv)
priv->ptype_lut_dqo = NULL;
gve_free_flow_rule_caches(priv);
+ gve_free_rss_config_cache(priv);
gve_free_counter_array(priv);
gve_free_notify_blocks(priv);
gve_free_stats_report(priv);
@@ -1390,6 +1445,12 @@ static int gve_queues_start(struct gve_priv *priv,
if (err)
goto stop_and_free_rings;
+ if (rx_alloc_cfg->reset_rss) {
+ err = gve_init_rss_config(priv, priv->rx_cfg.num_queues);
+ if (err)
+ goto reset;
+ }
+
err = gve_register_qpls(priv);
if (err)
goto reset;
@@ -1786,6 +1847,26 @@ static int gve_xdp(struct net_device *dev, struct netdev_bpf *xdp)
}
}
+int gve_init_rss_config(struct gve_priv *priv, u16 num_queues)
+{
+ struct gve_rss_config *rss_config = &priv->rss_config;
+ struct ethtool_rxfh_param rxfh;
+ u16 i;
+
+ if (!priv->cache_rss_config)
+ return 0;
+
+ for (i = 0; i < priv->rss_lut_size; i++)
+ rss_config->hash_lut[i] =
+ ethtool_rxfh_indir_default(i, num_queues);
+
+ netdev_rss_key_fill(rss_config->hash_key, priv->rss_key_size);
+
+ rxfh.hfunc = ETH_RSS_HASH_TOP;
+
+ return gve_adminq_configure_rss(priv, &rxfh);
+}
+
int gve_flow_rules_reset(struct gve_priv *priv)
{
if (!priv->max_flow_rules)
@@ -1834,7 +1915,8 @@ int gve_adjust_config(struct gve_priv *priv,
int gve_adjust_queues(struct gve_priv *priv,
struct gve_queue_config new_rx_config,
- struct gve_queue_config new_tx_config)
+ struct gve_queue_config new_tx_config,
+ bool reset_rss)
{
struct gve_tx_alloc_rings_cfg tx_alloc_cfg = {0};
struct gve_rx_alloc_rings_cfg rx_alloc_cfg = {0};
@@ -1847,6 +1929,7 @@ int gve_adjust_queues(struct gve_priv *priv,
tx_alloc_cfg.qcfg = &new_tx_config;
rx_alloc_cfg.qcfg_tx = &new_tx_config;
rx_alloc_cfg.qcfg = &new_rx_config;
+ rx_alloc_cfg.reset_rss = reset_rss;
tx_alloc_cfg.num_rings = new_tx_config.num_queues;
/* Add dedicated XDP TX queues if enabled. */
@@ -1858,6 +1941,11 @@ int gve_adjust_queues(struct gve_priv *priv,
return err;
}
/* Set the config for the next up. */
+ if (reset_rss) {
+ err = gve_init_rss_config(priv, new_rx_config.num_queues);
+ if (err)
+ return err;
+ }
priv->tx_cfg = new_tx_config;
priv->rx_cfg = new_rx_config;
--
2.48.1.362.g079036d154-googHi Jeroen,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Jeroen-de-Borst/gve-Add-RSS-cache-for-non-RSS-device-option-scenario/20250205-053317
base: net-next/main
patch link: https://lore.kernel.org/r/20250204213121.14195-1-jeroendb%40google.com
patch subject: [PATCH net-next v2] gve: Add RSS cache for non RSS device option scenario
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20250206/202502061802.Co0dVfXU-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250206/202502061802.Co0dVfXU-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502061802.Co0dVfXU-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/net/ethernet/google/gve/gve_ethtool.c:7:
In file included from include/linux/rtnetlink.h:7:
In file included from include/linux/netdevice.h:38:
In file included from include/net/net_namespace.h:43:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:8:
In file included from include/linux/cacheflush.h:5:
In file included from arch/x86/include/asm/cacheflush.h:5:
In file included from include/linux/mm.h:2224:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
505 | item];
| ~~~~
include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
512 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
525 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/ethernet/google/gve/gve_ethtool.c:502:6: warning: variable 'reset_rss' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
502 | if (new_rx != priv->rx_cfg.num_queues &&
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
503 | priv->cache_rss_config && !netif_is_rxfh_configured(netdev))
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/google/gve/gve_ethtool.c:509:57: note: uninitialized use occurs here
509 | return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg, reset_rss);
| ^~~~~~~~~
drivers/net/ethernet/google/gve/gve_ethtool.c:502:2: note: remove the 'if' if its condition is always true
502 | if (new_rx != priv->rx_cfg.num_queues &&
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
503 | priv->cache_rss_config && !netif_is_rxfh_configured(netdev))
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
504 | reset_rss = true;
>> drivers/net/ethernet/google/gve/gve_ethtool.c:502:6: warning: variable 'reset_rss' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
502 | if (new_rx != priv->rx_cfg.num_queues &&
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
503 | priv->cache_rss_config && !netif_is_rxfh_configured(netdev))
| ~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/google/gve/gve_ethtool.c:509:57: note: uninitialized use occurs here
509 | return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg, reset_rss);
| ^~~~~~~~~
drivers/net/ethernet/google/gve/gve_ethtool.c:502:6: note: remove the '&&' if its condition is always true
502 | if (new_rx != priv->rx_cfg.num_queues &&
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
503 | priv->cache_rss_config && !netif_is_rxfh_configured(netdev))
| ~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/ethernet/google/gve/gve_ethtool.c:502:6: warning: variable 'reset_rss' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
502 | if (new_rx != priv->rx_cfg.num_queues &&
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/google/gve/gve_ethtool.c:509:57: note: uninitialized use occurs here
509 | return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg, reset_rss);
| ^~~~~~~~~
drivers/net/ethernet/google/gve/gve_ethtool.c:502:6: note: remove the '&&' if its condition is always true
502 | if (new_rx != priv->rx_cfg.num_queues &&
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/google/gve/gve_ethtool.c:485:16: note: initialize the variable 'reset_rss' to silence this warning
485 | bool reset_rss;
| ^
| = 0
6 warnings generated.
vim +502 drivers/net/ethernet/google/gve/gve_ethtool.c
475
476 static int gve_set_channels(struct net_device *netdev,
477 struct ethtool_channels *cmd)
478 {
479 struct gve_priv *priv = netdev_priv(netdev);
480 struct gve_queue_config new_tx_cfg = priv->tx_cfg;
481 struct gve_queue_config new_rx_cfg = priv->rx_cfg;
482 struct ethtool_channels old_settings;
483 int new_tx = cmd->tx_count;
484 int new_rx = cmd->rx_count;
485 bool reset_rss;
486
487 gve_get_channels(netdev, &old_settings);
488
489 /* Changing combined is not allowed */
490 if (cmd->combined_count != old_settings.combined_count)
491 return -EINVAL;
492
493 if (!new_rx || !new_tx)
494 return -EINVAL;
495
496 if (priv->num_xdp_queues &&
497 (new_tx != new_rx || (2 * new_tx > priv->tx_cfg.max_queues))) {
498 dev_err(&priv->pdev->dev, "XDP load failed: The number of configured RX queues should be equal to the number of configured TX queues and the number of configured RX/TX queues should be less than or equal to half the maximum number of RX/TX queues");
499 return -EINVAL;
500 }
501
> 502 if (new_rx != priv->rx_cfg.num_queues &&
503 priv->cache_rss_config && !netif_is_rxfh_configured(netdev))
504 reset_rss = true;
505
506 new_tx_cfg.num_queues = new_tx;
507 new_rx_cfg.num_queues = new_rx;
508
> 509 return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg, reset_rss);
510 }
511
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Jerome,
On Wed, Feb 5, 2025 at 3:02 AM Jeroen de Borst <jeroendb@google.com> wrote:
>
> From: Ziwei Xiao <ziweixiao@google.com>
>
> Not all the devices have the capability for the driver to query for the
> registered RSS configuration. The driver can discover this by checking
> the relevant device option during setup. If it cannot, the driver needs
> to store the RSS config cache and directly return such cache when
> queried by the ethtool. RSS config is inited when driver probes. Also the
> default RSS config will be adjusted when there is RX queue count change.
>
> At this point, only keys of GVE_RSS_KEY_SIZE and indirection tables of
> GVE_RSS_INDIR_SIZE are supported.
>
> Signed-off-by: Ziwei Xiao <ziweixiao@google.com>
> Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Praveen Kaligineedi <pkaligineedi@google.com>
> Signed-off-by: Jeroen de Borst <jeroend@google.com>
> ---
> Changes in v2:
> - Change to initialize RSS config when the driver is up instead of
> doing that when the user setting the RSS.(Jakub Kicinski)
> - Use NL_SET_ERR_MSG_MOD to log errors when there is extack
> available.(Jakub Kicinski)
> - Use ethtool_rxfh_indir_default to set default RSS indir
> table.(Jakub Kicinski)
> - Adjust the default RSS config when there is RX queue count change to
> ensure the default RSS config is correct.
>
> drivers/net/ethernet/google/gve/gve.h | 16 +++-
> drivers/net/ethernet/google/gve/gve_adminq.c | 64 ++++++++++---
> drivers/net/ethernet/google/gve/gve_ethtool.c | 60 ++++++++++--
> drivers/net/ethernet/google/gve/gve_main.c | 92 ++++++++++++++++++-
> 4 files changed, 209 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index 8167cc5fb0df..8e1e706c6f5e 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -68,6 +68,9 @@
> #define GVE_FLOW_RULE_IDS_CACHE_SIZE \
> (GVE_ADMINQ_BUFFER_SIZE / sizeof(((struct gve_adminq_queried_flow_rule *)0)->location))
>
> +#define GVE_RSS_KEY_SIZE 40
> +#define GVE_RSS_INDIR_SIZE 128
> +
> #define GVE_XDP_ACTIONS 5
>
> #define GVE_GQ_TX_MIN_PKT_DESC_BYTES 182
> @@ -672,6 +675,7 @@ struct gve_rx_alloc_rings_cfg {
> u16 packet_buffer_size;
> bool raw_addressing;
> bool enable_header_split;
> + bool reset_rss;
>
> /* Allocated resources are returned here */
> struct gve_rx_ring *rx;
> @@ -722,6 +726,11 @@ struct gve_flow_rules_cache {
> u32 rule_ids_cache_num;
> };
>
> +struct gve_rss_config {
> + u8 *hash_key;
> + u32 *hash_lut;
> +};
> +
> struct gve_priv {
> struct net_device *dev;
> struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
> @@ -842,6 +851,8 @@ struct gve_priv {
>
> u16 rss_key_size;
> u16 rss_lut_size;
> + bool cache_rss_config;
> + struct gve_rss_config rss_config;
> };
>
> enum gve_service_task_flags_bit {
> @@ -1210,13 +1221,16 @@ int gve_adjust_config(struct gve_priv *priv,
> struct gve_rx_alloc_rings_cfg *rx_alloc_cfg);
> int gve_adjust_queues(struct gve_priv *priv,
> struct gve_queue_config new_rx_config,
> - struct gve_queue_config new_tx_config);
> + struct gve_queue_config new_tx_config,
> + bool reset_rss);
> /* flow steering rule */
> int gve_get_flow_rule_entry(struct gve_priv *priv, struct ethtool_rxnfc *cmd);
> int gve_get_flow_rule_ids(struct gve_priv *priv, struct ethtool_rxnfc *cmd, u32 *rule_locs);
> int gve_add_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd);
> int gve_del_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd);
> int gve_flow_rules_reset(struct gve_priv *priv);
> +/* RSS config */
> +int gve_init_rss_config(struct gve_priv *priv, u16 num_queues);
> /* report stats handling */
> void gve_handle_report_stats(struct gve_priv *priv);
> /* exported by ethtool.c */
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index aa7d723011d0..a9ae094456cb 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -885,6 +885,15 @@ static void gve_set_default_desc_cnt(struct gve_priv *priv,
> priv->min_rx_desc_cnt = priv->rx_desc_cnt;
> }
>
> +static void gve_set_default_rss_sizes(struct gve_priv *priv)
> +{
> + if (!gve_is_gqi(priv)) {
> + priv->rss_key_size = GVE_RSS_KEY_SIZE;
> + priv->rss_lut_size = GVE_RSS_INDIR_SIZE;
> + priv->cache_rss_config = true;
> + }
> +}
> +
> static void gve_enable_supported_features(struct gve_priv *priv,
> u32 supported_features_mask,
> const struct gve_device_option_jumbo_frames
> @@ -968,6 +977,10 @@ static void gve_enable_supported_features(struct gve_priv *priv,
> be16_to_cpu(dev_op_rss_config->hash_key_size);
> priv->rss_lut_size =
> be16_to_cpu(dev_op_rss_config->hash_lut_size);
> + priv->cache_rss_config = false;
> + dev_dbg(&priv->pdev->dev,
> + "RSS device option enabled with key size of %u, lut size of %u.\n",
> + priv->rss_key_size, priv->rss_lut_size);
> }
> }
>
> @@ -1052,6 +1065,8 @@ int gve_adminq_describe_device(struct gve_priv *priv)
> /* set default descriptor counts */
> gve_set_default_desc_cnt(priv, descriptor);
>
> + gve_set_default_rss_sizes(priv);
> +
> /* DQO supports LRO. */
> if (!gve_is_gqi(priv))
> priv->dev->hw_features |= NETIF_F_LRO;
> @@ -1276,8 +1291,9 @@ int gve_adminq_reset_flow_rules(struct gve_priv *priv)
>
> int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *rxfh)
> {
> + const u32 *hash_lut_to_config = NULL;
> + const u8 *hash_key_to_config = NULL;
> dma_addr_t lut_bus = 0, key_bus = 0;
> - u16 key_size = 0, lut_size = 0;
> union gve_adminq_command cmd;
> __be32 *lut = NULL;
> u8 hash_alg = 0;
> @@ -1287,7 +1303,7 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
>
> switch (rxfh->hfunc) {
> case ETH_RSS_HASH_NO_CHANGE:
> - break;
> + fallthrough;
> case ETH_RSS_HASH_TOP:
> hash_alg = ETH_RSS_HASH_TOP;
> break;
> @@ -1296,27 +1312,44 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
> }
>
> if (rxfh->indir) {
> - lut_size = priv->rss_lut_size;
> + if (rxfh->indir_size != priv->rss_lut_size)
> + return -EINVAL;
> +
> + hash_lut_to_config = rxfh->indir;
> + } else if (priv->cache_rss_config) {
> + hash_lut_to_config = priv->rss_config.hash_lut;
> + }
> +
> + if (hash_lut_to_config) {
> lut = dma_alloc_coherent(&priv->pdev->dev,
> - lut_size * sizeof(*lut),
> + priv->rss_lut_size * sizeof(*lut),
> &lut_bus, GFP_KERNEL);
> if (!lut)
> return -ENOMEM;
>
> for (i = 0; i < priv->rss_lut_size; i++)
> - lut[i] = cpu_to_be32(rxfh->indir[i]);
> + lut[i] = cpu_to_be32(hash_lut_to_config[i]);
> }
>
> if (rxfh->key) {
> - key_size = priv->rss_key_size;
> + if (rxfh->key_size != priv->rss_key_size)
You have to free lut memory in error case
> + return -EINVAL;
> +
> + hash_key_to_config = rxfh->key;
> + } else if (priv->cache_rss_config) {
> + hash_key_to_config = priv->rss_config.hash_key;
> + }
> +
> + if (hash_key_to_config) {
> key = dma_alloc_coherent(&priv->pdev->dev,
> - key_size, &key_bus, GFP_KERNEL);
> + priv->rss_key_size,
> + &key_bus, GFP_KERNEL);
> if (!key) {
> err = -ENOMEM;
> goto out;
> }
>
> - memcpy(key, rxfh->key, key_size);
> + memcpy(key, hash_key_to_config, priv->rss_key_size);
> }
>
> /* Zero-valued fields in the cmd.configure_rss instruct the device to
> @@ -1330,8 +1363,10 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
> BIT(GVE_RSS_HASH_TCPV6) |
> BIT(GVE_RSS_HASH_UDPV6)),
> .hash_alg = hash_alg,
> - .hash_key_size = cpu_to_be16(key_size),
> - .hash_lut_size = cpu_to_be16(lut_size),
> + .hash_key_size =
> + cpu_to_be16((key_bus) ? priv->rss_key_size : 0),
> + .hash_lut_size =
> + cpu_to_be16((lut_bus) ? priv->rss_lut_size : 0),
> .hash_key_addr = cpu_to_be64(key_bus),
> .hash_lut_addr = cpu_to_be64(lut_bus),
> };
> @@ -1341,11 +1376,11 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
> out:
> if (lut)
> dma_free_coherent(&priv->pdev->dev,
> - lut_size * sizeof(*lut),
> + priv->rss_lut_size * sizeof(*lut),
> lut, lut_bus);
> if (key)
> dma_free_coherent(&priv->pdev->dev,
> - key_size, key, key_bus);
> + priv->rss_key_size, key, key_bus);
> return err;
> }
>
> @@ -1449,12 +1484,15 @@ static int gve_adminq_process_rss_query(struct gve_priv *priv,
> rxfh->hfunc = descriptor->hash_alg;
>
> rss_info_addr = (void *)(descriptor + 1);
> - if (rxfh->key)
> + if (rxfh->key) {
> + rxfh->key_size = priv->rss_key_size;
> memcpy(rxfh->key, rss_info_addr, priv->rss_key_size);
> + }
>
> rss_info_addr += priv->rss_key_size;
> lut = (__be32 *)rss_info_addr;
> if (rxfh->indir) {
> + rxfh->indir_size = priv->rss_lut_size;
> for (i = 0; i < priv->rss_lut_size; i++)
> rxfh->indir[i] = be32_to_cpu(lut[i]);
> }
> diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
> index bdfc6e77b2af..efcafc607b2a 100644
> --- a/drivers/net/ethernet/google/gve/gve_ethtool.c
> +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
> @@ -482,6 +482,7 @@ static int gve_set_channels(struct net_device *netdev,
> struct ethtool_channels old_settings;
> int new_tx = cmd->tx_count;
> int new_rx = cmd->rx_count;
> + bool reset_rss;
>
> gve_get_channels(netdev, &old_settings);
>
> @@ -498,16 +499,14 @@ static int gve_set_channels(struct net_device *netdev,
> return -EINVAL;
> }
>
> - if (!netif_running(netdev)) {
> - priv->tx_cfg.num_queues = new_tx;
> - priv->rx_cfg.num_queues = new_rx;
> - return 0;
> - }
> + if (new_rx != priv->rx_cfg.num_queues &&
> + priv->cache_rss_config && !netif_is_rxfh_configured(netdev))
> + reset_rss = true;
>
> new_tx_cfg.num_queues = new_tx;
> new_rx_cfg.num_queues = new_rx;
>
> - return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg);
> + return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg, reset_rss);
> }
>
> static void gve_get_ringparam(struct net_device *netdev,
> @@ -855,6 +854,25 @@ static u32 gve_get_rxfh_indir_size(struct net_device *netdev)
> return priv->rss_lut_size;
> }
>
> +static void gve_get_rss_config_cache(struct gve_priv *priv,
> + struct ethtool_rxfh_param *rxfh)
> +{
> + struct gve_rss_config *rss_config = &priv->rss_config;
> +
> + rxfh->hfunc = ETH_RSS_HASH_TOP;
> +
> + if (rxfh->key) {
> + rxfh->key_size = priv->rss_key_size;
> + memcpy(rxfh->key, rss_config->hash_key, priv->rss_key_size);
> + }
> +
> + if (rxfh->indir) {
> + rxfh->indir_size = priv->rss_lut_size;
> + memcpy(rxfh->indir, rss_config->hash_lut,
> + priv->rss_lut_size * sizeof(*rxfh->indir));
> + }
> +}
> +
> static int gve_get_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh)
> {
> struct gve_priv *priv = netdev_priv(netdev);
> @@ -862,18 +880,46 @@ static int gve_get_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rx
> if (!priv->rss_key_size || !priv->rss_lut_size)
> return -EOPNOTSUPP;
>
> + if (priv->cache_rss_config) {
> + gve_get_rss_config_cache(priv, rxfh);
> + return 0;
> + }
> +
> return gve_adminq_query_rss_config(priv, rxfh);
> }
>
> +static void gve_set_rss_config_cache(struct gve_priv *priv,
> + struct ethtool_rxfh_param *rxfh)
> +{
> + struct gve_rss_config *rss_config = &priv->rss_config;
> +
> + if (rxfh->key)
> + memcpy(rss_config->hash_key, rxfh->key, priv->rss_key_size);
> +
> + if (rxfh->indir)
> + memcpy(rss_config->hash_lut, rxfh->indir,
> + priv->rss_lut_size * sizeof(*rxfh->indir));
> +}
> +
> static int gve_set_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh,
> struct netlink_ext_ack *extack)
> {
> struct gve_priv *priv = netdev_priv(netdev);
> + int err;
>
> if (!priv->rss_key_size || !priv->rss_lut_size)
> return -EOPNOTSUPP;
>
> - return gve_adminq_configure_rss(priv, rxfh);
> + err = gve_adminq_configure_rss(priv, rxfh);
> + if (err) {
> + NL_SET_ERR_MSG_MOD(extack, "Fail to configure RSS config\n");
> + return err;
> + }
> +
> + if (priv->cache_rss_config)
> + gve_set_rss_config_cache(priv, rxfh);
> +
> + return 0;
> }
>
> const struct ethtool_ops gve_ethtool_ops = {
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 533e659b15b3..d726a4d39b45 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -184,6 +184,49 @@ static void gve_free_flow_rule_caches(struct gve_priv *priv)
> flow_rules_cache->rules_cache = NULL;
> }
>
> +static int gve_alloc_rss_config_cache(struct gve_priv *priv)
> +{
> + struct gve_rss_config *rss_config = &priv->rss_config;
> + int err = 0;
> +
> + if (!priv->cache_rss_config)
> + return 0;
> +
> + rss_config->hash_key = kcalloc(priv->rss_key_size,
> + sizeof(rss_config->hash_key[0]),
> + GFP_KERNEL);
> + if (!rss_config->hash_key) {
> + dev_err(&priv->pdev->dev, "Cannot alloc rss key cache\n");
There is no need of dev_err message in memory allocation failure path.
> + return -ENOMEM;
> + }
> +
> + rss_config->hash_lut = kcalloc(priv->rss_lut_size,
> + sizeof(rss_config->hash_lut[0]),
> + GFP_KERNEL);
> + if (!rss_config->hash_lut) {
> + dev_err(&priv->pdev->dev, "Cannot alloc rss lut cache\n");
> + err = -ENOMEM;
> + goto free_rss_key_cache;
> + }
> +
> + return 0;
> +
> +free_rss_key_cache:
> + kfree(rss_config->hash_key);
> + rss_config->hash_key = NULL;
> + return err;
Maybe you can return -ENOMEM here and hence avoid the need of the
local variable err.
> +}
> +
> +static void gve_free_rss_config_cache(struct gve_priv *priv)
> +{
> + struct gve_rss_config *rss_config = &priv->rss_config;
> +
> + kfree(rss_config->hash_key);
> + kfree(rss_config->hash_lut);
> +
> + memset(rss_config, 0, sizeof(*rss_config));
> +}
> +
> static int gve_alloc_counter_array(struct gve_priv *priv)
> {
> priv->counter_array =
> @@ -575,9 +618,12 @@ static int gve_setup_device_resources(struct gve_priv *priv)
> err = gve_alloc_flow_rule_caches(priv);
> if (err)
> return err;
> - err = gve_alloc_counter_array(priv);
> + err = gve_alloc_rss_config_cache(priv);
> if (err)
> goto abort_with_flow_rule_caches;
> + err = gve_alloc_counter_array(priv);
> + if (err)
> + goto abort_with_rss_config_cache;
> err = gve_alloc_notify_blocks(priv);
> if (err)
> goto abort_with_counter;
> @@ -611,6 +657,12 @@ static int gve_setup_device_resources(struct gve_priv *priv)
> }
> }
>
> + err = gve_init_rss_config(priv, priv->rx_cfg.num_queues);
> + if (err) {
> + dev_err(&priv->pdev->dev, "Failed to init RSS config");
> + goto abort_with_ptype_lut;
> + }
> +
> err = gve_adminq_report_stats(priv, priv->stats_report_len,
> priv->stats_report_bus,
> GVE_STATS_REPORT_TIMER_PERIOD);
> @@ -629,6 +681,8 @@ static int gve_setup_device_resources(struct gve_priv *priv)
> gve_free_notify_blocks(priv);
> abort_with_counter:
> gve_free_counter_array(priv);
> +abort_with_rss_config_cache:
> + gve_free_rss_config_cache(priv);
> abort_with_flow_rule_caches:
> gve_free_flow_rule_caches(priv);
>
> @@ -669,6 +723,7 @@ static void gve_teardown_device_resources(struct gve_priv *priv)
> priv->ptype_lut_dqo = NULL;
>
> gve_free_flow_rule_caches(priv);
> + gve_free_rss_config_cache(priv);
> gve_free_counter_array(priv);
> gve_free_notify_blocks(priv);
> gve_free_stats_report(priv);
> @@ -1390,6 +1445,12 @@ static int gve_queues_start(struct gve_priv *priv,
> if (err)
> goto stop_and_free_rings;
>
> + if (rx_alloc_cfg->reset_rss) {
> + err = gve_init_rss_config(priv, priv->rx_cfg.num_queues);
> + if (err)
> + goto reset;
> + }
> +
> err = gve_register_qpls(priv);
> if (err)
> goto reset;
> @@ -1786,6 +1847,26 @@ static int gve_xdp(struct net_device *dev, struct netdev_bpf *xdp)
> }
> }
>
> +int gve_init_rss_config(struct gve_priv *priv, u16 num_queues)
> +{
> + struct gve_rss_config *rss_config = &priv->rss_config;
> + struct ethtool_rxfh_param rxfh;
> + u16 i;
> +
> + if (!priv->cache_rss_config)
> + return 0;
> +
> + for (i = 0; i < priv->rss_lut_size; i++)
> + rss_config->hash_lut[i] =
> + ethtool_rxfh_indir_default(i, num_queues);
> +
> + netdev_rss_key_fill(rss_config->hash_key, priv->rss_key_size);
> +
> + rxfh.hfunc = ETH_RSS_HASH_TOP;
> +
> + return gve_adminq_configure_rss(priv, &rxfh);
> +}
> +
> int gve_flow_rules_reset(struct gve_priv *priv)
> {
> if (!priv->max_flow_rules)
> @@ -1834,7 +1915,8 @@ int gve_adjust_config(struct gve_priv *priv,
>
> int gve_adjust_queues(struct gve_priv *priv,
> struct gve_queue_config new_rx_config,
> - struct gve_queue_config new_tx_config)
> + struct gve_queue_config new_tx_config,
> + bool reset_rss)
> {
> struct gve_tx_alloc_rings_cfg tx_alloc_cfg = {0};
> struct gve_rx_alloc_rings_cfg rx_alloc_cfg = {0};
> @@ -1847,6 +1929,7 @@ int gve_adjust_queues(struct gve_priv *priv,
> tx_alloc_cfg.qcfg = &new_tx_config;
> rx_alloc_cfg.qcfg_tx = &new_tx_config;
> rx_alloc_cfg.qcfg = &new_rx_config;
> + rx_alloc_cfg.reset_rss = reset_rss;
> tx_alloc_cfg.num_rings = new_tx_config.num_queues;
>
> /* Add dedicated XDP TX queues if enabled. */
> @@ -1858,6 +1941,11 @@ int gve_adjust_queues(struct gve_priv *priv,
> return err;
> }
> /* Set the config for the next up. */
> + if (reset_rss) {
> + err = gve_init_rss_config(priv, new_rx_config.num_queues);
> + if (err)
> + return err;
> + }
> priv->tx_cfg = new_tx_config;
> priv->rx_cfg = new_rx_config;
>
> --
> 2.48.1.362.g079036d154-goog
>
>
--
Regards,
Kalesh AP
On Wed, Feb 5, 2025 at 8:52 AM Kalesh Anakkur Purayil
<kalesh-anakkur.purayil@broadcom.com> wrote:
>
> Hi Jerome,
>
> On Wed, Feb 5, 2025 at 3:02 AM Jeroen de Borst <jeroendb@google.com> wrote:
> >
> > From: Ziwei Xiao <ziweixiao@google.com>
> >
> > Not all the devices have the capability for the driver to query for the
> > registered RSS configuration. The driver can discover this by checking
> > the relevant device option during setup. If it cannot, the driver needs
> > to store the RSS config cache and directly return such cache when
> > queried by the ethtool. RSS config is inited when driver probes. Also the
> > default RSS config will be adjusted when there is RX queue count change.
> >
> > At this point, only keys of GVE_RSS_KEY_SIZE and indirection tables of
> > GVE_RSS_INDIR_SIZE are supported.
> >
> > Signed-off-by: Ziwei Xiao <ziweixiao@google.com>
> > Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
> > Reviewed-by: Willem de Bruijn <willemb@google.com>
> > Signed-off-by: Praveen Kaligineedi <pkaligineedi@google.com>
> > Signed-off-by: Jeroen de Borst <jeroend@google.com>
> > ---
> > Changes in v2:
> > - Change to initialize RSS config when the driver is up instead of
> > doing that when the user setting the RSS.(Jakub Kicinski)
> > - Use NL_SET_ERR_MSG_MOD to log errors when there is extack
> > available.(Jakub Kicinski)
> > - Use ethtool_rxfh_indir_default to set default RSS indir
> > table.(Jakub Kicinski)
> > - Adjust the default RSS config when there is RX queue count change to
> > ensure the default RSS config is correct.
> >
> > drivers/net/ethernet/google/gve/gve.h | 16 +++-
> > drivers/net/ethernet/google/gve/gve_adminq.c | 64 ++++++++++---
> > drivers/net/ethernet/google/gve/gve_ethtool.c | 60 ++++++++++--
> > drivers/net/ethernet/google/gve/gve_main.c | 92 ++++++++++++++++++-
> > 4 files changed, 209 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> > index 8167cc5fb0df..8e1e706c6f5e 100644
> > --- a/drivers/net/ethernet/google/gve/gve.h
> > +++ b/drivers/net/ethernet/google/gve/gve.h
> > @@ -68,6 +68,9 @@
> > #define GVE_FLOW_RULE_IDS_CACHE_SIZE \
> > (GVE_ADMINQ_BUFFER_SIZE / sizeof(((struct gve_adminq_queried_flow_rule *)0)->location))
> >
> > +#define GVE_RSS_KEY_SIZE 40
> > +#define GVE_RSS_INDIR_SIZE 128
> > +
> > #define GVE_XDP_ACTIONS 5
> >
> > #define GVE_GQ_TX_MIN_PKT_DESC_BYTES 182
> > @@ -672,6 +675,7 @@ struct gve_rx_alloc_rings_cfg {
> > u16 packet_buffer_size;
> > bool raw_addressing;
> > bool enable_header_split;
> > + bool reset_rss;
> >
> > /* Allocated resources are returned here */
> > struct gve_rx_ring *rx;
> > @@ -722,6 +726,11 @@ struct gve_flow_rules_cache {
> > u32 rule_ids_cache_num;
> > };
> >
> > +struct gve_rss_config {
> > + u8 *hash_key;
> > + u32 *hash_lut;
> > +};
> > +
> > struct gve_priv {
> > struct net_device *dev;
> > struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
> > @@ -842,6 +851,8 @@ struct gve_priv {
> >
> > u16 rss_key_size;
> > u16 rss_lut_size;
> > + bool cache_rss_config;
> > + struct gve_rss_config rss_config;
> > };
> >
> > enum gve_service_task_flags_bit {
> > @@ -1210,13 +1221,16 @@ int gve_adjust_config(struct gve_priv *priv,
> > struct gve_rx_alloc_rings_cfg *rx_alloc_cfg);
> > int gve_adjust_queues(struct gve_priv *priv,
> > struct gve_queue_config new_rx_config,
> > - struct gve_queue_config new_tx_config);
> > + struct gve_queue_config new_tx_config,
> > + bool reset_rss);
> > /* flow steering rule */
> > int gve_get_flow_rule_entry(struct gve_priv *priv, struct ethtool_rxnfc *cmd);
> > int gve_get_flow_rule_ids(struct gve_priv *priv, struct ethtool_rxnfc *cmd, u32 *rule_locs);
> > int gve_add_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd);
> > int gve_del_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd);
> > int gve_flow_rules_reset(struct gve_priv *priv);
> > +/* RSS config */
> > +int gve_init_rss_config(struct gve_priv *priv, u16 num_queues);
> > /* report stats handling */
> > void gve_handle_report_stats(struct gve_priv *priv);
> > /* exported by ethtool.c */
> > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> > index aa7d723011d0..a9ae094456cb 100644
> > --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> > @@ -885,6 +885,15 @@ static void gve_set_default_desc_cnt(struct gve_priv *priv,
> > priv->min_rx_desc_cnt = priv->rx_desc_cnt;
> > }
> >
> > +static void gve_set_default_rss_sizes(struct gve_priv *priv)
> > +{
> > + if (!gve_is_gqi(priv)) {
> > + priv->rss_key_size = GVE_RSS_KEY_SIZE;
> > + priv->rss_lut_size = GVE_RSS_INDIR_SIZE;
> > + priv->cache_rss_config = true;
> > + }
> > +}
> > +
> > static void gve_enable_supported_features(struct gve_priv *priv,
> > u32 supported_features_mask,
> > const struct gve_device_option_jumbo_frames
> > @@ -968,6 +977,10 @@ static void gve_enable_supported_features(struct gve_priv *priv,
> > be16_to_cpu(dev_op_rss_config->hash_key_size);
> > priv->rss_lut_size =
> > be16_to_cpu(dev_op_rss_config->hash_lut_size);
> > + priv->cache_rss_config = false;
> > + dev_dbg(&priv->pdev->dev,
> > + "RSS device option enabled with key size of %u, lut size of %u.\n",
> > + priv->rss_key_size, priv->rss_lut_size);
> > }
> > }
> >
> > @@ -1052,6 +1065,8 @@ int gve_adminq_describe_device(struct gve_priv *priv)
> > /* set default descriptor counts */
> > gve_set_default_desc_cnt(priv, descriptor);
> >
> > + gve_set_default_rss_sizes(priv);
> > +
> > /* DQO supports LRO. */
> > if (!gve_is_gqi(priv))
> > priv->dev->hw_features |= NETIF_F_LRO;
> > @@ -1276,8 +1291,9 @@ int gve_adminq_reset_flow_rules(struct gve_priv *priv)
> >
> > int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *rxfh)
> > {
> > + const u32 *hash_lut_to_config = NULL;
> > + const u8 *hash_key_to_config = NULL;
> > dma_addr_t lut_bus = 0, key_bus = 0;
> > - u16 key_size = 0, lut_size = 0;
> > union gve_adminq_command cmd;
> > __be32 *lut = NULL;
> > u8 hash_alg = 0;
> > @@ -1287,7 +1303,7 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
> >
> > switch (rxfh->hfunc) {
> > case ETH_RSS_HASH_NO_CHANGE:
> > - break;
> > + fallthrough;
> > case ETH_RSS_HASH_TOP:
> > hash_alg = ETH_RSS_HASH_TOP;
> > break;
> > @@ -1296,27 +1312,44 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
> > }
> >
> > if (rxfh->indir) {
> > - lut_size = priv->rss_lut_size;
> > + if (rxfh->indir_size != priv->rss_lut_size)
> > + return -EINVAL;
> > +
> > + hash_lut_to_config = rxfh->indir;
> > + } else if (priv->cache_rss_config) {
> > + hash_lut_to_config = priv->rss_config.hash_lut;
> > + }
> > +
> > + if (hash_lut_to_config) {
> > lut = dma_alloc_coherent(&priv->pdev->dev,
> > - lut_size * sizeof(*lut),
> > + priv->rss_lut_size * sizeof(*lut),
> > &lut_bus, GFP_KERNEL);
> > if (!lut)
> > return -ENOMEM;
> >
> > for (i = 0; i < priv->rss_lut_size; i++)
> > - lut[i] = cpu_to_be32(rxfh->indir[i]);
> > + lut[i] = cpu_to_be32(hash_lut_to_config[i]);
> > }
> >
> > if (rxfh->key) {
> > - key_size = priv->rss_key_size;
> > + if (rxfh->key_size != priv->rss_key_size)
> You have to free lut memory in error case
Thanks for catching that. Will fix it on the V3 patch.
> > + return -EINVAL;
> > +
> > + hash_key_to_config = rxfh->key;
> > + } else if (priv->cache_rss_config) {
> > + hash_key_to_config = priv->rss_config.hash_key;
> > + }
> > +
> > + if (hash_key_to_config) {
> > key = dma_alloc_coherent(&priv->pdev->dev,
> > - key_size, &key_bus, GFP_KERNEL);
> > + priv->rss_key_size,
> > + &key_bus, GFP_KERNEL);
> > if (!key) {
> > err = -ENOMEM;
> > goto out;
> > }
> >
> > - memcpy(key, rxfh->key, key_size);
> > + memcpy(key, hash_key_to_config, priv->rss_key_size);
> > }
> >
> > /* Zero-valued fields in the cmd.configure_rss instruct the device to
> > @@ -1330,8 +1363,10 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
> > BIT(GVE_RSS_HASH_TCPV6) |
> > BIT(GVE_RSS_HASH_UDPV6)),
> > .hash_alg = hash_alg,
> > - .hash_key_size = cpu_to_be16(key_size),
> > - .hash_lut_size = cpu_to_be16(lut_size),
> > + .hash_key_size =
> > + cpu_to_be16((key_bus) ? priv->rss_key_size : 0),
> > + .hash_lut_size =
> > + cpu_to_be16((lut_bus) ? priv->rss_lut_size : 0),
> > .hash_key_addr = cpu_to_be64(key_bus),
> > .hash_lut_addr = cpu_to_be64(lut_bus),
> > };
> > @@ -1341,11 +1376,11 @@ int gve_adminq_configure_rss(struct gve_priv *priv, struct ethtool_rxfh_param *r
> > out:
> > if (lut)
> > dma_free_coherent(&priv->pdev->dev,
> > - lut_size * sizeof(*lut),
> > + priv->rss_lut_size * sizeof(*lut),
> > lut, lut_bus);
> > if (key)
> > dma_free_coherent(&priv->pdev->dev,
> > - key_size, key, key_bus);
> > + priv->rss_key_size, key, key_bus);
> > return err;
> > }
> >
> > @@ -1449,12 +1484,15 @@ static int gve_adminq_process_rss_query(struct gve_priv *priv,
> > rxfh->hfunc = descriptor->hash_alg;
> >
> > rss_info_addr = (void *)(descriptor + 1);
> > - if (rxfh->key)
> > + if (rxfh->key) {
> > + rxfh->key_size = priv->rss_key_size;
> > memcpy(rxfh->key, rss_info_addr, priv->rss_key_size);
> > + }
> >
> > rss_info_addr += priv->rss_key_size;
> > lut = (__be32 *)rss_info_addr;
> > if (rxfh->indir) {
> > + rxfh->indir_size = priv->rss_lut_size;
> > for (i = 0; i < priv->rss_lut_size; i++)
> > rxfh->indir[i] = be32_to_cpu(lut[i]);
> > }
> > diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
> > index bdfc6e77b2af..efcafc607b2a 100644
> > --- a/drivers/net/ethernet/google/gve/gve_ethtool.c
> > +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
> > @@ -482,6 +482,7 @@ static int gve_set_channels(struct net_device *netdev,
> > struct ethtool_channels old_settings;
> > int new_tx = cmd->tx_count;
> > int new_rx = cmd->rx_count;
> > + bool reset_rss;
> >
> > gve_get_channels(netdev, &old_settings);
> >
> > @@ -498,16 +499,14 @@ static int gve_set_channels(struct net_device *netdev,
> > return -EINVAL;
> > }
> >
> > - if (!netif_running(netdev)) {
> > - priv->tx_cfg.num_queues = new_tx;
> > - priv->rx_cfg.num_queues = new_rx;
> > - return 0;
> > - }
> > + if (new_rx != priv->rx_cfg.num_queues &&
> > + priv->cache_rss_config && !netif_is_rxfh_configured(netdev))
> > + reset_rss = true;
> >
> > new_tx_cfg.num_queues = new_tx;
> > new_rx_cfg.num_queues = new_rx;
> >
> > - return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg);
> > + return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg, reset_rss);
> > }
> >
> > static void gve_get_ringparam(struct net_device *netdev,
> > @@ -855,6 +854,25 @@ static u32 gve_get_rxfh_indir_size(struct net_device *netdev)
> > return priv->rss_lut_size;
> > }
> >
> > +static void gve_get_rss_config_cache(struct gve_priv *priv,
> > + struct ethtool_rxfh_param *rxfh)
> > +{
> > + struct gve_rss_config *rss_config = &priv->rss_config;
> > +
> > + rxfh->hfunc = ETH_RSS_HASH_TOP;
> > +
> > + if (rxfh->key) {
> > + rxfh->key_size = priv->rss_key_size;
> > + memcpy(rxfh->key, rss_config->hash_key, priv->rss_key_size);
> > + }
> > +
> > + if (rxfh->indir) {
> > + rxfh->indir_size = priv->rss_lut_size;
> > + memcpy(rxfh->indir, rss_config->hash_lut,
> > + priv->rss_lut_size * sizeof(*rxfh->indir));
> > + }
> > +}
> > +
> > static int gve_get_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh)
> > {
> > struct gve_priv *priv = netdev_priv(netdev);
> > @@ -862,18 +880,46 @@ static int gve_get_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rx
> > if (!priv->rss_key_size || !priv->rss_lut_size)
> > return -EOPNOTSUPP;
> >
> > + if (priv->cache_rss_config) {
> > + gve_get_rss_config_cache(priv, rxfh);
> > + return 0;
> > + }
> > +
> > return gve_adminq_query_rss_config(priv, rxfh);
> > }
> >
> > +static void gve_set_rss_config_cache(struct gve_priv *priv,
> > + struct ethtool_rxfh_param *rxfh)
> > +{
> > + struct gve_rss_config *rss_config = &priv->rss_config;
> > +
> > + if (rxfh->key)
> > + memcpy(rss_config->hash_key, rxfh->key, priv->rss_key_size);
> > +
> > + if (rxfh->indir)
> > + memcpy(rss_config->hash_lut, rxfh->indir,
> > + priv->rss_lut_size * sizeof(*rxfh->indir));
> > +}
> > +
> > static int gve_set_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh,
> > struct netlink_ext_ack *extack)
> > {
> > struct gve_priv *priv = netdev_priv(netdev);
> > + int err;
> >
> > if (!priv->rss_key_size || !priv->rss_lut_size)
> > return -EOPNOTSUPP;
> >
> > - return gve_adminq_configure_rss(priv, rxfh);
> > + err = gve_adminq_configure_rss(priv, rxfh);
> > + if (err) {
> > + NL_SET_ERR_MSG_MOD(extack, "Fail to configure RSS config\n");
> > + return err;
> > + }
> > +
> > + if (priv->cache_rss_config)
> > + gve_set_rss_config_cache(priv, rxfh);
> > +
> > + return 0;
> > }
> >
> > const struct ethtool_ops gve_ethtool_ops = {
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index 533e659b15b3..d726a4d39b45 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
> > @@ -184,6 +184,49 @@ static void gve_free_flow_rule_caches(struct gve_priv *priv)
> > flow_rules_cache->rules_cache = NULL;
> > }
> >
> > +static int gve_alloc_rss_config_cache(struct gve_priv *priv)
> > +{
> > + struct gve_rss_config *rss_config = &priv->rss_config;
> > + int err = 0;
> > +
> > + if (!priv->cache_rss_config)
> > + return 0;
> > +
> > + rss_config->hash_key = kcalloc(priv->rss_key_size,
> > + sizeof(rss_config->hash_key[0]),
> > + GFP_KERNEL);
> > + if (!rss_config->hash_key) {
> > + dev_err(&priv->pdev->dev, "Cannot alloc rss key cache\n");
> There is no need of dev_err message in memory allocation failure path.
Thanks for the advice, will consider that.
> > + return -ENOMEM;
> > + }
> > +
> > + rss_config->hash_lut = kcalloc(priv->rss_lut_size,
> > + sizeof(rss_config->hash_lut[0]),
> > + GFP_KERNEL);
> > + if (!rss_config->hash_lut) {
> > + dev_err(&priv->pdev->dev, "Cannot alloc rss lut cache\n");
> > + err = -ENOMEM;
> > + goto free_rss_key_cache;
> > + }
> > +
> > + return 0;
> > +
> > +free_rss_key_cache:
> > + kfree(rss_config->hash_key);
> > + rss_config->hash_key = NULL;
> > + return err;
> Maybe you can return -ENOMEM here and hence avoid the need of the
> local variable err.
Thank you, will change that in the V3 patch.
> > +}
> > +
> > +static void gve_free_rss_config_cache(struct gve_priv *priv)
> > +{
> > + struct gve_rss_config *rss_config = &priv->rss_config;
> > +
> > + kfree(rss_config->hash_key);
> > + kfree(rss_config->hash_lut);
> > +
> > + memset(rss_config, 0, sizeof(*rss_config));
> > +}
> > +
> > static int gve_alloc_counter_array(struct gve_priv *priv)
> > {
> > priv->counter_array =
> > @@ -575,9 +618,12 @@ static int gve_setup_device_resources(struct gve_priv *priv)
> > err = gve_alloc_flow_rule_caches(priv);
> > if (err)
> > return err;
> > - err = gve_alloc_counter_array(priv);
> > + err = gve_alloc_rss_config_cache(priv);
> > if (err)
> > goto abort_with_flow_rule_caches;
> > + err = gve_alloc_counter_array(priv);
> > + if (err)
> > + goto abort_with_rss_config_cache;
> > err = gve_alloc_notify_blocks(priv);
> > if (err)
> > goto abort_with_counter;
> > @@ -611,6 +657,12 @@ static int gve_setup_device_resources(struct gve_priv *priv)
> > }
> > }
> >
> > + err = gve_init_rss_config(priv, priv->rx_cfg.num_queues);
> > + if (err) {
> > + dev_err(&priv->pdev->dev, "Failed to init RSS config");
> > + goto abort_with_ptype_lut;
> > + }
> > +
> > err = gve_adminq_report_stats(priv, priv->stats_report_len,
> > priv->stats_report_bus,
> > GVE_STATS_REPORT_TIMER_PERIOD);
> > @@ -629,6 +681,8 @@ static int gve_setup_device_resources(struct gve_priv *priv)
> > gve_free_notify_blocks(priv);
> > abort_with_counter:
> > gve_free_counter_array(priv);
> > +abort_with_rss_config_cache:
> > + gve_free_rss_config_cache(priv);
> > abort_with_flow_rule_caches:
> > gve_free_flow_rule_caches(priv);
> >
> > @@ -669,6 +723,7 @@ static void gve_teardown_device_resources(struct gve_priv *priv)
> > priv->ptype_lut_dqo = NULL;
> >
> > gve_free_flow_rule_caches(priv);
> > + gve_free_rss_config_cache(priv);
> > gve_free_counter_array(priv);
> > gve_free_notify_blocks(priv);
> > gve_free_stats_report(priv);
> > @@ -1390,6 +1445,12 @@ static int gve_queues_start(struct gve_priv *priv,
> > if (err)
> > goto stop_and_free_rings;
> >
> > + if (rx_alloc_cfg->reset_rss) {
> > + err = gve_init_rss_config(priv, priv->rx_cfg.num_queues);
> > + if (err)
> > + goto reset;
> > + }
> > +
> > err = gve_register_qpls(priv);
> > if (err)
> > goto reset;
> > @@ -1786,6 +1847,26 @@ static int gve_xdp(struct net_device *dev, struct netdev_bpf *xdp)
> > }
> > }
> >
> > +int gve_init_rss_config(struct gve_priv *priv, u16 num_queues)
> > +{
> > + struct gve_rss_config *rss_config = &priv->rss_config;
> > + struct ethtool_rxfh_param rxfh;
> > + u16 i;
> > +
> > + if (!priv->cache_rss_config)
> > + return 0;
> > +
> > + for (i = 0; i < priv->rss_lut_size; i++)
> > + rss_config->hash_lut[i] =
> > + ethtool_rxfh_indir_default(i, num_queues);
> > +
> > + netdev_rss_key_fill(rss_config->hash_key, priv->rss_key_size);
> > +
> > + rxfh.hfunc = ETH_RSS_HASH_TOP;
> > +
> > + return gve_adminq_configure_rss(priv, &rxfh);
> > +}
> > +
> > int gve_flow_rules_reset(struct gve_priv *priv)
> > {
> > if (!priv->max_flow_rules)
> > @@ -1834,7 +1915,8 @@ int gve_adjust_config(struct gve_priv *priv,
> >
> > int gve_adjust_queues(struct gve_priv *priv,
> > struct gve_queue_config new_rx_config,
> > - struct gve_queue_config new_tx_config)
> > + struct gve_queue_config new_tx_config,
> > + bool reset_rss)
> > {
> > struct gve_tx_alloc_rings_cfg tx_alloc_cfg = {0};
> > struct gve_rx_alloc_rings_cfg rx_alloc_cfg = {0};
> > @@ -1847,6 +1929,7 @@ int gve_adjust_queues(struct gve_priv *priv,
> > tx_alloc_cfg.qcfg = &new_tx_config;
> > rx_alloc_cfg.qcfg_tx = &new_tx_config;
> > rx_alloc_cfg.qcfg = &new_rx_config;
> > + rx_alloc_cfg.reset_rss = reset_rss;
> > tx_alloc_cfg.num_rings = new_tx_config.num_queues;
> >
> > /* Add dedicated XDP TX queues if enabled. */
> > @@ -1858,6 +1941,11 @@ int gve_adjust_queues(struct gve_priv *priv,
> > return err;
> > }
> > /* Set the config for the next up. */
> > + if (reset_rss) {
> > + err = gve_init_rss_config(priv, new_rx_config.num_queues);
> > + if (err)
> > + return err;
> > + }
> > priv->tx_cfg = new_tx_config;
> > priv->rx_cfg = new_rx_config;
> >
> > --
> > 2.48.1.362.g079036d154-goog
> >
> >
>
>
> --
> Regards,
> Kalesh AP
On Tue, Feb 04, 2025 at 01:31:21PM -0800, Jeroen de Borst wrote:
> From: Ziwei Xiao <ziweixiao@google.com>
>
> Not all the devices have the capability for the driver to query for the
> registered RSS configuration. The driver can discover this by checking
> the relevant device option during setup. If it cannot, the driver needs
> to store the RSS config cache and directly return such cache when
> queried by the ethtool. RSS config is inited when driver probes. Also the
> default RSS config will be adjusted when there is RX queue count change.
>
> At this point, only keys of GVE_RSS_KEY_SIZE and indirection tables of
> GVE_RSS_INDIR_SIZE are supported.
>
> Signed-off-by: Ziwei Xiao <ziweixiao@google.com>
> Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Praveen Kaligineedi <pkaligineedi@google.com>
> Signed-off-by: Jeroen de Borst <jeroend@google.com>
> ---
> Changes in v2:
> - Change to initialize RSS config when the driver is up instead of
> doing that when the user setting the RSS.(Jakub Kicinski)
> - Use NL_SET_ERR_MSG_MOD to log errors when there is extack
> available.(Jakub Kicinski)
> - Use ethtool_rxfh_indir_default to set default RSS indir
> table.(Jakub Kicinski)
> - Adjust the default RSS config when there is RX queue count change to
> ensure the default RSS config is correct.
>
> drivers/net/ethernet/google/gve/gve.h | 16 +++-
> drivers/net/ethernet/google/gve/gve_adminq.c | 64 ++++++++++---
> drivers/net/ethernet/google/gve/gve_ethtool.c | 60 ++++++++++--
> drivers/net/ethernet/google/gve/gve_main.c | 92 ++++++++++++++++++-
> 4 files changed, 209 insertions(+), 23 deletions(-)
...
> diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
> index bdfc6e77b2af..efcafc607b2a 100644
> --- a/drivers/net/ethernet/google/gve/gve_ethtool.c
> +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
> @@ -482,6 +482,7 @@ static int gve_set_channels(struct net_device *netdev,
> struct ethtool_channels old_settings;
> int new_tx = cmd->tx_count;
> int new_rx = cmd->rx_count;
> + bool reset_rss;
>
> gve_get_channels(netdev, &old_settings);
>
> @@ -498,16 +499,14 @@ static int gve_set_channels(struct net_device *netdev,
> return -EINVAL;
> }
>
> - if (!netif_running(netdev)) {
> - priv->tx_cfg.num_queues = new_tx;
> - priv->rx_cfg.num_queues = new_rx;
> - return 0;
> - }
> + if (new_rx != priv->rx_cfg.num_queues &&
> + priv->cache_rss_config && !netif_is_rxfh_configured(netdev))
> + reset_rss = true;
Hi Jerome,
This is not a full review (which I was working on but ran out of time for now).
But, if the condition above is not met then reset_rss will be used
uninitialised below.
Flagged by W=1 build with clang-19.
>
> new_tx_cfg.num_queues = new_tx;
> new_rx_cfg.num_queues = new_rx;
>
> - return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg);
> + return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg, reset_rss);
> }
>
> static void gve_get_ringparam(struct net_device *netdev,
...
--
pw-bot: cr
On Wed, Feb 5, 2025 at 8:28 AM Simon Horman <horms@kernel.org> wrote:
>
> On Tue, Feb 04, 2025 at 01:31:21PM -0800, Jeroen de Borst wrote:
> > From: Ziwei Xiao <ziweixiao@google.com>
> >
> > Not all the devices have the capability for the driver to query for the
> > registered RSS configuration. The driver can discover this by checking
> > the relevant device option during setup. If it cannot, the driver needs
> > to store the RSS config cache and directly return such cache when
> > queried by the ethtool. RSS config is inited when driver probes. Also the
> > default RSS config will be adjusted when there is RX queue count change.
> >
> > At this point, only keys of GVE_RSS_KEY_SIZE and indirection tables of
> > GVE_RSS_INDIR_SIZE are supported.
> >
> > Signed-off-by: Ziwei Xiao <ziweixiao@google.com>
> > Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
> > Reviewed-by: Willem de Bruijn <willemb@google.com>
> > Signed-off-by: Praveen Kaligineedi <pkaligineedi@google.com>
> > Signed-off-by: Jeroen de Borst <jeroend@google.com>
> > ---
> > Changes in v2:
> > - Change to initialize RSS config when the driver is up instead of
> > doing that when the user setting the RSS.(Jakub Kicinski)
> > - Use NL_SET_ERR_MSG_MOD to log errors when there is extack
> > available.(Jakub Kicinski)
> > - Use ethtool_rxfh_indir_default to set default RSS indir
> > table.(Jakub Kicinski)
> > - Adjust the default RSS config when there is RX queue count change to
> > ensure the default RSS config is correct.
> >
> > drivers/net/ethernet/google/gve/gve.h | 16 +++-
> > drivers/net/ethernet/google/gve/gve_adminq.c | 64 ++++++++++---
> > drivers/net/ethernet/google/gve/gve_ethtool.c | 60 ++++++++++--
> > drivers/net/ethernet/google/gve/gve_main.c | 92 ++++++++++++++++++-
> > 4 files changed, 209 insertions(+), 23 deletions(-)
>
> ...
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
> > index bdfc6e77b2af..efcafc607b2a 100644
> > --- a/drivers/net/ethernet/google/gve/gve_ethtool.c
> > +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
> > @@ -482,6 +482,7 @@ static int gve_set_channels(struct net_device *netdev,
> > struct ethtool_channels old_settings;
> > int new_tx = cmd->tx_count;
> > int new_rx = cmd->rx_count;
> > + bool reset_rss;
> >
> > gve_get_channels(netdev, &old_settings);
> >
> > @@ -498,16 +499,14 @@ static int gve_set_channels(struct net_device *netdev,
> > return -EINVAL;
> > }
> >
> > - if (!netif_running(netdev)) {
> > - priv->tx_cfg.num_queues = new_tx;
> > - priv->rx_cfg.num_queues = new_rx;
> > - return 0;
> > - }
> > + if (new_rx != priv->rx_cfg.num_queues &&
> > + priv->cache_rss_config && !netif_is_rxfh_configured(netdev))
> > + reset_rss = true;
>
> Hi Jerome,
>
> This is not a full review (which I was working on but ran out of time for now).
> But, if the condition above is not met then reset_rss will be used
> uninitialised below.
>
> Flagged by W=1 build with clang-19.
>
Thank you for pointing that out. Will fix it on the V3 patch.
> >
> > new_tx_cfg.num_queues = new_tx;
> > new_rx_cfg.num_queues = new_rx;
> >
> > - return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg);
> > + return gve_adjust_queues(priv, new_rx_cfg, new_tx_cfg, reset_rss);
> > }
> >
> > static void gve_get_ringparam(struct net_device *netdev,
>
> ...
>
> --
> pw-bot: cr
© 2016 - 2025 Red Hat, Inc.