From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro <nelio.laranjeiro@6wind.com>
Subject: Re: [PATCH v2 19/20] net/mlx5: add flow MPLS item
Date: Mon, 9 Jul 2018 17:00:26 +0200
Message-ID: <20180709150026.tsaeadpwngeo3v3t@laranjeiro-vm.dev.6wind.com>
References: <cover.1527506071.git.nelio.laranjeiro@6wind.com>
 <cover.1530111623.git.nelio.laranjeiro@6wind.com>
 <dcce366c08a8163ec7f38da6447a70ee72ff72e5.1530111623.git.nelio.laranjeiro@6wind.com>
 <20180707001131.GD53779@yongseok-MBP.local>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8bit
Cc: dev@dpdk.org, Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Yongseok Koh <yskoh@mellanox.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com
 [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id 96B4E4CE4
 for <dev@dpdk.org>; Mon,  9 Jul 2018 17:00:42 +0200 (CEST)
Received: by mail-wr1-f66.google.com with SMTP id r16-v6so11345241wrt.11
 for <dev@dpdk.org>; Mon, 09 Jul 2018 08:00:42 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <20180707001131.GD53779@yongseok-MBP.local>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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