Re: [PATCH v2 19/20] net/mlx5: add flow MPLS item

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

On Fri, Jul 06, 2018 at 05:11:31PM -0700, Yongseok Koh wrote:
> On Wed, Jun 27, 2018 at 05:07:51PM +0200, Nelio Laranjeiro wrote:
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > ---
>[...]
> > +	if (spec) {
> > +		memcpy(&mpls.val.label, spec, sizeof(mpls.val.label));
> > +		memcpy(&mpls.mask.label, mask, sizeof(mpls.mask.label));
> > +		/* Remove unwanted bits from values.  */
> > +		mpls.val.label &= mpls.mask.label;
> > +	}
> > +	if (size <= flow_size)
> 
> Is it guaranteed flow->cur_verbs isn't null if size fits? Could be obvious but
> just want to make sure.

Yes it is.

> > +		mlx5_flow_spec_verbs_add(flow, &mpls, size);
> > +	mlx5_flow_layers_update(flow, MLX5_FLOW_LAYER_MPLS);
> > +	if (layers & MLX5_FLOW_LAYER_OUTER_L4_UDP)
> > +		flow->ptype = RTE_PTYPE_TUNNEL_MPLS_IN_GRE | RTE_PTYPE_L4_UDP;
> > +	else
> > +		flow->ptype = RTE_PTYPE_TUNNEL_MPLS_IN_GRE;
> > +	return size;
> > +#endif /* !HAVE_IBV_DEVICE_MPLS_SUPPORT */
> > +	return rte_flow_error_set(error, ENOTSUP,
> > +				  RTE_FLOW_ERROR_TYPE_ITEM,
> > +				  item,
> > +				  "MPLS is not supported by Verbs, please"
> > +				  " update.");
> > +}
> > +
> >  /**
> >   * Validate items provided by the user.
> >   *
> > @@ -1650,6 +1722,9 @@ mlx5_flow_items(struct rte_eth_dev *dev,
> >  		case RTE_FLOW_ITEM_TYPE_GRE:
> >  			ret = mlx5_flow_item_gre(items, flow, remain, error);
> >  			break;
> 
> #ifdef HAVE_IBV_DEVICE_MPLS_SUPPORT
> 
> > +		case RTE_FLOW_ITEM_TYPE_MPLS:
> > +			ret = mlx5_flow_item_mpls(items, flow, remain, error);
> > +			break;
> 
> #endif /* !HAVE_IBV_DEVICE_MPLS_SUPPORT */
> 
> How about this?
>[...]

It adds another couple of #ifdef #endif and the final output won't help
much the user, having an error "MPLS is not updated by Verbs, please
update" will help more than "item not supported".

Regards,

-- 
Nélio Laranjeiro
6WIND