Re: [PATCH net-next 2/2] gve: Add RSS adminq commands and ethtool support

[Jakub Kicinski <kuba@kernel.org>]

On Thu,  1 Aug 2024 18:28:34 -0700 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);
> +	struct gve_rss_config rss_config = {0};

I never remember the exact rules, are you sure this is the one that 
is guaranteed per standard to zero all the fields?

> +	u32 *indir = rxfh->indir;
> +	u8 hfunc = rxfh->hfunc;
> +	u8 *key = rxfh->key;
> +	int err = 0;
> +
> +	if (!priv->rss_key_size || !priv->rss_lut_size)
> +		return -EOPNOTSUPP;
> +
> +	switch (hfunc) {
> +	case ETH_RSS_HASH_NO_CHANGE:
> +		break;
> +	case ETH_RSS_HASH_TOP:
> +		rss_config.hash_alg = ETH_RSS_HASH_TOP;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (key) {
> +		rss_config.hash_key = kvcalloc(priv->rss_key_size,
> +					       sizeof(*rss_config.hash_key), GFP_KERNEL);

key is a bitstream...
IOW this code is like allocating a string with 
	calloc(length, sizeof(char)) 
:S

But what is the point of the allocations in this function in the first
place? You kvcalloc here, copy, call gve_adminq_configure_rss()
which dma_alloc_coherent() and copies into that, again.

> +		if (!rss_config.hash_key)
> +			return -ENOMEM;
> +
> +		memcpy(rss_config.hash_key, key, priv->rss_key_size * sizeof(*key));
> +	}
> +
> +	if (indir) {
> +		rss_config.hash_lut = kvcalloc(priv->rss_lut_size,
> +					       sizeof(*rss_config.hash_lut), GFP_KERNEL);
> +		if (!rss_config.hash_lut) {
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +
> +		memcpy(rss_config.hash_lut, indir, priv->rss_lut_size * sizeof(*indir));
> +	}
> +
> +	err = gve_adminq_configure_rss(priv, &rss_config);
> +
> +out:
> +	kvfree(rss_config.hash_lut);
> +	kvfree(rss_config.hash_key);
> +	return err;
> +}