[dpdk-dev] [PATCH v2 12/20] net/mlx5: add mark/flag flow action

[Nélio Laranjeiro]

On Thu, Jul 05, 2018 at 12:56:09PM -0700, Yongseok Koh wrote:
>[...]
> > > > +	if (mark->id >= MLX5_FLOW_MARK_MAX)
> > > > +		return rte_flow_error_set(error, EINVAL,
> > > > +					  RTE_FLOW_ERROR_TYPE_ACTION_CONF,
> > > > +					  &mark->id,
> > > > +					  "mark must be between 0 and"
> > > > +					  " 16777199");
> > > 
> > > Use %d and (MLX5_FLOW_MARK_MAX - 1), instead of fixed string.
> > 
> > It needs an snprintf, rte_flow_error_set() does not accept formatting
> > strings.
> 
> I think the following would work but never mind. I'm okay with leaving it as is.
> No need to make a change here.
> 
> #define STRINGIFY(x) #x
> #define TOSTRING(x) STRINGIFY(x)
> 					"mark must be between 0 and "
> 					TOSTRING(MLX5_FLOW_MARK_MAX - 1));
> 

It was to avoid adding a macro, but indeed there is the same kind in
mlx4, I'll port them for mlx5.

> > >[...]
> > Addressing both question here, for the flow_stop() and flow_destroy()
> > the process is different, for the stop, the flow remains with the mark
> > bit set but all queues must me cleared, there is no comparison to make.
> > As you can see, it don't even get a flow, it directly unset the mask bit
> > in the Rx queues.
> > For the destroy the issue is different, several flows may be using the
> > same Rx queues, if one of them will remains and has a mark, then the
> > associated queues must keep their mark bit set.
> > As the process is different, it would end in two distinct functions and
> > each one used by a single function.
> > 
> > For the mlx5_flow_rxq_mark(), the situation is different, the same
> > process is make when a flow is created and the flow are started.
> 
> I knew the differences but I just wanted to ask if having a separate function
> can be a viable option, e.g.,
> 
> mlx5_flow_rxq_mark_set()
> mlx5_flow_rxq_mark_clear()
> mlx5_flow_rxq_mark_trim()

Certainly, the point is those functions have a short life as few patches
letter they will be removed.
I suppose you prefer to have them and I don't think it will take too
much time to add such function, it will make part of the next revision
;).

Thanks,

-- 
Nélio Laranjeiro
6WIND