Re: [dpdk-dev] [PATCH v1] net/ice: update the RSS RETA size with support values

["Yang, Qiming" <qiming.yang@intel.com>]

Little comments.

> -----Original Message-----
> From: Wang, Haiyue
> Sent: Monday, May 20, 2019 4:15 PM
> To: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>
> Cc: Wang, Haiyue <haiyue.wang@intel.com>
> Subject: [PATCH v1] net/ice: update the RSS RETA size with support values
> 
> Since ice can support 128, 512, 2K RSS RETA size value, change the update
> API to set it to resize the RSS RETA table. And by default, use 512 to sync
> with ETH_RSS_RETA_SIZE_x maximum value definition.
> Also the flag ICE_FLAG_RSS_AQ_CAPABLE is missed to set.
> 
> Fixes: 690175ee51bf ("net/ice: support getting device information")
> Fixes: ff963bfa7cb1 ("net/ice: support RSS")
> 
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> ---
>  drivers/net/ice/ice_ethdev.c | 41 +++++++++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c index
> bbaa7cf..c4ea09f 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -1149,6 +1149,12 @@ ice_setup_vsi(struct ice_pf *pf, enum ice_vsi_type
> type)
>  	TAILQ_INIT(&vsi->mac_list);
>  	TAILQ_INIT(&vsi->vlan_list);
> 
> +	/* Be sync with ETH_RSS_RETA_SIZE_x maximum value definition */

I think this stats is not clear, can't understand what's the ETH_RSS_RETA_SIZE_x maximum value, why don't say the default max
Value is 512?

> +	pf->hash_lut_size = hw->func_caps.common_cap.rss_table_size >
> +			ETH_RSS_RETA_SIZE_512 ? ETH_RSS_RETA_SIZE_512 :
> +			hw->func_caps.common_cap.rss_table_size;
> +	pf->flags |= ICE_FLAG_RSS_AQ_CAPABLE;
> +
>  	memset(&vsi_ctx, 0, sizeof(vsi_ctx));
>  	/* base_queue in used in queue mapping of VSI add/update
> command.
>  	 * Suppose vsi->base_queue is 0 now, don't consider SRIOV, VMDQ
> @@ -1627,7 +1633,7 @@ static int ice_init_rss(struct ice_pf *pf)
>  	rss_conf = &dev->data->dev_conf.rx_adv_conf.rss_conf;
>  	nb_q = dev->data->nb_rx_queues;
>  	vsi->rss_key_size = ICE_AQC_GET_SET_RSS_KEY_DATA_RSS_KEY_SIZE;
> -	vsi->rss_lut_size = hw->func_caps.common_cap.rss_table_size;
> +	vsi->rss_lut_size = pf->hash_lut_size;
> 
>  	if (is_safe_mode) {
>  		PMD_DRV_LOG(WARNING, "RSS is not supported in safe
> mode\n"); @@ -2033,7 +2039,7 @@ ice_dev_info_get(struct rte_eth_dev
> *dev, struct rte_eth_dev_info *dev_info)
>  	dev_info->rx_queue_offload_capa = 0;
>  	dev_info->tx_queue_offload_capa = 0;
> 
> -	dev_info->reta_size = hw->func_caps.common_cap.rss_table_size;
> +	dev_info->reta_size = pf->hash_lut_size;
>  	dev_info->hash_key_size = (VSIQF_HKEY_MAX_INDEX + 1) *
> sizeof(uint32_t);
> 
>  	dev_info->default_rxconf = (struct rte_eth_rxconf) { @@ -2605,28
> +2611,31 @@ ice_rss_reta_update(struct rte_eth_dev *dev,
>  		    uint16_t reta_size)
>  {
>  	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> -	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> -	uint16_t i, lut_size = hw->func_caps.common_cap.rss_table_size;
> +	uint16_t i, lut_size = pf->hash_lut_size;
>  	uint16_t idx, shift;
>  	uint8_t *lut;
>  	int ret;
> 
> -	if (reta_size != lut_size ||
> -	    reta_size > ETH_RSS_RETA_SIZE_512) {
> +	if (reta_size != ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_128 &&
> +	    reta_size != ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_512 &&
> +	    reta_size != ICE_AQC_GSET_RSS_LUT_TABLE_SIZE_2K) {
>  		PMD_DRV_LOG(ERR,
>  			    "The size of hash lookup table configured (%d)"
>  			    "doesn't match the number hardware can "
> -			    "supported (%d)",
> -			    reta_size, lut_size);
> +			    "supported (128, 512, 2048)",
> +			    reta_size);
>  		return -EINVAL;
>  	}
> 
> -	lut = rte_zmalloc(NULL, reta_size, 0);
> +	/* It MUST use the current LUT size to get the RSS lookup table,
> +	 * otherwise if will fail with -100 error code.
> +	 */

I think it's no need too detail explain. /* use current size to create lookup table*/

> +	lut = rte_zmalloc(NULL,  RTE_MAX(reta_size, lut_size), 0);
>  	if (!lut) {
>  		PMD_DRV_LOG(ERR, "No memory can be allocated");
>  		return -ENOMEM;
>  	}
> -	ret = ice_get_rss_lut(pf->main_vsi, lut, reta_size);
> +	ret = ice_get_rss_lut(pf->main_vsi, lut, lut_size);
>  	if (ret)
>  		goto out;
> 
> @@ -2637,6 +2646,12 @@ ice_rss_reta_update(struct rte_eth_dev *dev,
>  			lut[i] = reta_conf[idx].reta[shift];
>  	}
>  	ret = ice_set_rss_lut(pf->main_vsi, lut, reta_size);
> +	if (ret == 0 && lut_size != reta_size) {
> +		PMD_DRV_LOG(INFO,
> +			    "The size of hash lookup table is changed from (%d)
> to (%d)",
> +			    lut_size, reta_size);
> +		pf->hash_lut_size = reta_size;
> +	}
> 
>  out:
>  	rte_free(lut);
> @@ -2650,14 +2665,12 @@ ice_rss_reta_query(struct rte_eth_dev *dev,
>  		   uint16_t reta_size)
>  {
>  	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> -	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> -	uint16_t i, lut_size = hw->func_caps.common_cap.rss_table_size;
> +	uint16_t i, lut_size = pf->hash_lut_size;
>  	uint16_t idx, shift;
>  	uint8_t *lut;
>  	int ret;
> 
> -	if (reta_size != lut_size ||
> -	    reta_size > ETH_RSS_RETA_SIZE_512) {
> +	if (reta_size != lut_size) {
>  		PMD_DRV_LOG(ERR,
>  			    "The size of hash lookup table configured (%d)"
>  			    "doesn't match the number hardware can "
> --
> 2.7.4