[dpdk-dev] 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(-)
  

> -----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
  

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.
  

> -----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
  

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.
  

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>