Re: [dpdk-dev] [PATCH v2 04/29] net/ena/base: set default hash key

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/1/2020 3:21 PM, Michal Krawczyk wrote:
> The RSS hash key was present in the device, but it wasn't exposed to
> the user. The other key still cannot be set, but now it can be accessed
> if one needs to do that.

What is 'the other key'?

> 
> By default, the random hash key is used and it is generated only once
> when requested for the first time.

It looks like this patch does
1- setting default rss hash key (generated randomly)
2- Separate 'ena_com_get_hash_function()' and 'ena_com_get_hash_key()'
  - I didn't get the motivation of this seperation, new function not called
3- Minor fixes, like 'key' check in 'ena_com_fill_hash_function()', in
'ENA_ADMIN_TOEPLITZ' case, ...
4- remove 'ena_com_ind_tbl_convert_from_device()'

What to you think seperating above ones if they are logically seperate?

> 
> Signed-off-by: Michal Krawczyk <mk@semihalf.com>
> Reviewed-by: Igor Chauskin <igorch@amazon.com>
> Reviewed-by: Guy Tzalik <gtzalik@amazon.com>

<...>

> @@ -1032,6 +1032,24 @@ static int ena_com_get_feature(struct ena_com_dev *ena_dev,
>  				      feature_ver);
>  }
>  
> +int ena_com_get_current_hash_function(struct ena_com_dev *ena_dev)
> +{
> +	return ena_dev->rss.hash_func;
> +}


This function is not called, who is the intendent consumer of the function?
Commit log mentions exposing to user, is the intention this function to be
called by application, if so it shoudln't, applications doesn't call driver
fucntions directly.

<...>

> @@ -1266,30 +1284,6 @@ static int ena_com_ind_tbl_convert_to_device(struct ena_com_dev *ena_dev)
>  	return 0;
>  }
>  
> -static int ena_com_ind_tbl_convert_from_device(struct ena_com_dev *ena_dev)
> -{
> -	u16 dev_idx_to_host_tbl[ENA_TOTAL_NUM_QUEUES] = { (u16)-1 };
> -	struct ena_rss *rss = &ena_dev->rss;
> -	u8 idx;
> -	u16 i;
> -
> -	for (i = 0; i < ENA_TOTAL_NUM_QUEUES; i++)
> -		dev_idx_to_host_tbl[ena_dev->io_sq_queues[i].idx] = i;
> -
> -	for (i = 0; i < 1 << rss->tbl_log_size; i++) {
> -		if (rss->rss_ind_tbl[i].cq_idx > ENA_TOTAL_NUM_QUEUES)
> -			return ENA_COM_INVAL;
> -		idx = (u8)rss->rss_ind_tbl[i].cq_idx;
> -
> -		if (dev_idx_to_host_tbl[idx] > ENA_TOTAL_NUM_QUEUES)
> -			return ENA_COM_INVAL;
> -
> -		rss->host_rss_ind_tbl[i] = dev_idx_to_host_tbl[idx];
> -	}
> -
> -	return 0;
> -}> -

This function and where it is called seems removed, is this related to this
patch, "setting default hash key"?

<...>

>  	case ENA_ADMIN_TOEPLITZ:
> -		if (key_len > sizeof(hash_key->key)) {
> -			ena_trc_err("key len (%hu) is bigger than the max supported (%zu)\n",
> -				    key_len, sizeof(hash_key->key));
> -			return ENA_COM_INVAL;
> +		if (key) {
> +			if (key_len != sizeof(hash_key->key)) {
> +				ena_trc_err("key len (%hu) doesn't equal the supported size (%zu)\n",
> +					     key_len, sizeof(hash_key->key));
> +				return ENA_COM_INVAL;
> +			}
> +			memcpy(hash_key->key, key, key_len);
> +			rss->hash_init_val = init_val;
> +			hash_key->keys_num = key_len >> 2;

I am aware this is copy/paste, but here '>> 2' is "/ sizeof(unit2_t)", would it
be better to use that way instead of hardcoded value?

<...>

> @@ -256,6 +256,23 @@ static const struct eth_dev_ops ena_dev_ops = {
>  	.reta_query           = ena_rss_reta_query,
>  };
>  
> +void ena_rss_key_fill(void *key, size_t size)
> +{
> +	static bool key_generated;
> +	static uint8_t default_key[ENA_HASH_KEY_SIZE];

If there are multiple 'ena' devices in the platform, using 'static' variables
will cause each device use same rss key, I just want to double check this is the
intention.