RE: [PATCH v3 2/6] common/idpf: add RSS set/get ops

["Liu, Mingxia" <mingxia.liu@intel.com>]

> > +static int idpf_config_rss_hf(struct idpf_vport *vport, uint64_t
> > +rss_hf) {
> > +	uint64_t hena = 0, valid_rss_hf = 0;
> According to the coding style, only the last variable on a line should be
> initialized.
> 
[Liu, Mingxia] Ok, thank, I'll check if the same issue exist otherwhere.


> > +	vport->rss_hf = hena;
> > +
> > +	ret = idpf_vc_set_rss_hash(vport);
> > +	if (ret != 0) {
> > +		PMD_DRV_LOG(WARNING,
> > +			    "fail to set RSS offload types, ret: %d", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (valid_rss_hf & idpf_ipv4_rss)
> > +		valid_rss_hf |= rss_hf & RTE_ETH_RSS_IPV4;
> > +
> > +	if (valid_rss_hf & idpf_ipv6_rss)
> > +		valid_rss_hf |= rss_hf & RTE_ETH_RSS_IPV6;
> > +
> > +	if (rss_hf & ~valid_rss_hf)
> > +		PMD_DRV_LOG(WARNING, "Unsupported rss_hf 0x%"
> PRIx64,
> > +			    rss_hf & ~valid_rss_hf);
> It makes me a bit confused, valid_rss_hf is would be the sub of rss_hf
> according above assignment. Would it be possible to go here?
> And if it is possible, why not set valid_rss_hf before calling vc command?
>
[Liu, Mingxia] According to cmd_config_rss_parsed(), when the rss_hf set is not belong to flow_type_rss_offloads, it will be delete by &flow_type_rss_offloads.
What's more, in rte_eth_dev_rss_hash_update(), it will check again if rss_hf set is belong to flow_type_rss_offloads, if not, will return error.
So when entering function idpf_config_rss_hf, it wouldn't be possible that (rss_hf & ~valid_rss_hf) != 0.
Better to delete this piece of code.

For the second question,  why not set valid_rss_hf before calling vc command?
Because if we  set rss_hf to RTE_ETH_RSS_IPV4, then the rss hf value on idpf side mapping to  RTE_ETH_RSS_NONFRAG_IPV4_UDP | RTE_ETH_RSS_NONFRAG_IPV4_TCP |  RTE_ETH_RSS_NONFRAG_IPV4_SCTP |RTE_ETH_RSS_NONFRAG_IPV4_OTHER |RTE_ETH_RSS_FRAG_IPV4 is been set. 
But there is no  rss hf value on idpf side mapping to RTE_ETH_RSS_IPV4.
When we get rss_hf from vc, it won't tell us if RTE_ETH_RSS_IPV4 have ever been configured.
So dpdk software should record if RTE_ETH_RSS_IPV4 have ever been set by valid_rss_hf |= rss_hf & RTE_ETH_RSS_IPV4, and return to user when needed.

RTE_ETH_RSS_IPV6 is similar.


> > +	/* It MUST use the current LUT size to get the RSS lookup table,
> > +	 * otherwise if will fail with -100 error code.
> > +	 */
> > +	lut = rte_zmalloc(NULL, reta_size * sizeof(uint32_t), 0);
> > +	if (!lut) {
> > +		PMD_DRV_LOG(ERR, "No memory can be allocated");
> > +		return -ENOMEM;
> > +	}
> > +	/* store the old lut table temporarily */
> > +	rte_memcpy(lut, vport->rss_lut, reta_size * sizeof(uint32_t));
> Stored the vport->rss_lut to lut? But you overwrite the lut below?
> 
[Liu, Mingxia] Because lut include all redirection table, but we may want to update only several value of redirection table,
so we first stored the original lut, and update the required table entries.

> -----Original Message-----
> From: Wu, Jingjing <jingjing.wu@intel.com>
> Sent: Thursday, February 2, 2023 11:28 AM
> To: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>
> Subject: RE: [PATCH v3 2/6] common/idpf: add RSS set/get ops
> 
> > +static int idpf_config_rss_hf(struct idpf_vport *vport, uint64_t
> > +rss_hf) {
> > +	uint64_t hena = 0, valid_rss_hf = 0;
> According to the coding style, only the last variable on a line should be
> initialized.
> 
> > +	int ret = 0;
> > +	uint16_t i;
> > +
> > +	/**
> > +	 * RTE_ETH_RSS_IPV4 and RTE_ETH_RSS_IPV6 can be considered as 2
> > +	 * generalizations of all other IPv4 and IPv6 RSS types.
> > +	 */
> > +	if (rss_hf & RTE_ETH_RSS_IPV4)
> > +		rss_hf |= idpf_ipv4_rss;
> > +
> > +	if (rss_hf & RTE_ETH_RSS_IPV6)
> > +		rss_hf |= idpf_ipv6_rss;
> > +
> > +	for (i = 0; i < RTE_DIM(idpf_map_hena_rss); i++) {
> > +		uint64_t bit = BIT_ULL(i);
> > +
> > +		if (idpf_map_hena_rss[i] & rss_hf) {
> > +			valid_rss_hf |= idpf_map_hena_rss[i];
> > +			hena |= bit;
> > +		}
> > +	}
> > +
> > +	vport->rss_hf = hena;
> > +
> > +	ret = idpf_vc_set_rss_hash(vport);
> > +	if (ret != 0) {
> > +		PMD_DRV_LOG(WARNING,
> > +			    "fail to set RSS offload types, ret: %d", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (valid_rss_hf & idpf_ipv4_rss)
> > +		valid_rss_hf |= rss_hf & RTE_ETH_RSS_IPV4;
> > +
> > +	if (valid_rss_hf & idpf_ipv6_rss)
> > +		valid_rss_hf |= rss_hf & RTE_ETH_RSS_IPV6;
> > +
> > +	if (rss_hf & ~valid_rss_hf)
> > +		PMD_DRV_LOG(WARNING, "Unsupported rss_hf 0x%"
> PRIx64,
> > +			    rss_hf & ~valid_rss_hf);
> It makes me a bit confused, valid_rss_hf is would be the sub of rss_hf
> according above assignment. Would it be possible to go here?
> And if it is possible, why not set valid_rss_hf before calling vc command?
> 
> > +	vport->last_general_rss_hf = valid_rss_hf;
> > +
> > +	return ret;
> > +}
> > +
> >  static int
> >  idpf_init_rss(struct idpf_vport *vport)  { @@ -256,6 +357,204 @@
> > idpf_init_rss(struct idpf_vport *vport)
> >  	return ret;
> >  }
> >
> > +static int
> > +idpf_rss_reta_update(struct rte_eth_dev *dev,
> > +		     struct rte_eth_rss_reta_entry64 *reta_conf,
> > +		     uint16_t reta_size)
> > +{
> > +	struct idpf_vport *vport = dev->data->dev_private;
> > +	struct idpf_adapter *adapter = vport->adapter;
> > +	uint16_t idx, shift;
> > +	uint32_t *lut;
> > +	int ret = 0;
> > +	uint16_t i;
> > +
> > +	if (adapter->caps.rss_caps == 0 || dev->data->nb_rx_queues == 0) {
> > +		PMD_DRV_LOG(DEBUG, "RSS is not supported");
> > +		return -ENOTSUP;
> > +	}
> > +
> > +	if (reta_size != vport->rss_lut_size) {
> > +		PMD_DRV_LOG(ERR, "The size of hash lookup table
> configured "
> > +				 "(%d) doesn't match the number of
> hardware can "
> > +				 "support (%d)",
> > +			    reta_size, vport->rss_lut_size);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* It MUST use the current LUT size to get the RSS lookup table,
> > +	 * otherwise if will fail with -100 error code.
> > +	 */
> > +	lut = rte_zmalloc(NULL, reta_size * sizeof(uint32_t), 0);
> > +	if (!lut) {
> > +		PMD_DRV_LOG(ERR, "No memory can be allocated");
> > +		return -ENOMEM;
> > +	}
> > +	/* store the old lut table temporarily */
> > +	rte_memcpy(lut, vport->rss_lut, reta_size * sizeof(uint32_t));
> Stored the vport->rss_lut to lut? But you overwrite the lut below?
> 
[Liu, Mingxia] Because lut include all redirection table, but we may want to update only several value of redirection table,
so we first stored the original lut, and update the required table entries.

> > +
> > +	for (i = 0; i < reta_size; i++) {
> > +		idx = i / RTE_ETH_RETA_GROUP_SIZE;
> > +		shift = i % RTE_ETH_RETA_GROUP_SIZE;
> > +		if (reta_conf[idx].mask & (1ULL << shift))
> > +			lut[i] = reta_conf[idx].reta[shift];
> > +	}
> > +
> > +	rte_memcpy(vport->rss_lut, lut, reta_size * sizeof(uint32_t));
> > +	/* send virtchnl ops to configure RSS */
> > +	ret = idpf_vc_set_rss_lut(vport);
> > +	if (ret) {
> > +		PMD_INIT_LOG(ERR, "Failed to configure RSS lut");
> > +		goto out;
> > +	}
> > +out:
> > +	rte_free(lut);
> > +
> > +	return ret;
> > +}