[dpdk-dev,v3] mbuf: fix of enabling all newly added RX error flags

  Before redefining mbuf structure, there was lack of free bits in 'ol_flags'
(32 bits in total) for new RX or TX flags. So it tried to reuse existant
bits as most as possible, or even assigning 0 to some of bit flags. After
new mbuf structure defined, there are quite a lot of free bits. So those
newly added bit flags should be assigned with correct and valid bit values,
and getting their names should be enabled as well. Note that 'RECIP' should
be removed, as nowhere will use it. 'PKT_RX_ERR' is defined to replace all
other error bit flags, e.g. MAC error, Oversize error, header buffer
overflow error.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 lib/librte_mbuf/rte_mbuf.c      |  7 ++-----
 lib/librte_mbuf/rte_mbuf.h      |  7 ++-----
 lib/librte_pmd_i40e/i40e_rxtx.c | 15 ++++-----------
 3 files changed, 8 insertions(+), 21 deletions(-)

v2 changes:
* Removed error flag of 'ECIPE' processing only in both i40e PMD and mbuf. All
  other error flags were added back.
* Assigned error flags with correct and valid values, as their previous values
  were invalid.
* Enabled getting all error flag names.

v3 changes:
* 'PKT_RX_ERR' is defined to replace error bit flags of MAC error, Oversize
  error, header buffer overflow error.
* Removed assigning values to PKT_TX_* bit flags, as it has already been done
  in another packet set.
  

Hi Helin,

> -----Original Message-----
> From: Zhang, Helin
> Sent: Saturday, December 06, 2014 1:34 AM
> To: dev@dpdk.org
> Cc: Cao, Waterman; Cao, Min; olivier.matz@6wind.com; Ananyev, Konstantin; Zhang, Helin
> Subject: [PATCH v3] mbuf: fix of enabling all newly added RX error flags
> 
> Before redefining mbuf structure, there was lack of free bits in 'ol_flags'
> (32 bits in total) for new RX or TX flags. So it tried to reuse existant
> bits as most as possible, or even assigning 0 to some of bit flags. After
> new mbuf structure defined, there are quite a lot of free bits. So those
> newly added bit flags should be assigned with correct and valid bit values,
> and getting their names should be enabled as well. Note that 'RECIP' should
> be removed, as nowhere will use it. 'PKT_RX_ERR' is defined to replace all
> other error bit flags, e.g. MAC error, Oversize error, header buffer
> overflow error.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  lib/librte_mbuf/rte_mbuf.c      |  7 ++-----
>  lib/librte_mbuf/rte_mbuf.h      |  7 ++-----
>  lib/librte_pmd_i40e/i40e_rxtx.c | 15 ++++-----------
>  3 files changed, 8 insertions(+), 21 deletions(-)
> 
> v2 changes:
> * Removed error flag of 'ECIPE' processing only in both i40e PMD and mbuf. All
>   other error flags were added back.
> * Assigned error flags with correct and valid values, as their previous values
>   were invalid.
> * Enabled getting all error flag names.
> 
> v3 changes:
> * 'PKT_RX_ERR' is defined to replace error bit flags of MAC error, Oversize
>   error, header buffer overflow error.
> * Removed assigning values to PKT_TX_* bit flags, as it has already been done
>   in another packet set.
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 1b14e02..5e6b5d0 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -210,11 +210,8 @@ const char *rte_get_rx_ol_flag_name(uint64_t mask)
>  	case PKT_RX_FDIR: return "PKT_RX_FDIR";
>  	case PKT_RX_L4_CKSUM_BAD: return "PKT_RX_L4_CKSUM_BAD";
>  	case PKT_RX_IP_CKSUM_BAD: return "PKT_RX_IP_CKSUM_BAD";
> -	/* case PKT_RX_EIP_CKSUM_BAD: return "PKT_RX_EIP_CKSUM_BAD"; */
> -	/* case PKT_RX_OVERSIZE: return "PKT_RX_OVERSIZE"; */
> -	/* case PKT_RX_HBUF_OVERFLOW: return "PKT_RX_HBUF_OVERFLOW"; */
> -	/* case PKT_RX_RECIP_ERR: return "PKT_RX_RECIP_ERR"; */
> -	/* case PKT_RX_MAC_ERR: return "PKT_RX_MAC_ERR"; */
> +	case PKT_RX_EIP_CKSUM_BAD: return "PKT_RX_EIP_CKSUM_BAD";
> +	case PKT_RX_ERR: return "PKT_RX_ERR";
>  	case PKT_RX_IPV4_HDR: return "PKT_RX_IPV4_HDR";
>  	case PKT_RX_IPV4_HDR_EXT: return "PKT_RX_IPV4_HDR_EXT";
>  	case PKT_RX_IPV6_HDR: return "PKT_RX_IPV6_HDR";
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index efdefc4..5e647a9 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -84,11 +84,6 @@ extern "C" {
>  #define PKT_RX_FDIR          (1ULL << 2)  /**< RX packet with FDIR match indicate. */
>  #define PKT_RX_L4_CKSUM_BAD  (1ULL << 3)  /**< L4 cksum of RX pkt. is not OK. */
>  #define PKT_RX_IP_CKSUM_BAD  (1ULL << 4)  /**< IP cksum of RX pkt. is not OK. */
> -#define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**< External IP header checksum error. */
> -#define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX pkt oversize. */
> -#define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer overflow. */
> -#define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing error. */
> -#define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
>  #define PKT_RX_IPV4_HDR      (1ULL << 5)  /**< RX packet with IPv4 header. */
>  #define PKT_RX_IPV4_HDR_EXT  (1ULL << 6)  /**< RX packet with extended IPv4 header. */
>  #define PKT_RX_IPV6_HDR      (1ULL << 7)  /**< RX packet with IPv6 header. */
> @@ -99,6 +94,8 @@ extern "C" {
>  #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet with IPv6 header. */
>  #define PKT_RX_FDIR_ID       (1ULL << 13) /**< FD id reported if FDIR match. */
>  #define PKT_RX_FDIR_FLX      (1ULL << 14) /**< Flexible bytes reported if FDIR match. */
> +#define PKT_RX_EIP_CKSUM_BAD (1ULL << 15)  /**< External IP header checksum error. */
> +#define PKT_RX_ERR           (1ULL << 16)  /**< Other errors, e.g. MAC error. */
>  /* add new RX flags here */
> 
>  /* add new TX flags here */
> diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c
> index 2beae3c..5d99bd2 100644
> --- a/lib/librte_pmd_i40e/i40e_rxtx.c
> +++ b/lib/librte_pmd_i40e/i40e_rxtx.c
> @@ -123,25 +123,18 @@ i40e_rxd_error_to_pkt_flags(uint64_t qword)
>  		return flags;
>  	/* If RXE bit set, all other status bits are meaningless */
>  	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_RXE_SHIFT))) {
> -		flags |= PKT_RX_MAC_ERR;
> +		flags |= PKT_RX_ERR;
>  		return flags;
>  	}
> -
> -	/* If RECIPE bit set, all other status indications should be ignored */
> -	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_RECIPE_SHIFT))) {
> -		flags |= PKT_RX_RECIP_ERR;
> -		return flags;
> -	}
> -	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_HBO_SHIFT)))
> -		flags |= PKT_RX_HBUF_OVERFLOW;
>  	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_IPE_SHIFT)))
>  		flags |= PKT_RX_IP_CKSUM_BAD;
>  	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_L4E_SHIFT)))
>  		flags |= PKT_RX_L4_CKSUM_BAD;
>  	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_EIPE_SHIFT)))
>  		flags |= PKT_RX_EIP_CKSUM_BAD;
> -	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT)))
> -		flags |= PKT_RX_OVERSIZE;
> +	if (unlikely(error_bits & ((1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT) ||
> +					(1 << I40E_RX_DESC_ERROR_HBO_SHIFT))))
> +		flags |= PKT_RX_ERR;

It should be:
error_bits & (1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT | 1 << I40E_RX_DESC_ERROR_HBO_SHIFT)

Another 2 questions:
- Why not to put all these 3 bits processing together:
error_bits & (1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT | 1 << I40E_RX_DESC_ERROR_HBO_SHIFT | 1 << I40E_RX_DESC_ERROR_RXE_SHIFT)
?
- Is there any reason why you don't want to put PMD_RX_LOG(DEBUG, ...) for them?
I think it might be usefull for debugging.

Konstantin


> 
>  	return flags;
>  }
> --
> 1.9.3
  

Hi Helin,

2014-12-06 09:33, Helin Zhang:
> Before redefining mbuf structure, there was lack of free bits in 'ol_flags'
> (32 bits in total) for new RX or TX flags. So it tried to reuse existant
> bits as most as possible, or even assigning 0 to some of bit flags. After
> new mbuf structure defined, there are quite a lot of free bits. So those
> newly added bit flags should be assigned with correct and valid bit values,
> and getting their names should be enabled as well. Note that 'RECIP' should
> be removed, as nowhere will use it. 'PKT_RX_ERR' is defined to replace all
> other error bit flags, e.g. MAC error, Oversize error, header buffer
> overflow error.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
[...]
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -84,11 +84,6 @@ extern "C" {
>  #define PKT_RX_FDIR          (1ULL << 2)  /**< RX packet with FDIR match indicate. */
>  #define PKT_RX_L4_CKSUM_BAD  (1ULL << 3)  /**< L4 cksum of RX pkt. is not OK. */
>  #define PKT_RX_IP_CKSUM_BAD  (1ULL << 4)  /**< IP cksum of RX pkt. is not OK. */
> -#define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**< External IP header checksum error. */
> -#define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX pkt oversize. */
> -#define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer overflow. */
> -#define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing error. */
> -#define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
>  #define PKT_RX_IPV4_HDR      (1ULL << 5)  /**< RX packet with IPv4 header. */
>  #define PKT_RX_IPV4_HDR_EXT  (1ULL << 6)  /**< RX packet with extended IPv4 header. */
>  #define PKT_RX_IPV6_HDR      (1ULL << 7)  /**< RX packet with IPv6 header. */
> @@ -99,6 +94,8 @@ extern "C" {
>  #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet with IPv6 header. */
>  #define PKT_RX_FDIR_ID       (1ULL << 13) /**< FD id reported if FDIR match. */
>  #define PKT_RX_FDIR_FLX      (1ULL << 14) /**< Flexible bytes reported if FDIR match. */
> +#define PKT_RX_EIP_CKSUM_BAD (1ULL << 15)  /**< External IP header checksum error. */

Could PKT_RX_EIP_CKSUM_BAD be aggregated with PKT_RX_IP_CKSUM_BAD?
The conclusion is the same: the packet is corrupted.
And some hardwares could not detect the encapsulation and use PKT_RX_IP_CKSUM_BAD.

Another interesting improvement would be to have PKT_RX_IP_CKSUM_OK.
I think we'll have to think about this kind of flag for next version.

Note that this patch is an API change and shouldn't be applied for 1.8.0.
But we can do an exception as it has no impact on existing applications and
fixes the 0 flags.
  

Hi Thomas

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, December 8, 2014 6:51 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] mbuf: fix of enabling all newly added RX
> error flags
> 
> Hi Helin,
> 
> 2014-12-06 09:33, Helin Zhang:
> > Before redefining mbuf structure, there was lack of free bits in 'ol_flags'
> > (32 bits in total) for new RX or TX flags. So it tried to reuse
> > existant bits as most as possible, or even assigning 0 to some of bit
> > flags. After new mbuf structure defined, there are quite a lot of free
> > bits. So those newly added bit flags should be assigned with correct
> > and valid bit values, and getting their names should be enabled as
> > well. Note that 'RECIP' should be removed, as nowhere will use it.
> > 'PKT_RX_ERR' is defined to replace all other error bit flags, e.g. MAC
> > error, Oversize error, header buffer overflow error.
> >
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> [...]
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -84,11 +84,6 @@ extern "C" {
> >  #define PKT_RX_FDIR          (1ULL << 2)  /**< RX packet with FDIR
> match indicate. */
> >  #define PKT_RX_L4_CKSUM_BAD  (1ULL << 3)  /**< L4 cksum of RX pkt.
> is
> > not OK. */  #define PKT_RX_IP_CKSUM_BAD  (1ULL << 4)  /**< IP cksum
> of
> > RX pkt. is not OK. */ -#define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**<
> External IP header checksum error. */
> > -#define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX
> pkt oversize. */
> > -#define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer
> overflow. */
> > -#define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing
> error. */
> > -#define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
> >  #define PKT_RX_IPV4_HDR      (1ULL << 5)  /**< RX packet with IPv4
> header. */
> >  #define PKT_RX_IPV4_HDR_EXT  (1ULL << 6)  /**< RX packet with
> extended IPv4 header. */
> >  #define PKT_RX_IPV6_HDR      (1ULL << 7)  /**< RX packet with IPv6
> header. */
> > @@ -99,6 +94,8 @@ extern "C" {
> >  #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet
> with IPv6 header. */
> >  #define PKT_RX_FDIR_ID       (1ULL << 13) /**< FD id reported if FDIR
> match. */
> >  #define PKT_RX_FDIR_FLX      (1ULL << 14) /**< Flexible bytes reported
> if FDIR match. */
> > +#define PKT_RX_EIP_CKSUM_BAD (1ULL << 15)  /**< External IP header
> > +checksum error. */
> 
> Could PKT_RX_EIP_CKSUM_BAD be aggregated with
> PKT_RX_IP_CKSUM_BAD?
I tend to say no, but I would listen to comments from others.
For tunneling case (e.g. IP over IP), it is a bit similar to the case of L3/L4 (e.g. UDP over IP).
For L3/L4 case, we have PKT_RX_IP_CKSUM_BAD and PKT_RX_L4_CKSUM_BAD to
indicate the checksum error is in L3 or L4.
So I'd prefer to have PKT_RX_IP_CKSUM_BAD and PKT_RX_EIP_CKSUM_BAD to indicate
the checksum error is in outer or inner header.
Otherwise we have no chance to know where the checksum error is, based on mbuf.

> The conclusion is the same: the packet is corrupted.
> And some hardwares could not detect the encapsulation and use
> PKT_RX_IP_CKSUM_BAD.
If the hardware don't know it is a tunneling packet, it will just treat it as an IP packet. But if
hardware supports tunneling, it would be better for apps to know that more about the
packet which can be offloaded by hardware.

> 
> Another interesting improvement would be to have PKT_RX_IP_CKSUM_OK.
> I think we'll have to think about this kind of flag for next version.
For checksum OK, if no 'BAD' indicated, we can assume it is OK. Any other hints from you?

> 
> Note that this patch is an API change and shouldn't be applied for 1.8.0.
> But we can do an exception as it has no impact on existing applications and
> fixes the 0 flags.
Agree with you!

> 
> --
> Thomas

Thank you very much for thinking so much about this!

Regards,
Helin
  

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, December 8, 2014 6:45 PM
> To: Zhang, Helin; dev@dpdk.org
> Cc: Cao, Waterman; Cao, Min; olivier.matz@6wind.com
> Subject: RE: [PATCH v3] mbuf: fix of enabling all newly added RX error flags
> 
> Hi Helin,
> 
> > -----Original Message-----
> > From: Zhang, Helin
> > Sent: Saturday, December 06, 2014 1:34 AM
> > To: dev@dpdk.org
> > Cc: Cao, Waterman; Cao, Min; olivier.matz@6wind.com; Ananyev,
> > Konstantin; Zhang, Helin
> > Subject: [PATCH v3] mbuf: fix of enabling all newly added RX error
> > flags
> >
> > Before redefining mbuf structure, there was lack of free bits in 'ol_flags'
> > (32 bits in total) for new RX or TX flags. So it tried to reuse
> > existant bits as most as possible, or even assigning 0 to some of bit
> > flags. After new mbuf structure defined, there are quite a lot of free
> > bits. So those newly added bit flags should be assigned with correct
> > and valid bit values, and getting their names should be enabled as
> > well. Note that 'RECIP' should be removed, as nowhere will use it.
> > 'PKT_RX_ERR' is defined to replace all other error bit flags, e.g. MAC
> > error, Oversize error, header buffer overflow error.
> >
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> > ---
> >  lib/librte_mbuf/rte_mbuf.c      |  7 ++-----
> >  lib/librte_mbuf/rte_mbuf.h      |  7 ++-----
> >  lib/librte_pmd_i40e/i40e_rxtx.c | 15 ++++-----------
> >  3 files changed, 8 insertions(+), 21 deletions(-)
> >
> > v2 changes:
> > * Removed error flag of 'ECIPE' processing only in both i40e PMD and mbuf.
> All
> >   other error flags were added back.
> > * Assigned error flags with correct and valid values, as their previous values
> >   were invalid.
> > * Enabled getting all error flag names.
> >
> > v3 changes:
> > * 'PKT_RX_ERR' is defined to replace error bit flags of MAC error, Oversize
> >   error, header buffer overflow error.
> > * Removed assigning values to PKT_TX_* bit flags, as it has already been
> done
> >   in another packet set.
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 1b14e02..5e6b5d0 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -210,11 +210,8 @@ const char *rte_get_rx_ol_flag_name(uint64_t
> mask)
> >  	case PKT_RX_FDIR: return "PKT_RX_FDIR";
> >  	case PKT_RX_L4_CKSUM_BAD: return "PKT_RX_L4_CKSUM_BAD";
> >  	case PKT_RX_IP_CKSUM_BAD: return "PKT_RX_IP_CKSUM_BAD";
> > -	/* case PKT_RX_EIP_CKSUM_BAD: return "PKT_RX_EIP_CKSUM_BAD"; */
> > -	/* case PKT_RX_OVERSIZE: return "PKT_RX_OVERSIZE"; */
> > -	/* case PKT_RX_HBUF_OVERFLOW: return "PKT_RX_HBUF_OVERFLOW";
> */
> > -	/* case PKT_RX_RECIP_ERR: return "PKT_RX_RECIP_ERR"; */
> > -	/* case PKT_RX_MAC_ERR: return "PKT_RX_MAC_ERR"; */
> > +	case PKT_RX_EIP_CKSUM_BAD: return "PKT_RX_EIP_CKSUM_BAD";
> > +	case PKT_RX_ERR: return "PKT_RX_ERR";
> >  	case PKT_RX_IPV4_HDR: return "PKT_RX_IPV4_HDR";
> >  	case PKT_RX_IPV4_HDR_EXT: return "PKT_RX_IPV4_HDR_EXT";
> >  	case PKT_RX_IPV6_HDR: return "PKT_RX_IPV6_HDR"; diff --git
> > a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h index
> > efdefc4..5e647a9 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -84,11 +84,6 @@ extern "C" {
> >  #define PKT_RX_FDIR          (1ULL << 2)  /**< RX packet with FDIR
> match indicate. */
> >  #define PKT_RX_L4_CKSUM_BAD  (1ULL << 3)  /**< L4 cksum of RX pkt.
> is
> > not OK. */  #define PKT_RX_IP_CKSUM_BAD  (1ULL << 4)  /**< IP cksum
> of
> > RX pkt. is not OK. */ -#define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**<
> External IP header checksum error. */
> > -#define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX
> pkt oversize. */
> > -#define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer
> overflow. */
> > -#define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing
> error. */
> > -#define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
> >  #define PKT_RX_IPV4_HDR      (1ULL << 5)  /**< RX packet with IPv4
> header. */
> >  #define PKT_RX_IPV4_HDR_EXT  (1ULL << 6)  /**< RX packet with
> extended IPv4 header. */
> >  #define PKT_RX_IPV6_HDR      (1ULL << 7)  /**< RX packet with IPv6
> header. */
> > @@ -99,6 +94,8 @@ extern "C" {
> >  #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet
> with IPv6 header. */
> >  #define PKT_RX_FDIR_ID       (1ULL << 13) /**< FD id reported if FDIR
> match. */
> >  #define PKT_RX_FDIR_FLX      (1ULL << 14) /**< Flexible bytes reported
> if FDIR match. */
> > +#define PKT_RX_EIP_CKSUM_BAD (1ULL << 15)  /**< External IP header
> checksum error. */
> > +#define PKT_RX_ERR           (1ULL << 16)  /**< Other errors, e.g.
> MAC error. */
> >  /* add new RX flags here */
> >
> >  /* add new TX flags here */
> > diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c
> > b/lib/librte_pmd_i40e/i40e_rxtx.c index 2beae3c..5d99bd2 100644
> > --- a/lib/librte_pmd_i40e/i40e_rxtx.c
> > +++ b/lib/librte_pmd_i40e/i40e_rxtx.c
> > @@ -123,25 +123,18 @@ i40e_rxd_error_to_pkt_flags(uint64_t qword)
> >  		return flags;
> >  	/* If RXE bit set, all other status bits are meaningless */
> >  	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_RXE_SHIFT))) {
> > -		flags |= PKT_RX_MAC_ERR;
> > +		flags |= PKT_RX_ERR;
> >  		return flags;
> >  	}
> > -
> > -	/* If RECIPE bit set, all other status indications should be ignored */
> > -	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_RECIPE_SHIFT))) {
> > -		flags |= PKT_RX_RECIP_ERR;
> > -		return flags;
> > -	}
> > -	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_HBO_SHIFT)))
> > -		flags |= PKT_RX_HBUF_OVERFLOW;
> >  	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_IPE_SHIFT)))
> >  		flags |= PKT_RX_IP_CKSUM_BAD;
> >  	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_L4E_SHIFT)))
> >  		flags |= PKT_RX_L4_CKSUM_BAD;
> >  	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_EIPE_SHIFT)))
> >  		flags |= PKT_RX_EIP_CKSUM_BAD;
> > -	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT)))
> > -		flags |= PKT_RX_OVERSIZE;
> > +	if (unlikely(error_bits & ((1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT) ||
> > +					(1 << I40E_RX_DESC_ERROR_HBO_SHIFT))))
> > +		flags |= PKT_RX_ERR;
> 
> It should be:
> error_bits & (1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT | 1 <<
> I40E_RX_DESC_ERROR_HBO_SHIFT)
Ohm, stupid fault! I will fix it right now.

> 
> Another 2 questions:
> - Why not to put all these 3 bits processing together:
> error_bits & (1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT | 1 <<
> I40E_RX_DESC_ERROR_HBO_SHIFT | 1 <<
> I40E_RX_DESC_ERROR_RXE_SHIFT) ?
If RECIPE, other bits are meaningless, said by EAS.
If HBO or OVERSIZE, IPE, L4E, EIPE could be meaningful correct or not.

> - Is there any reason why you don't want to put PMD_RX_LOG(DEBUG, ...) for
> them?
> I think it might be usefull for debugging.
For all error bits or just HBO and OVERSIZE?

> 
> Konstantin
> 
> 
> >
> >  	return flags;
> >  }
> > --
> > 1.9.3

Regards,
Helin
  

2014-12-09 02:14, Zhang, Helin:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2014-12-06 09:33, Helin Zhang:
> > > Before redefining mbuf structure, there was lack of free bits in 'ol_flags'
> > > (32 bits in total) for new RX or TX flags. So it tried to reuse
> > > existant bits as most as possible, or even assigning 0 to some of bit
> > > flags. After new mbuf structure defined, there are quite a lot of free
> > > bits. So those newly added bit flags should be assigned with correct
> > > and valid bit values, and getting their names should be enabled as
> > > well. Note that 'RECIP' should be removed, as nowhere will use it.
> > > 'PKT_RX_ERR' is defined to replace all other error bit flags, e.g. MAC
> > > error, Oversize error, header buffer overflow error.
> > >
> > > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> > [...]
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -84,11 +84,6 @@ extern "C" {
> > >  #define PKT_RX_FDIR          (1ULL << 2)  /**< RX packet with FDIR
> > match indicate. */
> > >  #define PKT_RX_L4_CKSUM_BAD  (1ULL << 3)  /**< L4 cksum of RX pkt.
> > is
> > > not OK. */  #define PKT_RX_IP_CKSUM_BAD  (1ULL << 4)  /**< IP cksum
> > of
> > > RX pkt. is not OK. */ -#define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**<
> > External IP header checksum error. */
> > > -#define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX
> > pkt oversize. */
> > > -#define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer
> > overflow. */
> > > -#define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing
> > error. */
> > > -#define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
> > >  #define PKT_RX_IPV4_HDR      (1ULL << 5)  /**< RX packet with IPv4
> > header. */
> > >  #define PKT_RX_IPV4_HDR_EXT  (1ULL << 6)  /**< RX packet with
> > extended IPv4 header. */
> > >  #define PKT_RX_IPV6_HDR      (1ULL << 7)  /**< RX packet with IPv6
> > header. */
> > > @@ -99,6 +94,8 @@ extern "C" {
> > >  #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet
> > with IPv6 header. */
> > >  #define PKT_RX_FDIR_ID       (1ULL << 13) /**< FD id reported if FDIR
> > match. */
> > >  #define PKT_RX_FDIR_FLX      (1ULL << 14) /**< Flexible bytes reported
> > if FDIR match. */
> > > +#define PKT_RX_EIP_CKSUM_BAD (1ULL << 15)  /**< External IP header
> > > +checksum error. */
> > 
> > Could PKT_RX_EIP_CKSUM_BAD be aggregated with
> > PKT_RX_IP_CKSUM_BAD?
> 
> I tend to say no, but I would listen to comments from others.
> For tunneling case (e.g. IP over IP), it is a bit similar to the case of L3/L4 (e.g. UDP over IP).
> For L3/L4 case, we have PKT_RX_IP_CKSUM_BAD and PKT_RX_L4_CKSUM_BAD to
> indicate the checksum error is in L3 or L4.
> So I'd prefer to have PKT_RX_IP_CKSUM_BAD and PKT_RX_EIP_CKSUM_BAD to indicate
> the checksum error is in outer or inner header.

I think OUTER_IP would be more consistent than EIP.

> Otherwise we have no chance to know where the checksum error is, based on mbuf.
> 
> > The conclusion is the same: the packet is corrupted.
> > And some hardwares could not detect the encapsulation and use
> > PKT_RX_IP_CKSUM_BAD.
> 
> If the hardware don't know it is a tunneling packet, it will just treat it as an IP packet. But if
> hardware supports tunneling, it would be better for apps to know that more about the
> packet which can be offloaded by hardware.
> 
> > 
> > Another interesting improvement would be to have PKT_RX_IP_CKSUM_OK.
> > I think we'll have to think about this kind of flag for next version.
> 
> For checksum OK, if no 'BAD' indicated, we can assume it is OK. Any other hints from you?

No, having no BAD can indicate also that it hasn't been checked (i.e. check not
enabled or not supported).

> > Note that this patch is an API change and shouldn't be applied for 1.8.0.
> > But we can do an exception as it has no impact on existing applications and
> > fixes the 0 flags.
> Agree with you!
> 
> Thank you very much for thinking so much about this!
> 
> Regards,
> Helin