[dpdk-dev] [PATCH] mbuf: fix tunnel flags check

[dpdk-dev] [PATCH] mbuf: fix tunnel flags check

[Thomas Monjalon]

A packet is tunnelled if the tunnel type is identified or if it has
an inner part.

Fix also a typo in RTE_PTYPE_INNER_L3_MASK.

Fixes: f295a00a2b44 ("mbuf: add definitions of unified packet types")

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 app/test-pmd/rxonly.c      | 2 +-
 lib/librte_mbuf/rte_mbuf.h | 7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/app/test-pmd/rxonly.c b/app/test-pmd/rxonly.c
index 41871d3..ee7fd8d 100644
--- a/app/test-pmd/rxonly.c
+++ b/app/test-pmd/rxonly.c
@@ -288,7 +288,7 @@ pkt_burst_receive(struct fwd_stream *fs)
 			}
 
 			/* inner L3 packet type */
-			ptype = mb->packet_type & RTE_PTYPE_INNER_INNER_L3_MASK;
+			ptype = mb->packet_type & RTE_PTYPE_INNER_L3_MASK;
 			switch (ptype) {
 			case RTE_PTYPE_INNER_L3_IPV4:
 				printf(" - Inner L3 type: IPV4");
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 115c560..dbd9095 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -595,7 +595,7 @@ extern "C" {
 /**
  * Mask of inner layer 3 packet types.
  */
-#define RTE_PTYPE_INNER_INNER_L3_MASK       0x00f00000
+#define RTE_PTYPE_INNER_L3_MASK             0x00f00000
 /**
  * TCP (Transmission Control Protocol) packet type.
  * It is used for inner packet only.
@@ -689,7 +689,10 @@ extern "C" {
 #define  RTE_ETH_IS_IPV6_HDR(ptype) ((ptype) & RTE_PTYPE_L3_IPV6)
 
 /* Check if it is a tunneling packet */
-#define RTE_ETH_IS_TUNNEL_PKT(ptype) ((ptype) & RTE_PTYPE_TUNNEL_MASK)
+#define RTE_ETH_IS_TUNNEL_PKT(ptype) ((ptype) & (RTE_PTYPE_TUNNEL_MASK | \
+                                                 RTE_PTYPE_INNER_L2_MASK | \
+                                                 RTE_PTYPE_INNER_L3_MASK | \
+                                                 RTE_PTYPE_INNER_L4_MASK))
 #endif /* RTE_NEXT_ABI */
 
 /**
-- 
2.4.2

Re: [dpdk-dev] [PATCH] mbuf: fix tunnel flags check

[Zhang, Helin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, July 15, 2015 4:50 PM
> To: Zhang, Helin
> Cc: olivier.matz@6wind.com; dev@dpdk.org
> Subject: [PATCH] mbuf: fix tunnel flags check
> 
> A packet is tunnelled if the tunnel type is identified or if it has an inner part.
> 
> Fix also a typo in RTE_PTYPE_INNER_L3_MASK.
> 
> Fixes: f295a00a2b44 ("mbuf: add definitions of unified packet types")
> 
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
>  app/test-pmd/rxonly.c      | 2 +-
>  lib/librte_mbuf/rte_mbuf.h | 7 +++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test-pmd/rxonly.c b/app/test-pmd/rxonly.c index
> 41871d3..ee7fd8d 100644
> --- a/app/test-pmd/rxonly.c
> +++ b/app/test-pmd/rxonly.c
> @@ -288,7 +288,7 @@ pkt_burst_receive(struct fwd_stream *fs)
>  			}
> 
>  			/* inner L3 packet type */
> -			ptype = mb->packet_type &
> RTE_PTYPE_INNER_INNER_L3_MASK;
> +			ptype = mb->packet_type & RTE_PTYPE_INNER_L3_MASK;
>  			switch (ptype) {
>  			case RTE_PTYPE_INNER_L3_IPV4:
>  				printf(" - Inner L3 type: IPV4");
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h index
> 115c560..dbd9095 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -595,7 +595,7 @@ extern "C" {
>  /**
>   * Mask of inner layer 3 packet types.
>   */
> -#define RTE_PTYPE_INNER_INNER_L3_MASK       0x00f00000
> +#define RTE_PTYPE_INNER_L3_MASK             0x00f00000
>  /**
>   * TCP (Transmission Control Protocol) packet type.
>   * It is used for inner packet only.
> @@ -689,7 +689,10 @@ extern "C" {
>  #define  RTE_ETH_IS_IPV6_HDR(ptype) ((ptype) & RTE_PTYPE_L3_IPV6)
> 
>  /* Check if it is a tunneling packet */ -#define RTE_ETH_IS_TUNNEL_PKT(ptype)
> ((ptype) & RTE_PTYPE_TUNNEL_MASK)
> +#define RTE_ETH_IS_TUNNEL_PKT(ptype) ((ptype) &
> (RTE_PTYPE_TUNNEL_MASK | \
> +
> RTE_PTYPE_INNER_L2_MASK | \
> +
> RTE_PTYPE_INNER_L3_MASK | \
> +
> +RTE_PTYPE_INNER_L4_MASK))
>  #endif /* RTE_NEXT_ABI */
Could you help to explain more of why here?
My understanding is that if an inner one can be recognized, there must be a tunnel type there.

Regards,
Helin

> 
>  /**
> --
> 2.4.2

Re: [dpdk-dev] [PATCH] mbuf: fix tunnel flags check

[Thomas Monjalon]

2015-07-15 23:57, Zhang, Helin:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > A packet is tunnelled if the tunnel type is identified or if it has an inner part.
> > 
> > Fix also a typo in RTE_PTYPE_INNER_L3_MASK.
> > 
> > Fixes: f295a00a2b44 ("mbuf: add definitions of unified packet types")
> > 
> > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
|...]
> >  /* Check if it is a tunneling packet */
> >  -#define RTE_ETH_IS_TUNNEL_PKT(ptype) ((ptype) & RTE_PTYPE_TUNNEL_MASK)
> > +#define RTE_ETH_IS_TUNNEL_PKT(ptype) ((ptype) & (RTE_PTYPE_TUNNEL_MASK | \
> > + RTE_PTYPE_INNER_L2_MASK | \
> > + RTE_PTYPE_INNER_L3_MASK | \
> > + RTE_PTYPE_INNER_L4_MASK))
> 
> Could you help to explain more of why here?
> My understanding is that if an inner one can be recognized, there must be a tunnel type there.

Not always.
It was my comment in mlx4 patch:
	http://dpdk.org/ml/archives/dev/2015-July/021702.html
Currently we can know that mlx4 has detected a tunnel but don't know which one.

Re: [dpdk-dev] [PATCH] mbuf: fix tunnel flags check

[Zhang, Helin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, July 15, 2015 5:13 PM
> To: Zhang, Helin
> Cc: olivier.matz@6wind.com; dev@dpdk.org
> Subject: Re: [PATCH] mbuf: fix tunnel flags check
> 
> 2015-07-15 23:57, Zhang, Helin:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > A packet is tunnelled if the tunnel type is identified or if it has an inner part.
> > >
> > > Fix also a typo in RTE_PTYPE_INNER_L3_MASK.
> > >
> > > Fixes: f295a00a2b44 ("mbuf: add definitions of unified packet
> > > types")
> > >
> > > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> |...]
> > >  /* Check if it is a tunneling packet */  -#define
> > > RTE_ETH_IS_TUNNEL_PKT(ptype) ((ptype) & RTE_PTYPE_TUNNEL_MASK)
> > > +#define RTE_ETH_IS_TUNNEL_PKT(ptype) ((ptype) &
> > > +(RTE_PTYPE_TUNNEL_MASK | \  RTE_PTYPE_INNER_L2_MASK | \
> > > +RTE_PTYPE_INNER_L3_MASK | \
> > > + RTE_PTYPE_INNER_L4_MASK))
> >
> > Could you help to explain more of why here?
> > My understanding is that if an inner one can be recognized, there must be a
> tunnel type there.
> 
> Not always.
> It was my comment in mlx4 patch:
> 	http://dpdk.org/ml/archives/dev/2015-July/021702.html
> Currently we can know that mlx4 has detected a tunnel but don't know which
> one.
I'd suggest to do more in mlx4 driver, rather than adding more checks in this macro.
If it can detect the inner l2/l3 type, the driver should tell it is a tunneled packet.
If it cannot know which tunnel it is, I'd suggest to add one more tunnel type of
RTE_PTYPE_TUNNEL_UNKNOWN.
Two reasons:
- PMD should tell enough info to high level caller or application. It should be clear enough.
- Adding more checks in those macro results in more cpu cycles for other NICs to check
the packet types.

Regards,
Helin

Re: [dpdk-dev] [PATCH] mbuf: fix tunnel flags check

[Thomas Monjalon]

2015-07-16 00:36, Zhang, Helin:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2015-07-15 23:57, Zhang, Helin:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > A packet is tunnelled if the tunnel type is identified or if it has an inner part.
> > > >
> > > > Fix also a typo in RTE_PTYPE_INNER_L3_MASK.
> > > >
> > > > Fixes: f295a00a2b44 ("mbuf: add definitions of unified packet
> > > > types")
> > > >
> > > > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> > |...]
> > > >  /* Check if it is a tunneling packet */  -#define
> > > > RTE_ETH_IS_TUNNEL_PKT(ptype) ((ptype) & RTE_PTYPE_TUNNEL_MASK)
> > > > +#define RTE_ETH_IS_TUNNEL_PKT(ptype) ((ptype) &
> > > > +(RTE_PTYPE_TUNNEL_MASK | \  RTE_PTYPE_INNER_L2_MASK | \
> > > > +RTE_PTYPE_INNER_L3_MASK | \
> > > > + RTE_PTYPE_INNER_L4_MASK))
> > >
> > > Could you help to explain more of why here?
> > > My understanding is that if an inner one can be recognized, there must be a
> > tunnel type there.
> > 
> > Not always.
> > It was my comment in mlx4 patch:
> > 	http://dpdk.org/ml/archives/dev/2015-July/021702.html
> > Currently we can know that mlx4 has detected a tunnel but don't know which
> > one.
> 
> I'd suggest to do more in mlx4 driver, rather than adding more checks in this macro.
> If it can detect the inner l2/l3 type, the driver should tell it is a tunneled packet.
> If it cannot know which tunnel it is, I'd suggest to add one more tunnel type of
> RTE_PTYPE_TUNNEL_UNKNOWN.
> Two reasons:
> - PMD should tell enough info to high level caller or application. It should be clear enough.

RTE_PTYPE_TUNNEL_UNKNOWN doesn't make it clearer, IMHO.

> - Adding more checks in those macro results in more cpu cycles for other NICs to check
> the packet types.

Not sure. It only extends the mask to check. It shouldn't add more cpu cycle.

Re: [dpdk-dev] [PATCH] mbuf: fix tunnel flags check

[Adrien Mazarguil]

On Thu, Jul 16, 2015 at 01:50:12AM +0200, Thomas Monjalon wrote:
> A packet is tunnelled if the tunnel type is identified or if it has
> an inner part.
> 
> Fix also a typo in RTE_PTYPE_INNER_L3_MASK.
> 
> Fixes: f295a00a2b44 ("mbuf: add definitions of unified packet types")
> 
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Makes sense.

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] mbuf: fix tunnel flags check

[Sanford, Robert]

I just noticed a few minor typos in comments:

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 917dd59..6352c32 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -680,14 +680,14 @@ extern "C" {
 /**
  * Check if the (outer) L3 header is IPv4. To avoid comparing IPv4 types
one by
  * one, bit 4 is selected to be used for IPv4 only. Then checking bit 4
can
- * determin if it is an IPV4 packet.
+ * determine if it is an IPV4 packet.
  */
 #define  RTE_ETH_IS_IPV4_HDR(ptype) ((ptype) & RTE_PTYPE_L3_IPV4)

 
 /**
- * Check if the (outer) L3 header is IPv4. To avoid comparing IPv4 types
one by
- * one, bit 6 is selected to be used for IPv4 only. Then checking bit 6
can
- * determin if it is an IPV4 packet.
+ * Check if the (outer) L3 header is IPv6. To avoid comparing IPv6 types
one by
+ * one, bit 6 is selected to be used for IPv6 only. Then checking bit 6
can
+ * determine if it is an IPV6 packet.
  */
 #define  RTE_ETH_IS_IPV6_HDR(ptype) ((ptype) & RTE_PTYPE_L3_IPV6)


--
Regards,
Robert

>On Thu, Jul 16, 2015 at 01:50:12AM +0200, Thomas Monjalon wrote:
>> A packet is tunnelled if the tunnel type is identified or if it has
>> an inner part.
>> 
>> Fix also a typo in RTE_PTYPE_INNER_L3_MASK.
>> 
>> Fixes: f295a00a2b44 ("mbuf: add definitions of unified packet types")
>> 
>> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
>
>Makes sense.
>
>Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>
>-- 
>Adrien Mazarguil
>6WIND

Re: [dpdk-dev] [PATCH] mbuf: fix tunnel flags check

[Thomas Monjalon]

2015-07-24 15:06, Sanford, Robert:
> I just noticed a few minor typos in comments:
[...]
> >On Thu, Jul 16, 2015 at 01:50:12AM +0200, Thomas Monjalon wrote:
> >> A packet is tunnelled if the tunnel type is identified or if it has
> >> an inner part.
> >> 
> >> Fix also a typo in RTE_PTYPE_INNER_L3_MASK.
> >> 
> >> Fixes: f295a00a2b44 ("mbuf: add definitions of unified packet types")
> >> 
> >> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> >
> >Makes sense.
> >
> >Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Applied with typo fixes.