[dpdk-dev] [PATCH v2 3/6] librte_ether: initialise IPv4 protocol mask for rte_flow

[Iremonger, Bernard]

Hi Adrien,

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> Sent: Wednesday, August 30, 2017 3:39 PM
> To: Iremonger, Bernard <bernard.iremonger at intel.com>
> Cc: dev at dpdk.org; Yigit, Ferruh <ferruh.yigit at intel.com>; Ananyev,
> Konstantin <konstantin.ananyev at intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu at intel.com>
> Subject: Re: [PATCH v2 3/6] librte_ether: initialise IPv4 protocol mask for
> rte_flow
> 
> On Wed, Aug 30, 2017 at 01:28:04PM +0000, Iremonger, Bernard wrote:
> > Hi Adrien,
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil at 6wind.com]
> > > Sent: Wednesday, August 30, 2017 1:39 PM
> > > To: Iremonger, Bernard <bernard.iremonger at intel.com>
> > > Cc: dev at dpdk.org; Yigit, Ferruh <ferruh.yigit at intel.com>; Ananyev,
> > > Konstantin <konstantin.ananyev at intel.com>; Dumitrescu, Cristian
> > > <cristian.dumitrescu at intel.com>
> > > 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 <bernard.iremonger at intel.com>
> > > > ---
> > > >  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

I will drop this patch and send a v3 patch set.
I will define an ipv4_mask in the application.

Regards,

Bernard.