Re: [PATCH] octeontx2-pf: Add RSS multi group support

[Jakub Kicinski]

On Fri,  4 Dec 2020 14:54:51 +0530 Geetha sowjanya wrote:
> Hardware supports 8 RSS groups per interface. Currently we are using
> only group '0'. This patch allows user to create new RSS groups/contexts
> and use the same as destination for flow steering rules.
> 
> usage:
> To steer the traffic to RQ 2,3
> 
> ethtool -X eth0 weight 0 0 1 1 context new
> (It will print the allocated context id number)
> New RSS context is 1
> 
> ethtool -N eth0 flow-type tcp4 dst-port 80 context 1 loc 1
> 
> To delete the context
> ethtool -X eth0 context 1 delete
> 
> When an RSS context is removed, the active classification
> rules using this context are also removed.
> 
> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx>
> Signed-off-by: Geetha sowjanya <gakula@xxxxxxxxxxx>

Looks good, minor coding nits below.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 73fb94d..0c84dcf 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -270,14 +270,17 @@ int otx2_set_flowkey_cfg(struct otx2_nic *pfvf)
>  	return err;
>  }
>  
> -int otx2_set_rss_table(struct otx2_nic *pfvf)
> +int otx2_set_rss_table(struct otx2_nic *pfvf, int ctx_id)
>  {
>  	struct otx2_rss_info *rss = &pfvf->hw.rss_info;
> +	int index = rss->rss_size * ctx_id;

const?

>  	struct mbox *mbox = &pfvf->mbox;
> +	struct otx2_rss_ctx *rss_ctx;
>  	struct nix_aq_enq_req *aq;
>  	int idx, err;
>  
>  	mutex_lock(&mbox->lock);
> +	rss_ctx = rss->rss_ctx[ctx_id];
>  	/* Get memory to put this msg */
>  	for (idx = 0; idx < rss->rss_size; idx++) {
>  		aq = otx2_mbox_alloc_msg_nix_aq_enq(mbox);

> +/* RSS context configuration */
> +static int otx2_set_rxfh_context(struct net_device *dev, const u32 *indir,
> +				 const u8 *hkey, const u8 hfunc,
> +				 u32 *rss_context, bool delete)
> +{
>  	struct otx2_nic *pfvf = netdev_priv(dev);
> +	struct otx2_rss_ctx *rss_ctx;
> +	struct otx2_rss_info *rss;
> +	int ret, idx;
> +
> +	if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP)
> +		return -EOPNOTSUPP;
> +
> +	rss = &pfvf->hw.rss_info;
>  
> -	return pfvf->hw.rss_info.rss_size;
> +	if (!rss->enable) {
> +		netdev_err(dev, "RSS is disabled, cannot change settings\n");
> +		return -EIO;
> +	}
> +
> +	if (hkey) {
> +		memcpy(rss->key, hkey, sizeof(rss->key));
> +		otx2_set_rss_key(pfvf);
> +	}
> +	if (delete)
> +		return otx2_rss_ctx_delete(pfvf, *rss_context);
> +
> +	if (*rss_context == ETH_RXFH_CONTEXT_ALLOC) {
> +		ret = otx2_rss_ctx_create(pfvf, rss_context);
> +		if (ret)
> +			return ret;
> +	}
> +	if (indir) {
> +		rss_ctx = rss->rss_ctx[*rss_context];
> +		for (idx = 0; idx < rss->rss_size; idx++)
> +			rss_ctx->ind_tbl[idx] = indir[idx];
> +	}
> +	otx2_set_rss_table(pfvf, *rss_context);
> +
> +	return 0;
> +}
> +
> +static int otx2_get_rxfh_context(struct net_device *dev, u32 *indir,
> +				 u8 *hkey, u8 *hfunc, u32 rss_context)
> +{
> +	struct otx2_nic *pfvf = netdev_priv(dev);
> +	struct otx2_rss_ctx *rss_ctx;
> +	struct otx2_rss_info *rss;
> +	int idx;
> +
> +	rss = &pfvf->hw.rss_info;
> +
> +	if (!rss->enable) {
> +		netdev_err(dev, "RSS is disabled\n");
> +		return -EIO;
> +	}
> +	if (rss_context >= MAX_RSS_GROUPS)
> +		return -EINVAL;
> +
> +	rss_ctx = rss->rss_ctx[rss_context];
> +	if (!rss_ctx)
> +		return -EINVAL;
> +
> +	if (indir) {
> +		for (idx = 0; idx < rss->rss_size; idx++)
> +			indir[idx] = rss_ctx->ind_tbl[idx];
> +	}
> +	if (hkey)
> +		memcpy(hkey, rss->key, sizeof(rss->key));
> +
> +	if (hfunc)
> +		*hfunc = ETH_RSS_HASH_TOP;
> +
> +	return 0;
>  }

Can the old callbacks not be converted to something like:

static int otx2_get_rxfh(...)
{
	return otx2_get_rxfh_context(..., DEFAULT_RSS_CONTEXT_GROUP);
}

?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c
> index be8ccfc..e5f6b4a 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c
> @@ -17,6 +17,7 @@ struct otx2_flow {
>  	u16 entry;
>  	bool is_vf;
>  	int vf;
> +	u8 rss_ctx_id;

If you put it next to the bool it will make the structure smaller (less
padding).

>  };
>  
>  int otx2_alloc_mcam_entries(struct otx2_nic *pfvf)

> @@ -521,7 +523,6 @@ static int otx2_add_flow_msg(struct otx2_nic *pfvf, struct otx2_flow *flow)
>  		mutex_unlock(&pfvf->mbox.lock);
>  		return err;
>  	}
> -

Unrelated whitespace change.

>  	req->entry = flow->entry;
>  	req->intf = NIX_INTF_RX;
>  	req->set_cntr = 1;

> @@ -555,9 +560,10 @@ static int otx2_add_flow_msg(struct otx2_nic *pfvf, struct otx2_flow *flow)
>  	return err;
>  }
>  
> -int otx2_add_flow(struct otx2_nic *pfvf, struct ethtool_rx_flow_spec *fsp)
> +int otx2_add_flow(struct otx2_nic *pfvf, struct ethtool_rxnfc *nfc)
>  {
>  	struct otx2_flow_config *flow_cfg = pfvf->flow_cfg;
> +	struct ethtool_rx_flow_spec *fsp = &nfc->fs;
>  	u32 ring = ethtool_get_flow_spec_ring(fsp->ring_cookie);

Please keep variable decl lines sorted longest to shortest.

>  	struct otx2_flow *flow;
>  	bool new = false;

> @@ -643,10 +652,27 @@ int otx2_remove_flow(struct otx2_nic *pfvf, u32 location)
>  	list_del(&flow->list);
>  	kfree(flow);
>  	flow_cfg->nr_flows--;
> -

Unrelated whitespace change.

>  	return 0;
>  }
>  
> +void  otx2_rss_ctx_flow_del(struct otx2_nic *pfvf, int ctx_id)

Double space after void

> +{
> +	struct otx2_flow *flow, *tmp;
> +	int err;
> +
> +	list_for_each_entry_safe(flow, tmp, &pfvf->flow_cfg->flow_list, list) {
> +		if (!flow)
> +			return;

I don't think you can possibly have a NULL entry on a standard list.

> +		if (flow->rss_ctx_id != ctx_id)
> +			continue;
> +		err = otx2_remove_flow(pfvf, flow->location);
> +		if (err)
> +			netdev_warn(pfvf->netdev,
> +				    "Can't delete the rule %d associated with this rss group",
> +				    flow->location);

Printing the error code could be useful : %d", ..., err);

> +	}
> +}