Re: [dpdk-dev] [PATCH v2 13/20] net/mlx5: add RSS flow action

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

Hi Yongseok,

I am only addressing your questions concerns here, almost all other
points I also agree with them.

On Thu, Jul 05, 2018 at 07:16:35PM -0700, Yongseok Koh wrote:
> On Wed, Jun 27, 2018 at 05:07:45PM +0200, Nelio Laranjeiro wrote:
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > ---
>[...]
> 
> > + */
> > +static void
> > +mlx5_flow_layers_update(struct rte_flow *flow, uint32_t layers)
> > +{
> > +	if (flow->expand) {
> > +		if (flow->cur_verbs)
> > +			flow->cur_verbs->layers |= layers;
> 
> If flow->cur_verbs is null, does that mean it is a testing call? Then, is it
> unnecessary to update layers for the testing call? Confusing..

No it may also happen if the buffer was too small, in any case the code
continues its validation.

> > +	} else {
> > +		flow->layers |= layers;
> > +	}
> > +}
> > +
> > +/**
> > + * Get layers bit-field.
> > + *
> > + * @param flow[in, out]
> > + *   Pointer to flow structure.
> > + */
> > +static uint32_t
> > +mlx5_flow_layers(struct rte_flow *flow)
> > +{
> > +	uint32_t layers = flow->layers;
> > +
> > +	if (flow->expand && flow->cur_verbs)
> 
> If flow is expanded and it is a testing call, then flow->layers is used?
>
> > +		layers |= flow->cur_verbs->layers;
> > +	return layers;
> 
> This part is so unclear to me, hard to understand. There are two 'layers'
> fields, one in rte_flow and the other in mlx5_flow_verbs. It seems
> rte_flow->layers is used only when the flow isn't expanded. If a flow is
> expanded, flow->expand is set after processing the first entry in the expanded
> list. In mlx5_flow_merge(),
> 
> 	for (i = 0; i != buf->entries; ++i) {
> 
> 		...
> 
> 		flow->expand = !!(buf->entries > 1);
> 	}
> 
> Why is flow->expand set at the end of the loop? Is this in order to avoid
> validation for the expanded flows?
> mlx5_flow_item_xxx() executes validation only if flow->expand is zero,
> why?

Expanded flows are PMD internal cooking to match the user request, they
are fully added by the PMD and thus must be valid.  There is not need to
validate their position nor redundancy and for the spec, last, mask they
are provided as wildcards which means those pointer are null.

> And why does mlx5_flow_layers() have to return (flow->layers |
> flow->cur_verbs->layers) if expanded?

You are right, and indeed it is a bug.  With the fix, only the expanded
flow will use the verbs->layers, whereas the first conversion will use
the flow->layers.
 
> If there are 3 entries in the rte_flow_expand_rss,
> 	eth
> 	eth / ipv4 / udp
> 	eth / ipv6 / udp
> 
> Then, the 2nd and 3rd don't have MLX5_FLOW_LAYER_OUTER_L2 in layers field?
> Please explain in details and add comments appropriately.

The 2nd and 3rd should have the same layers bits as the 1st rule in
addition of their own bits set .e.g 2nd rule will have L2 + L3_IPv4 +
L4_UDP the 3rd L3 + L3_IPv6 + L4_UDP.


I will try to sanitize it, the issue being for a single rte_flow we can
have several verbs flows with the expansion, for each of them we need to
update the priority and this based on the more specific layer of each,
it needs to be stored in some place.  I think the biggest issue you are
raising here is the lack of correct variable naming and documentation,
I'll try to improve it at most.

>[...]
> > +	if (verbs) {
> > +		verbs->modifier |= MLX5_FLOW_MOD_MARK;
> > +		if (verbs->modifier & MLX5_FLOW_MOD_FLAG) {
> > +			mlx5_flow_verbs_mark_update(verbs, mark->id);
> > +			size = 0;
> > +		} else if (size <= flow_size) {
> 
> If verbs isn't null (not testing call), isn't it guaranteed there's enough
> space? Is it still needed to check the size?

Unfortunately not, verbs variable may be valid and pointing into a zone
in the buffer but it does not mean there is enough space to store the
mark specification in the buffer.

> >  			tag.tag_id = mlx5_flow_mark_set(mark->id);
> >  			mlx5_flow_spec_verbs_add(flow, &tag, size);
> >  		}
> > +	} else {
> > +		if (flow->modifier & MLX5_FLOW_MOD_FLAG)
> > +			size = 0;
> >  	}
> >  	flow->modifier |= MLX5_FLOW_MOD_MARK;
> >  	return size;
> > @@ -1185,6 +1656,15 @@ mlx5_flow_actions(struct rte_eth_dev *dev,
> >  	int remain = flow_size;
> >  	int ret = 0;
> >  
> > +	/*
> > +	 * FLAG/MARK are the only actions having a specification in Verbs and
> > +	 * not making part of the packet fate.  Due to this specificity and to
> > +	 * avoid extra variable, their bit in the flow->modifier bit-field are
> > +	 * disabled here to compute the exact necessary memory those action
> > +	 * needs.
> > +	 */
> > +	flow->modifier &= ~(MLX5_FLOW_MOD_FLAG | MLX5_FLOW_MOD_MARK);
> 
> Can't understand this well. Is this for the case where the flow is expanded? If
> so, why don't you reset flow->modifier in the for loop of mlx5_flow_merge()?

Yes it is, I'll move it.

>[...]
> > +	assert(ret > 0);
> > +	buf = rte_calloc(__func__, 1, ret, 0);
> > +	if (!buf) {
> > +		rte_flow_error_set(error, ENOMEM,
> > +				   RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +				   NULL,
> > +				   "not enough memory to expand the RSS flow");
> > +		goto error;
> > +	}
> 
> I'm pretty sure you've already fixed this bug. Validation can't return ENOMEM.

You know me well ;)

Thanks,

-- 
Nélio Laranjeiro
6WIND