From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v2 3/6] librte_ether: initialise IPv4 protocol mask for rte_flow Date: Wed, 30 Aug 2017 16:39:03 +0200 Message-ID: <20170830143903.GK4301@6wind.com> References: <1503496275-27492-1-git-send-email-bernard.iremonger@intel.com> <1503677438-27591-4-git-send-email-bernard.iremonger@intel.com> <20170830123912.GJ4301@6wind.com> <8CEF83825BEC744B83065625E567D7C24E042C4F@IRSMSX108.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "dev@dpdk.org" , "Yigit, Ferruh" , "Ananyev, Konstantin" , "Dumitrescu, Cristian" To: "Iremonger, Bernard" Return-path: Received: from mail-wm0-f54.google.com (mail-wm0-f54.google.com [74.125.82.54]) by dpdk.org (Postfix) with ESMTP id 1AE092BAF for ; Wed, 30 Aug 2017 16:39:15 +0200 (CEST) Received: by mail-wm0-f54.google.com with SMTP id 187so2848093wmn.1 for ; Wed, 30 Aug 2017 07:39:15 -0700 (PDT) Content-Disposition: inline In-Reply-To: <8CEF83825BEC744B83065625E567D7C24E042C4F@IRSMSX108.ger.corp.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Aug 30, 2017 at 01:28:04PM +0000, Iremonger, Bernard wrote: > Hi Adrien, > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > Sent: Wednesday, August 30, 2017 1:39 PM > > To: Iremonger, Bernard > > Cc: dev@dpdk.org; Yigit, Ferruh ; Ananyev, > > Konstantin ; Dumitrescu, Cristian > > > > Subject: Re: [PATCH v2 3/6] librte_ether: initialise IPv4 protocol mask for > > rte_flow > > > > Hi Bernard, > > > > On Fri, Aug 25, 2017 at 05:10:35PM +0100, Bernard Iremonger wrote: > > > Initialise the next_proto_id mask in the default mask for > > > rte_flow_item_type_ipv4. > > > > > > Signed-off-by: Bernard Iremonger > > > --- > > > lib/librte_ether/rte_flow.h | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > > > index bba6169..59c42fa 100644 > > > --- a/lib/librte_ether/rte_flow.h > > > +++ b/lib/librte_ether/rte_flow.h > > > @@ -489,6 +489,7 @@ struct rte_flow_item_ipv4 { #ifndef __cplusplus > > > static const struct rte_flow_item_ipv4 rte_flow_item_ipv4_mask = { > > > .hdr = { > > > + .next_proto_id = 0xff, > > > > Please don't change the default mask to cover this field as it means > > all rte_flow-based applications that do not provide a specific mask > > (.mask == NULL) have to always set this field to some valid value. > > This is not a convenient default behavior. > > > > > .src_addr = RTE_BE32(0xffffffff), > > > .dst_addr = RTE_BE32(0xffffffff), > > > }, > > > -- > > > 1.9.1 > > > > > > > I'll have to NACK this change. The example application should define its own > > mask if next_proto_id must be always set. > > Surely for IPv4 the next_proto_id will always be set to TCP(6) , UDP(17) or SCTP (132). > If the mask is 0 for next_proto_id then it is not possible to match on the protocol. Applications normally match the next protocol implicitly by providing it as the subsequent item (e.g. in testpmd by writing "eth / ip / tcp" instead of "eth / ip next_proto_id spec 6"). This change forces users to write "eth / ip next_proto_id spec 6 / tcp" or face an error due to an uninitialized next_proto_id (which might be garbage due to uninitialized memory, not just 0). > I can define an ipv4_mask in the application if you insist. Yes please, a better suggestion would be to rely on the subsequent item type and not on the value of this field. These default masks must only cover basic fields like source/destination addresses and ports for most protocols. -- Adrien Mazarguil 6WIND