diff mbox

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

Message ID 1503677438-27591-4-git-send-email-bernard.iremonger@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Iremonger, Bernard Aug. 25, 2017, 4:10 p.m. UTC 
  Initialise the next_proto_id mask in the default mask for
rte_flow_item_type_ipv4.

Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 lib/librte_ether/rte_flow.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Adrien Mazarguil Aug. 30, 2017, 12:39 p.m. UTC | #1 
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@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.
Iremonger, Bernard Aug. 30, 2017, 1:28 p.m. UTC | #2 
Hi Adrien,

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, August 30, 2017 1:39 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@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@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.
I can define an ipv4_mask in the application if you insist. 

Regards,

Bernard.
Adrien Mazarguil Aug. 30, 2017, 2:39 p.m. UTC | #3 
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 <bernard.iremonger@intel.com>
> > Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev,
> > Konstantin <konstantin.ananyev@intel.com>; Dumitrescu, Cristian
> > <cristian.dumitrescu@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@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.
Iremonger, Bernard Aug. 30, 2017, 3:12 p.m. UTC | #4 
Hi Adrien,

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, August 30, 2017 3:39 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@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@6wind.com]
> > > Sent: Wednesday, August 30, 2017 1:39 PM
> > > To: Iremonger, Bernard <bernard.iremonger@intel.com>
> > > Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev,
> > > Konstantin <konstantin.ananyev@intel.com>; Dumitrescu, Cristian
> > > <cristian.dumitrescu@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@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.
diff mbox

Patch

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,
 		.src_addr = RTE_BE32(0xffffffff),
 		.dst_addr = RTE_BE32(0xffffffff),
 	},