Re: [PATCH net-next] gve: Add RSS cache for non RSS device option scenario

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 13 Jan 2025 13:11:39 -0800 Praveen Kaligineedi wrote:
>  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);
> +	if (priv->cache_rss_config && !priv->rss_cache_configured) {
> +		err = gve_init_rss_config(priv);

I don't understand why this is here.
Why are you programming the default config the first time user asks 
to set rxfh? And just to immediately overwrite it.

Shouldn't you be doing this on probe or first open?

Oh and in gve_setup_device_resources() you call gve_init_rss_config()

> +	if (priv->rss_cache_configured) {

so you reset what user wanted to defaults, only if user wanted
something explicitly _not_ default. Hm.

> +		if (err) {
> +			dev_err(&priv->pdev->dev, "Fail to init RSS config\n");

use extack, please

> +			return err;
> +		}
> +	}

> +int gve_init_rss_config(struct gve_priv *priv)
> +{
> +	struct gve_rss_config *rss_config = &priv->rss_config;
> +	struct ethtool_rxfh_param rxfh;
> +	u16 i;
> +
> +	for (i = 0; i < priv->rss_lut_size; i++)
> +		rss_config->hash_lut[i] = i % priv->rx_cfg.num_queues;

nit: ethtool_rxfh_indir_default()

> +
> +	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);
> +}
-- 
pw-bot: cr