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

[Adrien Mazarguil]

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