[PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[Helin Zhang]

There were some bit flags of 0 for RX packet errors detected by hardware.
Actually only one bit of error flag is enough for all hardware detected
RX packet errors.

Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_mbuf/rte_mbuf.h      |  6 +-----
 lib/librte_pmd_i40e/i40e_rxtx.c | 31 +++----------------------------
 2 files changed, 4 insertions(+), 33 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 5899e5c..897fd26 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -80,11 +80,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. */
@@ -95,6 +90,7 @@ 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_ERR_HW        (1ULL << 15) /**< RX packet error detected by hardware. */
 
 #define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
 #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt. computed by NIC. */
diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c
index cce6911..3b2195d 100644
--- a/lib/librte_pmd_i40e/i40e_rxtx.c
+++ b/lib/librte_pmd_i40e/i40e_rxtx.c
@@ -115,35 +115,10 @@ i40e_rxd_status_to_pkt_flags(uint64_t qword)
 static inline uint64_t
 i40e_rxd_error_to_pkt_flags(uint64_t qword)
 {
-	uint64_t flags = 0;
-	uint64_t error_bits = (qword >> I40E_RXD_QW1_ERROR_SHIFT);
-
-#define I40E_RX_ERR_BITS 0x3f
-	if (likely((error_bits & I40E_RX_ERR_BITS) == 0))
-		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;
-		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(qword & I40E_RXD_QW1_ERROR_MASK))
+		return PKT_RX_ERR_HW;
 
-	return flags;
+	return 0;
 }
 
 /* Translate pkt types to pkt flags */
-- 
1.8.1.4

Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[Ananyev, Konstantin]

Hi Helin,

> -----Original Message-----
> From: Zhang, Helin
> Sent: Wednesday, November 26, 2014 6:07 AM
> To: dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min; Ananyev, Konstantin; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; Zhang, Helin
> Subject: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors
> 
> There were some bit flags of 0 for RX packet errors detected by hardware.
> Actually only one bit of error flag is enough for all hardware detected
> RX packet errors.
> 
> Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  lib/librte_mbuf/rte_mbuf.h      |  6 +-----
>  lib/librte_pmd_i40e/i40e_rxtx.c | 31 +++----------------------------
>  2 files changed, 4 insertions(+), 33 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 5899e5c..897fd26 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -80,11 +80,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. */
> @@ -95,6 +90,7 @@ 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_ERR_HW        (1ULL << 15) /**< RX packet error detected by hardware. */
> 
>  #define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
>  #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt. computed by NIC. */
> diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c
> index cce6911..3b2195d 100644
> --- a/lib/librte_pmd_i40e/i40e_rxtx.c
> +++ b/lib/librte_pmd_i40e/i40e_rxtx.c
> @@ -115,35 +115,10 @@ i40e_rxd_status_to_pkt_flags(uint64_t qword)
>  static inline uint64_t
>  i40e_rxd_error_to_pkt_flags(uint64_t qword)
>  {
> -	uint64_t flags = 0;
> -	uint64_t error_bits = (qword >> I40E_RXD_QW1_ERROR_SHIFT);
> -
> -#define I40E_RX_ERR_BITS 0x3f
> -	if (likely((error_bits & I40E_RX_ERR_BITS) == 0))
> -		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;
> -		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(qword & I40E_RXD_QW1_ERROR_MASK))
> +		return PKT_RX_ERR_HW;

Probably I didn't explain myself clear enough, sorry.
I didn't suggest to get rid of setting bits that indicate L3/L4 checksum errors:
PKT_RX_IP_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD, PKT_RX_EIP_CKSUM_BAD.
I think these flags should be set as before.

I was talking only about collapsing only these 4 RX error flags into one:

#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. */ 

>From my point of view the difference of these 2 groups are:
First - HW was able to receive whole packet without a problem, but L3/L4 checksum check failed.

Second - HW was not able to receive whole packet properly by whatever reason. 
>From upper layer SW perspective - there it probably makes little difference, what caused it,
as most likely SW has to throw away erroneous packet. 
And for debugging purposes, we can add PMD_LOG(DEBUG, ...) that would print what exactly HW error happened.

Thanks
Konstantin

> 
> -	return flags;
> +	return 0;
>  }
> 
>  /* Translate pkt types to pkt flags */
> --
> 1.8.1.4

Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[Olivier MATZ]

Hi Konstantin, Hi Helin,

On 11/26/2014 11:49 AM, Ananyev, Konstantin wrote:
> Hi Helin,
>
>> -----Original Message-----
>> From: Zhang, Helin
>> Sent: Wednesday, November 26, 2014 6:07 AM
>> To: dev-VfR2kkLFssw@public.gmane.org
>> Cc: Cao, Waterman; Cao, Min; Ananyev, Konstantin; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; Zhang, Helin
>> Subject: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors
>>
>> There were some bit flags of 0 for RX packet errors detected by hardware.
>> Actually only one bit of error flag is enough for all hardware detected
>> RX packet errors.
>>
>> Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>   lib/librte_mbuf/rte_mbuf.h      |  6 +-----
>>   lib/librte_pmd_i40e/i40e_rxtx.c | 31 +++----------------------------
>>   2 files changed, 4 insertions(+), 33 deletions(-)
>>
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index 5899e5c..897fd26 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -80,11 +80,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. */
>> @@ -95,6 +90,7 @@ 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_ERR_HW        (1ULL << 15) /**< RX packet error detected by hardware. */
>>
>>   #define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
>>   #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt. computed by NIC. */
>> diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c
>> index cce6911..3b2195d 100644
>> --- a/lib/librte_pmd_i40e/i40e_rxtx.c
>> +++ b/lib/librte_pmd_i40e/i40e_rxtx.c
>> @@ -115,35 +115,10 @@ i40e_rxd_status_to_pkt_flags(uint64_t qword)
>>   static inline uint64_t
>>   i40e_rxd_error_to_pkt_flags(uint64_t qword)
>>   {
>> -	uint64_t flags = 0;
>> -	uint64_t error_bits = (qword >> I40E_RXD_QW1_ERROR_SHIFT);
>> -
>> -#define I40E_RX_ERR_BITS 0x3f
>> -	if (likely((error_bits & I40E_RX_ERR_BITS) == 0))
>> -		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;
>> -		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(qword & I40E_RXD_QW1_ERROR_MASK))
>> +		return PKT_RX_ERR_HW;
>
> Probably I didn't explain myself clear enough, sorry.
> I didn't suggest to get rid of setting bits that indicate L3/L4 checksum errors:
> PKT_RX_IP_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD, PKT_RX_EIP_CKSUM_BAD.
> I think these flags should be set as before.
>
> I was talking only about collapsing only these 4 RX error flags into one:
>
> #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. */
>
>  From my point of view the difference of these 2 groups are:
> First - HW was able to receive whole packet without a problem, but L3/L4 checksum check failed.
>
> Second - HW was not able to receive whole packet properly by whatever reason.
>  From upper layer SW perspective - there it probably makes little difference, what caused it,
> as most likely SW has to throw away erroneous packet.
> And for debugging purposes, we can add PMD_LOG(DEBUG, ...) that would print what exactly HW error happened.

I agree with Konstantin that there are 2 different cases:

a) the packet is properly received by the hardware, but has a bad
    checksum (or another protocol error, for instance an invalid ip len,
    a ip_version == 8 :))

    in this case, it is useful to the application to have the mbuf with
    the data + an error flag. Then using a tcpdump-like tool could help
    to debug what is the cause of the error and what equipment generates
    a bad packet.

b) the packet is not properly received by the hardware. In this case
    the data is invalid in the mbuf and not useable by the application.
    I suggest to only have a stats counter in this case, as receiving the
    mbuf is cpu time consuming and the only thing the application can do
    is to drop the packet.

Regards,
Olivier

Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[Ananyev, Konstantin]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Wednesday, November 26, 2014 11:22 AM
> To: Ananyev, Konstantin; Zhang, Helin; dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min
> Subject: Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors
> 
> Hi Konstantin, Hi Helin,
> 
> On 11/26/2014 11:49 AM, Ananyev, Konstantin wrote:
> > Hi Helin,
> >
> >> -----Original Message-----
> >> From: Zhang, Helin
> >> Sent: Wednesday, November 26, 2014 6:07 AM
> >> To: dev-VfR2kkLFssw@public.gmane.org
> >> Cc: Cao, Waterman; Cao, Min; Ananyev, Konstantin; olivier.matz@6wind.com; Zhang, Helin
> >> Subject: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors
> >>
> >> There were some bit flags of 0 for RX packet errors detected by hardware.
> >> Actually only one bit of error flag is enough for all hardware detected
> >> RX packet errors.
> >>
> >> Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >> ---
> >>   lib/librte_mbuf/rte_mbuf.h      |  6 +-----
> >>   lib/librte_pmd_i40e/i40e_rxtx.c | 31 +++----------------------------
> >>   2 files changed, 4 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >> index 5899e5c..897fd26 100644
> >> --- a/lib/librte_mbuf/rte_mbuf.h
> >> +++ b/lib/librte_mbuf/rte_mbuf.h
> >> @@ -80,11 +80,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. */
> >> @@ -95,6 +90,7 @@ 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_ERR_HW        (1ULL << 15) /**< RX packet error detected by hardware. */
> >>
> >>   #define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
> >>   #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt. computed by NIC. */
> >> diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c
> >> index cce6911..3b2195d 100644
> >> --- a/lib/librte_pmd_i40e/i40e_rxtx.c
> >> +++ b/lib/librte_pmd_i40e/i40e_rxtx.c
> >> @@ -115,35 +115,10 @@ i40e_rxd_status_to_pkt_flags(uint64_t qword)
> >>   static inline uint64_t
> >>   i40e_rxd_error_to_pkt_flags(uint64_t qword)
> >>   {
> >> -	uint64_t flags = 0;
> >> -	uint64_t error_bits = (qword >> I40E_RXD_QW1_ERROR_SHIFT);
> >> -
> >> -#define I40E_RX_ERR_BITS 0x3f
> >> -	if (likely((error_bits & I40E_RX_ERR_BITS) == 0))
> >> -		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;
> >> -		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(qword & I40E_RXD_QW1_ERROR_MASK))
> >> +		return PKT_RX_ERR_HW;
> >
> > Probably I didn't explain myself clear enough, sorry.
> > I didn't suggest to get rid of setting bits that indicate L3/L4 checksum errors:
> > PKT_RX_IP_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD, PKT_RX_EIP_CKSUM_BAD.
> > I think these flags should be set as before.
> >
> > I was talking only about collapsing only these 4 RX error flags into one:
> >
> > #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. */
> >
> >  From my point of view the difference of these 2 groups are:
> > First - HW was able to receive whole packet without a problem, but L3/L4 checksum check failed.
> >
> > Second - HW was not able to receive whole packet properly by whatever reason.
> >  From upper layer SW perspective - there it probably makes little difference, what caused it,
> > as most likely SW has to throw away erroneous packet.
> > And for debugging purposes, we can add PMD_LOG(DEBUG, ...) that would print what exactly HW error happened.
> 
> I agree with Konstantin that there are 2 different cases:
> 
> a) the packet is properly received by the hardware, but has a bad
>     checksum (or another protocol error, for instance an invalid ip len,
>     a ip_version == 8 :))
> 
>     in this case, it is useful to the application to have the mbuf with
>     the data + an error flag. Then using a tcpdump-like tool could help
>     to debug what is the cause of the error and what equipment generates
>     a bad packet.
> 
> b) the packet is not properly received by the hardware. In this case
>     the data is invalid in the mbuf and not useable by the application.
>     I suggest to only have a stats counter in this case, as receiving the
>     mbuf is cpu time consuming and the only thing the application can do
>     is to drop the packet.

So for b) you suggest to drop the packet straight in PMD RX function?
Something like:
if (unlikely(error_bits & ...)) {
        PMD_LOG(DEBUG, ...);
         rte_pktmbuf_free(mb);
}
?

That's probably a bit too radical. 
Yes, mbuf doesn't contain the whole packet, but it may contain at least part of it, let say in case of 'packet oversize'. 
So for debugging purposes the user may still like to examine the mbuf contents.

Konstantin

> 
> Regards,
> Olivier

Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[Olivier MATZ]

Hi Konstantin,

On 11/26/2014 02:38 PM, Ananyev, Konstantin wrote:
>>> Probably I didn't explain myself clear enough, sorry.
>>> I didn't suggest to get rid of setting bits that indicate L3/L4 checksum errors:
>>> PKT_RX_IP_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD, PKT_RX_EIP_CKSUM_BAD.
>>> I think these flags should be set as before.
>>>
>>> I was talking only about collapsing only these 4 RX error flags into one:
>>>
>>> #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. */
>>>
>>>   From my point of view the difference of these 2 groups are:
>>> First - HW was able to receive whole packet without a problem, but L3/L4 checksum check failed.
>>>
>>> Second - HW was not able to receive whole packet properly by whatever reason.
>>>   From upper layer SW perspective - there it probably makes little difference, what caused it,
>>> as most likely SW has to throw away erroneous packet.
>>> And for debugging purposes, we can add PMD_LOG(DEBUG, ...) that would print what exactly HW error happened.
>>
>> I agree with Konstantin that there are 2 different cases:
>>
>> a) the packet is properly received by the hardware, but has a bad
>>      checksum (or another protocol error, for instance an invalid ip len,
>>      a ip_version == 8 :))
>>
>>      in this case, it is useful to the application to have the mbuf with
>>      the data + an error flag. Then using a tcpdump-like tool could help
>>      to debug what is the cause of the error and what equipment generates
>>      a bad packet.
>>
>> b) the packet is not properly received by the hardware. In this case
>>      the data is invalid in the mbuf and not useable by the application.
>>      I suggest to only have a stats counter in this case, as receiving the
>>      mbuf is cpu time consuming and the only thing the application can do
>>      is to drop the packet.
>
> So for b) you suggest to drop the packet straight in PMD RX function?
> Something like:
> if (unlikely(error_bits & ...)) {
>          PMD_LOG(DEBUG, ...);
>           rte_pktmbuf_free(mb);
> }
> ?

Yes

> That's probably a bit too radical.
> Yes, mbuf doesn't contain the whole packet, but it may contain at least part of it, let say in case of 'packet oversize'.
> So for debugging purposes the user may still like to examine the mbuf contents.

As soon as there is some exploitable data in the mbuf, I agree it can
be transfered to the application (ex: bad header, bad len, bad
checksum...).

But if the hardware is not able to provide any exploitable data, it
looks a bit overkill to give an mbuf with an error flag.

But grouping the flags as you suggest is already a good clean-up to me,
I don't want to be more catholic than the Pope ;)

Regards,
Olivier

Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[Zhang, Helin]

Hi Olivier, Konstantin

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Wednesday, November 26, 2014 10:12 PM
> To: Ananyev, Konstantin; Zhang, Helin; dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min
> Subject: Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet
> errors
> 
> Hi Konstantin,
> 
> On 11/26/2014 02:38 PM, Ananyev, Konstantin wrote:
> >>> Probably I didn't explain myself clear enough, sorry.
> >>> I didn't suggest to get rid of setting bits that indicate L3/L4 checksum errors:
> >>> PKT_RX_IP_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD,
> PKT_RX_EIP_CKSUM_BAD.
> >>> I think these flags should be set as before.
> >>>
> >>> I was talking only about collapsing only these 4 RX error flags into one:
> >>>
> >>> #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. */
> >>>
> >>>   From my point of view the difference of these 2 groups are:
> >>> First - HW was able to receive whole packet without a problem, but L3/L4
> checksum check failed.
> >>>
> >>> Second - HW was not able to receive whole packet properly by whatever
> reason.
> >>>   From upper layer SW perspective - there it probably makes little
> >>> difference, what caused it, as most likely SW has to throw away erroneous
> packet.
> >>> And for debugging purposes, we can add PMD_LOG(DEBUG, ...) that would
> print what exactly HW error happened.
> >>
> >> I agree with Konstantin that there are 2 different cases:
> >>
> >> a) the packet is properly received by the hardware, but has a bad
> >>      checksum (or another protocol error, for instance an invalid ip len,
> >>      a ip_version == 8 :))
> >>
> >>      in this case, it is useful to the application to have the mbuf with
> >>      the data + an error flag. Then using a tcpdump-like tool could help
> >>      to debug what is the cause of the error and what equipment generates
> >>      a bad packet.
> >>
> >> b) the packet is not properly received by the hardware. In this case
> >>      the data is invalid in the mbuf and not useable by the application.
> >>      I suggest to only have a stats counter in this case, as receiving the
> >>      mbuf is cpu time consuming and the only thing the application can do
> >>      is to drop the packet.
> >
> > So for b) you suggest to drop the packet straight in PMD RX function?
> > Something like:
> > if (unlikely(error_bits & ...)) {
> >          PMD_LOG(DEBUG, ...);
> >           rte_pktmbuf_free(mb);
> > }
> > ?
> 
> Yes
> 
> > That's probably a bit too radical.
> > Yes, mbuf doesn't contain the whole packet, but it may contain at least part of it,
> let say in case of 'packet oversize'.
> > So for debugging purposes the user may still like to examine the mbuf
> contents.
> 
> As soon as there is some exploitable data in the mbuf, I agree it can be transfered
> to the application (ex: bad header, bad len, bad checksum...).
> 
> But if the hardware is not able to provide any exploitable data, it looks a bit
> overkill to give an mbuf with an error flag.
> 
> But grouping the flags as you suggest is already a good clean-up to me, I don't
> want to be more catholic than the Pope ;)

After I have completed another task, I read the datasheet carefully again. For those 5
error bits I introduced for a long time, I'd like to explain one by one as below.

#define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**< External IP header checksum error. */
[Helin] Nobody complains it, so we will keep it there, and just assign a new value to it.

#define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX pkt oversize. */
[Helin] I don't think it can be merge with other hardware errors. It indicates the packet
received needs more descriptors than hardware allowed, and the part of packets can
still be stored in the mbufs provided. It is a good hint for users that larger size of mbuf
might be needed. If just put it as hardware error, users will lose this information. So I
prefer to keep it there, and just assign a new value to it.

#define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer overflow. */
[Helin] It indicates the header buff size is not enough, but not means hardware cannot
process the packet received. It is a good hint for the users to provide larger size of header
buffers. I also prefer to keep it there, and just assign new value to it.

#define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing error. */
[Helin] In the latest data sheet, it is not opened to external users. So we can just remove
it from here.

#define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
[Helin] This indicates a real hardware error happens.

So my point is to just remove PKT_RX_RECIP_ERR, and we still need other new bit flags.
Any thought from you guys?

Regards,
Helin

> 
> Regards,
> Olivier

Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[Olivier MATZ]

Hi Helin,

On 11/28/2014 09:07 AM, Zhang, Helin wrote:
> After I have completed another task, I read the datasheet carefully again. For those 5
> error bits I introduced for a long time, I'd like to explain one by one as below.
> 
> #define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**< External IP header checksum error. */
> [Helin] Nobody complains it, so we will keep it there, and just assign a new value to it.

ok.

But it would be nice to have a better definition of this flag: does
external mean outer header? For instance, when you receive a
Ether/IP1/UDP/vxlan/Ether/IP2/xxx, does the flag concerns IP1 or IP2?

If it's IP1, it's really strange compared to the current behavior (the
flag PKT_RX_IP_CKSUM_BAD refers to IP1).

> #define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX pkt oversize. */
> [Helin] I don't think it can be merge with other hardware errors. It indicates the packet
> received needs more descriptors than hardware allowed, and the part of packets can
> still be stored in the mbufs provided. It is a good hint for users that larger size of mbuf
> might be needed. If just put it as hardware error, users will lose this information. So I
> prefer to keep it there, and just assign a new value to it.

Again, a statistic counter would do the job which if it's just to
provide a hint to the application.

I wonder in which case this flag can happen. If you fill the ring with
mbufs that are large enough compared to your ethernet network, this
should not happen in normal conditions. I really don't believe that
an application receiving an mbuf with this flag would stop the driver,
then refill the rings it with larger mbufs.

Last but not least: If it's really useful, should we have the same
behavior on other drivers like ixgbe? I think we really need to care
about not having different ways to use the different drivers.

To me, the only argument in favor of keeping this flag is when the mbuf
contains a part of the data that could be dumped by a user for debug
purposes.

> #define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer overflow. */
> [Helin] It indicates the header buff size is not enough, but not means hardware cannot
> process the packet received. It is a good hint for the users to provide larger size of header
> buffers. I also prefer to keep it there, and just assign new value to it.

Same for this one.

> #define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing error. */
> [Helin] In the latest data sheet, it is not opened to external users. So we can just remove
> it from here.

ok

> #define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
> [Helin] This indicates a real hardware error happens.

And what is the content of the mbuf data in this case? Does the
application really need an mbuf?


Regards,
Olivier

Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[Zhang, Helin]

Hi Olivier

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Friday, November 28, 2014 4:48 PM
> To: Zhang, Helin; Ananyev, Konstantin; dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min
> Subject: Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet
> errors
> 
> Hi Helin,
> 
> On 11/28/2014 09:07 AM, Zhang, Helin wrote:
> > After I have completed another task, I read the datasheet carefully
> > again. For those 5 error bits I introduced for a long time, I'd like to explain one
> by one as below.
> >
> > #define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**< External IP header
> > checksum error. */ [Helin] Nobody complains it, so we will keep it there, and
> just assign a new value to it.
> 
> ok.
> 
> But it would be nice to have a better definition of this flag: does external mean
> outer header? For instance, when you receive a
> Ether/IP1/UDP/vxlan/Ether/IP2/xxx, does the flag concerns IP1 or IP2?
'E' means 'external', it indicates the (most) outer IP header checksum error. If you
don't think this name is not so clear, I can change it to 'PKT_RX_OUTER_IP_CHSUM_BAD'.
For inner IP header checksum error, it will be indicated by PKT_RX_IP_CKSUM_BAD.

> 
> If it's IP1, it's really strange compared to the current behavior (the flag
> PKT_RX_IP_CKSUM_BAD refers to IP1).
> 
> > #define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX pkt
> oversize. */
> > [Helin] I don't think it can be merge with other hardware errors. It
> > indicates the packet received needs more descriptors than hardware
> > allowed, and the part of packets can still be stored in the mbufs
> > provided. It is a good hint for users that larger size of mbuf might
> > be needed. If just put it as hardware error, users will lose this information. So I
> prefer to keep it there, and just assign a new value to it.
> 
> Again, a statistic counter would do the job which if it's just to provide a hint to the
> application.
It seems that we do not maintain a counter for packets in PMD, if I am not wrong. Two
ways current DPDK is using.
One is hardware provide registers to do that, we can read it directly when needed.
The other one is that applications or middle layer sw maintain its own statistics.

> 
> I wonder in which case this flag can happen. If you fill the ring with mbufs that are
> large enough compared to your ethernet network, this should not happen in
> normal conditions. I really don't believe that an application receiving an mbuf
> with this flag would stop the driver, then refill the rings it with larger mbufs.
This is not because of it is lack of available RX descriptors. It is because of a hardware
requirement. FVL hardware requires that it should not use more than 5 rx descriptors
for receiving a single packet.

> Last but not least: If it's really useful, should we have the same behavior on other
> drivers like ixgbe? I think we really need to care about not having different ways
> to use the different drivers.
I don't see the similar bit in ixgbe datasheet, but this restriction could be common
for some other NICs, as it is reasonable from hardware perspective.

> 
> To me, the only argument in favor of keeping this flag is when the mbuf contains
> a part of the data that could be dumped by a user for debug purposes.
> 
> > #define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer overflow.
> > */ [Helin] It indicates the header buff size is not enough, but not
> > means hardware cannot process the packet received. It is a good hint
> > for the users to provide larger size of header buffers. I also prefer to keep it
> there, and just assign new value to it.
> 
> Same for this one.
It is a bit different from previous one, it always has one header buffer, this flag means
the buffer size is not enough for the header.
These two flags are because of buffer size or number of buffers. The mbufs are prepared
in application or up layer software. If these two flags occur, it is easier for up layer software
to debug, and know different buffers are needed. They do not need to debug PMD, as they
generally don't want to do.

> 
> > #define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing error.
> */
> > [Helin] In the latest data sheet, it is not opened to external users.
> > So we can just remove it from here.
> 
> ok
> 
> > #define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
> > [Helin] This indicates a real hardware error happens.
> 
> And what is the content of the mbuf data in this case? Does the application really
> need an mbuf?
Mbuf contains both the data and other information. I prefer to let the up layer software to
decide how to deal with the packet, no matter it is correct or bad. In addition, even hardware
errors happened, it still can set a special bit to enable storing the whole packet to the mbuf,
for debug purpose. Hardware error bit can be used for all hardware errors. As we do not have
one there, why not add one?

> 
> 
> Regards,
> Olivier

Regards,
Helin

Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[Olivier MATZ]

Hi Helin,

On 12/01/2014 02:57 AM, Zhang, Helin wrote:
>>> #define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**< External IP header
>>> checksum error. */ [Helin] Nobody complains it, so we will keep it there, and
>> just assign a new value to it.
>>
>> ok.
>>
>> But it would be nice to have a better definition of this flag: does external mean
>> outer header? For instance, when you receive a
>> Ether/IP1/UDP/vxlan/Ether/IP2/xxx, does the flag concerns IP1 or IP2?
> 'E' means 'external', it indicates the (most) outer IP header checksum error. If you
> don't think this name is not so clear, I can change it to 'PKT_RX_OUTER_IP_CHSUM_BAD'.
> For inner IP header checksum error, it will be indicated by PKT_RX_IP_CKSUM_BAD.
> 
>>
>> If it's IP1, it's really strange compared to the current behavior (the flag
>> PKT_RX_IP_CKSUM_BAD refers to IP1).

Ok.
But the real sense of my question was about the behavior which seems
different than with previous hardware. Today, if you receive the packet
Ether/IP1/UDP/vxlan/Ether/IP2/xxx on an ixgbe, the flag
PKT_RX_IP_CKSUM_BAD can be set if the checksum of IP1 is wrong. From
your explanation, I understand that PKT_RX_EIP_CKSUM_BAD would be set
for the same thing on i40e. Is it correct?


>>> #define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX pkt
>> oversize. */
>>> [Helin] I don't think it can be merge with other hardware errors. It
>>> indicates the packet received needs more descriptors than hardware
>>> allowed, and the part of packets can still be stored in the mbufs
>>> provided. It is a good hint for users that larger size of mbuf might
>>> be needed. If just put it as hardware error, users will lose this information. So I
>> prefer to keep it there, and just assign a new value to it.
>>
>> Again, a statistic counter would do the job which if it's just to provide a hint to the
>> application.
> It seems that we do not maintain a counter for packets in PMD, if I am not wrong. Two
> ways current DPDK is using.
> One is hardware provide registers to do that, we can read it directly when needed.
> The other one is that applications or middle layer sw maintain its own statistics.

rte_eth_stats_get() gives the generic statistics
For specific error stats, rte_eth_xstats_get() can be used from an
application (the driver has to provide the full list of statistics).

>> I wonder in which case this flag can happen. If you fill the ring with mbufs that are
>> large enough compared to your ethernet network, this should not happen in
>> normal conditions. I really don't believe that an application receiving an mbuf
>> with this flag would stop the driver, then refill the rings it with larger mbufs.
> This is not because of it is lack of available RX descriptors. It is because of a hardware
> requirement. FVL hardware requires that it should not use more than 5 rx descriptors
> for receiving a single packet.

I still don't understand what the application should do when the flag
is set. Maybe you could provide an example in l2fwd or testpmd?

>> Last but not least: If it's really useful, should we have the same behavior on other
>> drivers like ixgbe? I think we really need to care about not having different ways
>> to use the different drivers.
> I don't see the similar bit in ixgbe datasheet, but this restriction could be common
> for some other NICs, as it is reasonable from hardware perspective.

In ixgbe, there are other error cases:
- frames shorter than 64 bytes
- oversize (frames larger than MAXFRS)
- ... maybe others?

Should we have a flag for each situation? I think not.


>> To me, the only argument in favor of keeping this flag is when the mbuf contains
>> a part of the data that could be dumped by a user for debug purposes.
>>
>>> #define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer overflow.
>>> */ [Helin] It indicates the header buff size is not enough, but not
>>> means hardware cannot process the packet received. It is a good hint
>>> for the users to provide larger size of header buffers. I also prefer to keep it
>> there, and just assign new value to it.
>>
>> Same for this one.
> It is a bit different from previous one, it always has one header buffer, this flag means
> the buffer size is not enough for the header.
> These two flags are because of buffer size or number of buffers. The mbufs are prepared
> in application or up layer software. If these two flags occur, it is easier for up layer software
> to debug, and know different buffers are needed. They do not need to debug PMD, as they
> generally don't want to do.

You say it's easier for the software to debug, but I cannot see the
difference. When it's a statistics counter, you just have to use
rte_eth_xstats_get(), which is an equivalent of "ethtool -S iface"
which gives all the hardware statistics. It will work for any driver
and any application.

If we add these flags, the application have to know about all these
specific flags and how to handle them.

>>> #define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing error.
>> */
>>> [Helin] In the latest data sheet, it is not opened to external users.
>>> So we can just remove it from here.
>>
>> ok
>>
>>> #define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
>>> [Helin] This indicates a real hardware error happens.
>>
>> And what is the content of the mbuf data in this case? Does the application really
>> need an mbuf?
> Mbuf contains both the data and other information. I prefer to let the up layer software to
> decide how to deal with the packet, no matter it is correct or bad. In addition, even hardware
> errors happened, it still can set a special bit to enable storing the whole packet to the mbuf,
> for debug purpose. Hardware error bit can be used for all hardware errors. As we do not have
> one there, why not add one?

You say "let the up layer software to decide how to deal with the
packet, no matter it is correct or bad". But what can do an application
with a packet if it does not know if the data is correct or bad?

Regards,
Olivier

Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors

[Zhang, Helin]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Monday, December 1, 2014 5:58 PM
> To: Zhang, Helin; Ananyev, Konstantin; dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min
> Subject: Re: [PATCH] i40e: Use one bit flag for all hardware detected RX packet
> errors
> 
> Hi Helin,
> 
> On 12/01/2014 02:57 AM, Zhang, Helin wrote:
> >>> #define PKT_RX_EIP_CKSUM_BAD (0ULL << 0)  /**< External IP header
> >>> checksum error. */ [Helin] Nobody complains it, so we will keep it
> >>> there, and
> >> just assign a new value to it.
> >>
> >> ok.
> >>
> >> But it would be nice to have a better definition of this flag: does
> >> external mean outer header? For instance, when you receive a
> >> Ether/IP1/UDP/vxlan/Ether/IP2/xxx, does the flag concerns IP1 or IP2?
> > 'E' means 'external', it indicates the (most) outer IP header checksum
> > error. If you don't think this name is not so clear, I can change it to
> 'PKT_RX_OUTER_IP_CHSUM_BAD'.
> > For inner IP header checksum error, it will be indicated by
> PKT_RX_IP_CKSUM_BAD.
> >
> >>
> >> If it's IP1, it's really strange compared to the current behavior
> >> (the flag PKT_RX_IP_CKSUM_BAD refers to IP1).
> 
> Ok.
> But the real sense of my question was about the behavior which seems different
> than with previous hardware. Today, if you receive the packet
> Ether/IP1/UDP/vxlan/Ether/IP2/xxx on an ixgbe, the flag
> PKT_RX_IP_CKSUM_BAD can be set if the checksum of IP1 is wrong. From your
> explanation, I understand that PKT_RX_EIP_CKSUM_BAD would be set for the
> same thing on i40e. Is it correct?
Yes, it is strange if ixgbe hardware checksum logic knows the packet is tunneling.
But, from another point of view, which seems the the ixgbe hardware does, if
checksum logic does not know tunneling, it may treat all others as data. This is
reasonable to report the error with PKT_RX_IP_CKSUM_BAD.

> 
> 
> >>> #define PKT_RX_OVERSIZE      (0ULL << 0)  /**< Num of desc of an RX
> pkt
> >> oversize. */
> >>> [Helin] I don't think it can be merge with other hardware errors. It
> >>> indicates the packet received needs more descriptors than hardware
> >>> allowed, and the part of packets can still be stored in the mbufs
> >>> provided. It is a good hint for users that larger size of mbuf might
> >>> be needed. If just put it as hardware error, users will lose this
> >>> information. So I
> >> prefer to keep it there, and just assign a new value to it.
> >>
> >> Again, a statistic counter would do the job which if it's just to
> >> provide a hint to the application.
> > It seems that we do not maintain a counter for packets in PMD, if I am
> > not wrong. Two ways current DPDK is using.
> > One is hardware provide registers to do that, we can read it directly when
> needed.
> > The other one is that applications or middle layer sw maintain its own statistics.
> 
> rte_eth_stats_get() gives the generic statistics For specific error stats,
> rte_eth_xstats_get() can be used from an application (the driver has to provide
> the full list of statistics).
Yes, that function read all the statistics from registers directly. I think there might
already have the corresponding registers for it.

> 
> >> I wonder in which case this flag can happen. If you fill the ring
> >> with mbufs that are large enough compared to your ethernet network,
> >> this should not happen in normal conditions. I really don't believe
> >> that an application receiving an mbuf with this flag would stop the driver, then
> refill the rings it with larger mbufs.
> > This is not because of it is lack of available RX descriptors. It is
> > because of a hardware requirement. FVL hardware requires that it
> > should not use more than 5 rx descriptors for receiving a single packet.
> 
> I still don't understand what the application should do when the flag is set.
> Maybe you could provide an example in l2fwd or testpmd?
For an application, if it is reported with this oversize error, it then easily knows that
the mbuf size might not be enough. If it is reported with just a hardware error, then
the application developer needs to dig into the PMD to see what happens. I think most
of application developers do not want to debug PMD to much, as it might not be their scope.

> 
> >> Last but not least: If it's really useful, should we have the same
> >> behavior on other drivers like ixgbe? I think we really need to care
> >> about not having different ways to use the different drivers.
> > I don't see the similar bit in ixgbe datasheet, but this restriction
> > could be common for some other NICs, as it is reasonable from hardware
> perspective.
> 
> In ixgbe, there are other error cases:
> - frames shorter than 64 bytes
> - oversize (frames larger than MAXFRS)
> - ... maybe others?
> 
> Should we have a flag for each situation? I think not.
The more the better, but we may need a tradeoff, as the flag bits is limited.

> 
> 
> >> To me, the only argument in favor of keeping this flag is when the
> >> mbuf contains a part of the data that could be dumped by a user for debug
> purposes.
> >>
> >>> #define PKT_RX_HBUF_OVERFLOW (0ULL << 0)  /**< Header buffer
> overflow.
> >>> */ [Helin] It indicates the header buff size is not enough, but not
> >>> means hardware cannot process the packet received. It is a good hint
> >>> for the users to provide larger size of header buffers. I also
> >>> prefer to keep it
> >> there, and just assign new value to it.
> >>
> >> Same for this one.
> > It is a bit different from previous one, it always has one header
> > buffer, this flag means the buffer size is not enough for the header.
> > These two flags are because of buffer size or number of buffers. The
> > mbufs are prepared in application or up layer software. If these two
> > flags occur, it is easier for up layer software to debug, and know
> > different buffers are needed. They do not need to debug PMD, as they
> generally don't want to do.
> 
> You say it's easier for the software to debug, but I cannot see the difference.
> When it's a statistics counter, you just have to use rte_eth_xstats_get(), which is
> an equivalent of "ethtool -S iface"
> which gives all the hardware statistics. It will work for any driver and any
> application.
> 
> If we add these flags, the application have to know about all these specific flags
> and how to handle them.
OK. I agree with you not all errors need to be reported as flags. But I'd prefer to
add those flags which is useful for applications to bypass the errors.

> 
> >>> #define PKT_RX_RECIP_ERR     (0ULL << 0)  /**< Hardware processing
> error.
> >> */
> >>> [Helin] In the latest data sheet, it is not opened to external users.
> >>> So we can just remove it from here.
> >>
> >> ok
> >>
> >>> #define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
> >>> [Helin] This indicates a real hardware error happens.
> >>
> >> And what is the content of the mbuf data in this case? Does the
> >> application really need an mbuf?
> > Mbuf contains both the data and other information. I prefer to let the
> > up layer software to decide how to deal with the packet, no matter it
> > is correct or bad. In addition, even hardware errors happened, it
> > still can set a special bit to enable storing the whole packet to the
> > mbuf, for debug purpose. Hardware error bit can be used for all hardware
> errors. As we do not have one there, why not add one?
> 
> You say "let the up layer software to decide how to deal with the packet, no
> matter it is correct or bad". But what can do an application with a packet if it does
> not know if the data is correct or bad?
Mbuf flags can tell him if the data is good or bad, if good, more info can be seen in flags.

> 
> Regards,
> Olivier

Yes, flags should be a tradeoff, we can provide useful flags in mbuf, but not all.

Regards,
Helin

[PATCH v2 0/2] fix of enabling all newly added error flags

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

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.

Helin Zhang (2):
  i40e: remove checking rxd flag which is not public
  mbuf: assign valid bit values for some RX and TX flags

 lib/librte_mbuf/rte_mbuf.c      |  9 ++++-----
 lib/librte_mbuf/rte_mbuf.h      | 19 +++++++++----------
 lib/librte_pmd_i40e/i40e_rxtx.c |  6 ------
 3 files changed, 13 insertions(+), 21 deletions(-)

-- 
1.9.3

[PATCH v2 1/2] i40e: remove checking rxd flag which is not public

[Helin Zhang]

According to the latest datasheet, RX descriptor error flag of
'RECIPE' is not for public use, so its checks should be removed.

Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_i40e/i40e_rxtx.c | 6 ------
 1 file changed, 6 deletions(-)

v2 changes:
* Removed error flag of 'ECIPE' processing only in i40e PMD. All other error
  flags were added back.

diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c
index 63c872d..7ffe50e 100644
--- a/lib/librte_pmd_i40e/i40e_rxtx.c
+++ b/lib/librte_pmd_i40e/i40e_rxtx.c
@@ -126,12 +126,6 @@ i40e_rxd_error_to_pkt_flags(uint64_t qword)
 		flags |= PKT_RX_MAC_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)))
-- 
1.9.3

[PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX flags

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

Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_mbuf/rte_mbuf.c |  9 ++++-----
 lib/librte_mbuf/rte_mbuf.h | 19 +++++++++----------
 2 files changed, 13 insertions(+), 15 deletions(-)

v2 changes:
* Removed error flag of 'ECIPE' processing only in 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.

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 87c2963..3ce7c8d 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -210,11 +210,10 @@ 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_OVERSIZE: return "PKT_RX_OVERSIZE";
+	case PKT_RX_HBUF_OVERFLOW: return "PKT_RX_HBUF_OVERFLOW";
+	case PKT_RX_MAC_ERR: return "PKT_RX_MAC_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 2e5fce5..c9591c0 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,10 @@ 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_OVERSIZE      (1ULL << 16)  /**< Num of desc of an RX pkt oversize. */
+#define PKT_RX_HBUF_OVERFLOW (1ULL << 17)  /**< Header buffer overflow. */
+#define PKT_RX_MAC_ERR       (1ULL << 18)  /**< MAC error. */
 /* add new RX flags here */
 
 /* add new TX flags here */
@@ -141,13 +140,13 @@ extern "C" {
 #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt. computed by NIC. */
 #define PKT_TX_IPV4_CSUM     PKT_TX_IP_CKSUM /**< Alias of PKT_TX_IP_CKSUM. */
 
-/** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO. */
-#define PKT_TX_IPV4          PKT_RX_IPV4_HDR
+/** Tell the NIC it's an IPv4 packet. */
+#define PKT_TX_IPV4          (1ULL << 55) /**< TX packet is a IPV4 packet. */
 
-/** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO. */
-#define PKT_TX_IPV6          PKT_RX_IPV6_HDR
+/** Tell the NIC it's an IPv6 packet. */
+#define PKT_TX_IPV6          (1ULL << 56) /**< TX packet is a IPV6 packet. */
 
-#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
+#define PKT_TX_VLAN_PKT      (1ULL << 57) /**< TX packet is a VLAN packet. */
 
 /* Use final bit of flags to indicate a control mbuf */
 #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains control data */
-- 
1.9.3

Re: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX flags

[Ananyev, Konstantin]

Hi Helin,

> -----Original Message-----
> From: Zhang, Helin
> Sent: Friday, December 05, 2014 1:46 AM
> To: dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang; Ananyev, Konstantin; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; Zhang, Helin
> Subject: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX 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.
> 
> Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  lib/librte_mbuf/rte_mbuf.c |  9 ++++-----
>  lib/librte_mbuf/rte_mbuf.h | 19 +++++++++----------
>  2 files changed, 13 insertions(+), 15 deletions(-)
> 
> v2 changes:
> * Removed error flag of 'ECIPE' processing only in 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.
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 87c2963..3ce7c8d 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -210,11 +210,10 @@ 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_OVERSIZE: return "PKT_RX_OVERSIZE";
> +	case PKT_RX_HBUF_OVERFLOW: return "PKT_RX_HBUF_OVERFLOW";
> +	case PKT_RX_MAC_ERR: return "PKT_RX_MAC_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 2e5fce5..c9591c0 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,10 @@ 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_OVERSIZE      (1ULL << 16)  /**< Num of desc of an RX pkt oversize. */
> +#define PKT_RX_HBUF_OVERFLOW (1ULL << 17)  /**< Header buffer overflow. */
> +#define PKT_RX_MAC_ERR       (1ULL << 18)  /**< MAC error. */
>  /* add new RX flags here */

I still think there is no point to have several flags to indicate HW error for the packet.
As I suggested before we can collapse 3 of them (OVERSIZE, HBUF_OVERFLOW, MAC_ERR) into one. 
As I remember, Oliver even suggested to drop such packets.
As was said above - if is not a whole packet SW can't do much with it anyway.
The only thing such bad packets can probably be used for - some sort of debugging.
So we probably can combine both things: 
- in normal operation just drop such packet 
- if PMD_DEBUG_RX is enabled, then write a log record, set RX_HW_ERR and deliver a packet to the upper layer.

> 
>  /* add new TX flags here */
> @@ -141,13 +140,13 @@ extern "C" {
>  #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt. computed by NIC. */
>  #define PKT_TX_IPV4_CSUM     PKT_TX_IP_CKSUM /**< Alias of PKT_TX_IP_CKSUM. */
> 
> -/** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO. */
> -#define PKT_TX_IPV4          PKT_RX_IPV4_HDR
> +/** Tell the NIC it's an IPv4 packet. */
> +#define PKT_TX_IPV4          (1ULL << 55) /**< TX packet is a IPV4 packet. */
> 
> -/** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO. */
> -#define PKT_TX_IPV6          PKT_RX_IPV6_HDR
> +/** Tell the NIC it's an IPv6 packet. */
> +#define PKT_TX_IPV6          (1ULL << 56) /**< TX packet is a IPV6 packet. */
> 
> -#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */
> +#define PKT_TX_VLAN_PKT      (1ULL << 57) /**< TX packet is a VLAN packet. */

I don't think these changes should be part of that patch.
They violate another patch that Frank sent before.

Konstantin

> 
>  /* Use final bit of flags to indicate a control mbuf */
>  #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains control data */
> --
> 1.9.3

Re: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX flags

[Zhang, Helin]

OK. I will send out another patch according to your comments. Thanks a lot!

Regards,
Helin

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, December 5, 2014 6:50 PM
> To: Zhang, Helin; dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang;
> olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org
> Subject: RE: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX
> flags
> 
> Hi Helin,
> 
> > -----Original Message-----
> > From: Zhang, Helin
> > Sent: Friday, December 05, 2014 1:46 AM
> > To: dev-VfR2kkLFssw@public.gmane.org
> > Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang; Ananyev,
> > Konstantin; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; Zhang, Helin
> > Subject: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and
> > TX 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.
> >
> > Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  lib/librte_mbuf/rte_mbuf.c |  9 ++++-----  lib/librte_mbuf/rte_mbuf.h
> > | 19 +++++++++----------
> >  2 files changed, 13 insertions(+), 15 deletions(-)
> >
> > v2 changes:
> > * Removed error flag of 'ECIPE' processing only in 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.
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 87c2963..3ce7c8d 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -210,11 +210,10 @@ 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_OVERSIZE: return "PKT_RX_OVERSIZE";
> > +	case PKT_RX_HBUF_OVERFLOW: return "PKT_RX_HBUF_OVERFLOW";
> > +	case PKT_RX_MAC_ERR: return "PKT_RX_MAC_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
> > 2e5fce5..c9591c0 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,10 @@ 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_OVERSIZE      (1ULL << 16)  /**< Num of desc of an RX
> pkt oversize. */
> > +#define PKT_RX_HBUF_OVERFLOW (1ULL << 17)  /**< Header buffer
> overflow. */
> > +#define PKT_RX_MAC_ERR       (1ULL << 18)  /**< MAC error. */
> >  /* add new RX flags here */
> 
> I still think there is no point to have several flags to indicate HW error for the
> packet.
> As I suggested before we can collapse 3 of them (OVERSIZE, HBUF_OVERFLOW,
> MAC_ERR) into one.
> As I remember, Oliver even suggested to drop such packets.
> As was said above - if is not a whole packet SW can't do much with it anyway.
> The only thing such bad packets can probably be used for - some sort of
> debugging.
> So we probably can combine both things:
> - in normal operation just drop such packet
> - if PMD_DEBUG_RX is enabled, then write a log record, set RX_HW_ERR and
> deliver a packet to the upper layer.
> 
> >
> >  /* add new TX flags here */
> > @@ -141,13 +140,13 @@ extern "C" {
> >  #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt.
> computed by NIC. */
> >  #define PKT_TX_IPV4_CSUM     PKT_TX_IP_CKSUM /**< Alias of
> PKT_TX_IP_CKSUM. */
> >
> > -/** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or TSO.
> */
> > -#define PKT_TX_IPV4          PKT_RX_IPV4_HDR
> > +/** Tell the NIC it's an IPv4 packet. */
> > +#define PKT_TX_IPV4          (1ULL << 55) /**< TX packet is a IPV4
> packet. */
> >
> > -/** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or TSO.
> */
> > -#define PKT_TX_IPV6          PKT_RX_IPV6_HDR
> > +/** Tell the NIC it's an IPv6 packet. */
> > +#define PKT_TX_IPV6          (1ULL << 56) /**< TX packet is a IPV6
> packet. */
> >
> > -#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q
> VLAN packet. */
> > +#define PKT_TX_VLAN_PKT      (1ULL << 57) /**< TX packet is a VLAN
> packet. */
> 
> I don't think these changes should be part of that patch.
> They violate another patch that Frank sent before.
> 
> Konstantin
> 
> >
> >  /* Use final bit of flags to indicate a control mbuf */
> >  #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains control
> data */
> > --
> > 1.9.3

Re: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX flags

[Zhang, Helin]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Saturday, December 6, 2014 8:40 AM
> To: Ananyev, Konstantin; dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang;
> olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org
> Subject: RE: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX
> flags
> 
> OK. I will send out another patch according to your comments. Thanks a lot!
> 
> Regards,
> Helin
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Friday, December 5, 2014 6:50 PM
> > To: Zhang, Helin; dev-VfR2kkLFssw@public.gmane.org
> > Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang;
> > olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org
> > Subject: RE: [PATCH v2 2/2] mbuf: assign valid bit values for some RX
> > and TX flags
> >
> > Hi Helin,
> >
> > > -----Original Message-----
> > > From: Zhang, Helin
> > > Sent: Friday, December 05, 2014 1:46 AM
> > > To: dev-VfR2kkLFssw@public.gmane.org
> > > Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang; Ananyev,
> > > Konstantin; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; Zhang, Helin
> > > Subject: [PATCH v2 2/2] mbuf: assign valid bit values for some RX
> > > and TX 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.
> > >
> > > Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  lib/librte_mbuf/rte_mbuf.c |  9 ++++-----
> > > lib/librte_mbuf/rte_mbuf.h
> > > | 19 +++++++++----------
> > >  2 files changed, 13 insertions(+), 15 deletions(-)
> > >
> > > v2 changes:
> > > * Removed error flag of 'ECIPE' processing only in 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.
> > >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > > index 87c2963..3ce7c8d 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.c
> > > +++ b/lib/librte_mbuf/rte_mbuf.c
> > > @@ -210,11 +210,10 @@ 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_OVERSIZE: return "PKT_RX_OVERSIZE";
> > > +	case PKT_RX_HBUF_OVERFLOW: return
> "PKT_RX_HBUF_OVERFLOW";
> > > +	case PKT_RX_MAC_ERR: return "PKT_RX_MAC_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
> > > 2e5fce5..c9591c0 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,10 @@ 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_OVERSIZE      (1ULL << 16)  /**< Num of desc of an
> RX
> > pkt oversize. */
> > > +#define PKT_RX_HBUF_OVERFLOW (1ULL << 17)  /**< Header buffer
> > overflow. */
> > > +#define PKT_RX_MAC_ERR       (1ULL << 18)  /**< MAC error. */
> > >  /* add new RX flags here */
> >
> > I still think there is no point to have several flags to indicate HW
> > error for the packet.
> > As I suggested before we can collapse 3 of them (OVERSIZE,
> > HBUF_OVERFLOW,
> > MAC_ERR) into one.
> > As I remember, Oliver even suggested to drop such packets.
> > As was said above - if is not a whole packet SW can't do much with it anyway.
> > The only thing such bad packets can probably be used for - some sort
> > of debugging.
> > So we probably can combine both things:
> > - in normal operation just drop such packet
> > - if PMD_DEBUG_RX is enabled, then write a log record, set RX_HW_ERR
> > and deliver a packet to the upper layer.
I still do not want to drop the bad packet here, as it may affect vector processing.
At least it should be investigated how much impact on vector RX. I prefer to let
up-layer software do that.

> >
> > >
> > >  /* add new TX flags here */
> > > @@ -141,13 +140,13 @@ extern "C" {
> > >  #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt.
> > computed by NIC. */
> > >  #define PKT_TX_IPV4_CSUM     PKT_TX_IP_CKSUM /**< Alias of
> > PKT_TX_IP_CKSUM. */
> > >
> > > -/** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or
> TSO.
> > */
> > > -#define PKT_TX_IPV4          PKT_RX_IPV4_HDR
> > > +/** Tell the NIC it's an IPv4 packet. */
> > > +#define PKT_TX_IPV4          (1ULL << 55) /**< TX packet is a IPV4
> > packet. */
> > >
> > > -/** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or
> TSO.
> > */
> > > -#define PKT_TX_IPV6          PKT_RX_IPV6_HDR
> > > +/** Tell the NIC it's an IPv6 packet. */
> > > +#define PKT_TX_IPV6          (1ULL << 56) /**< TX packet is a IPV6
> > packet. */
> > >
> > > -#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q
> > VLAN packet. */
> > > +#define PKT_TX_VLAN_PKT      (1ULL << 57) /**< TX packet is a VLAN
> > packet. */
> >
> > I don't think these changes should be part of that patch.
> > They violate another patch that Frank sent before.
> >
> > Konstantin
> >
> > >
> > >  /* Use final bit of flags to indicate a control mbuf */
> > >  #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains
> control
> > data */
> > > --
> > > 1.9.3

Re: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX flags

[Ananyev, Konstantin]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Saturday, December 06, 2014 1:08 AM
> To: Ananyev, Konstantin; dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang; olivier.matz@6wind.com
> Subject: RE: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX flags
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Helin
> > Sent: Saturday, December 6, 2014 8:40 AM
> > To: Ananyev, Konstantin; dev-VfR2kkLFssw@public.gmane.org
> > Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang;
> > olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org
> > Subject: RE: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX
> > flags
> >
> > OK. I will send out another patch according to your comments. Thanks a lot!
> >
> > Regards,
> > Helin
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Friday, December 5, 2014 6:50 PM
> > > To: Zhang, Helin; dev-VfR2kkLFssw@public.gmane.org
> > > Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang;
> > > olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org
> > > Subject: RE: [PATCH v2 2/2] mbuf: assign valid bit values for some RX
> > > and TX flags
> > >
> > > Hi Helin,
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Helin
> > > > Sent: Friday, December 05, 2014 1:46 AM
> > > > To: dev-VfR2kkLFssw@public.gmane.org
> > > > Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang; Ananyev,
> > > > Konstantin; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; Zhang, Helin
> > > > Subject: [PATCH v2 2/2] mbuf: assign valid bit values for some RX
> > > > and TX 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.
> > > >
> > > > Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > ---
> > > >  lib/librte_mbuf/rte_mbuf.c |  9 ++++-----
> > > > lib/librte_mbuf/rte_mbuf.h
> > > > | 19 +++++++++----------
> > > >  2 files changed, 13 insertions(+), 15 deletions(-)
> > > >
> > > > v2 changes:
> > > > * Removed error flag of 'ECIPE' processing only in 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.
> > > >
> > > > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > > > index 87c2963..3ce7c8d 100644
> > > > --- a/lib/librte_mbuf/rte_mbuf.c
> > > > +++ b/lib/librte_mbuf/rte_mbuf.c
> > > > @@ -210,11 +210,10 @@ 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_OVERSIZE: return "PKT_RX_OVERSIZE";
> > > > +	case PKT_RX_HBUF_OVERFLOW: return
> > "PKT_RX_HBUF_OVERFLOW";
> > > > +	case PKT_RX_MAC_ERR: return "PKT_RX_MAC_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
> > > > 2e5fce5..c9591c0 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,10 @@ 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_OVERSIZE      (1ULL << 16)  /**< Num of desc of an
> > RX
> > > pkt oversize. */
> > > > +#define PKT_RX_HBUF_OVERFLOW (1ULL << 17)  /**< Header buffer
> > > overflow. */
> > > > +#define PKT_RX_MAC_ERR       (1ULL << 18)  /**< MAC error. */
> > > >  /* add new RX flags here */
> > >
> > > I still think there is no point to have several flags to indicate HW
> > > error for the packet.
> > > As I suggested before we can collapse 3 of them (OVERSIZE,
> > > HBUF_OVERFLOW,
> > > MAC_ERR) into one.
> > > As I remember, Oliver even suggested to drop such packets.
> > > As was said above - if is not a whole packet SW can't do much with it anyway.
> > > The only thing such bad packets can probably be used for - some sort
> > > of debugging.
> > > So we probably can combine both things:
> > > - in normal operation just drop such packet
> > > - if PMD_DEBUG_RX is enabled, then write a log record, set RX_HW_ERR
> > > and deliver a packet to the upper layer.
> I still do not want to drop the bad packet here, as it may affect vector processing.
> At least it should be investigated how much impact on vector RX. I prefer to let
> up-layer software do that.

But right now, i40e doesn't have any vector RX support.
You probably meant implications with scatter RX, no?
Another thing - if you HW can't receive packets normally, then something really wrong is going on.
In such situation, you probably wouldn't worry about your RX performance anyway :) 
But I suppose it is a good first step, let's just collapse 3 error flags into one for now.
Then  we can decide should we drop such packets or not. 
Konstantin

> 
> > >
> > > >
> > > >  /* add new TX flags here */
> > > > @@ -141,13 +140,13 @@ extern "C" {
> > > >  #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX pkt.
> > > computed by NIC. */
> > > >  #define PKT_TX_IPV4_CSUM     PKT_TX_IP_CKSUM /**< Alias of
> > > PKT_TX_IP_CKSUM. */
> > > >
> > > > -/** Tell the NIC it's an IPv4 packet. Required for L4 checksum offload or
> > TSO.
> > > */
> > > > -#define PKT_TX_IPV4          PKT_RX_IPV4_HDR
> > > > +/** Tell the NIC it's an IPv4 packet. */
> > > > +#define PKT_TX_IPV4          (1ULL << 55) /**< TX packet is a IPV4
> > > packet. */
> > > >
> > > > -/** Tell the NIC it's an IPv6 packet. Required for L4 checksum offload or
> > TSO.
> > > */
> > > > -#define PKT_TX_IPV6          PKT_RX_IPV6_HDR
> > > > +/** Tell the NIC it's an IPv6 packet. */
> > > > +#define PKT_TX_IPV6          (1ULL << 56) /**< TX packet is a IPV6
> > > packet. */
> > > >
> > > > -#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a 802.1q
> > > VLAN packet. */
> > > > +#define PKT_TX_VLAN_PKT      (1ULL << 57) /**< TX packet is a VLAN
> > > packet. */
> > >
> > > I don't think these changes should be part of that patch.
> > > They violate another patch that Frank sent before.
> > >
> > > Konstantin
> > >
> > > >
> > > >  /* Use final bit of flags to indicate a control mbuf */
> > > >  #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains
> > control
> > > data */
> > > > --
> > > > 1.9.3

Re: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX flags

[Zhang, Helin]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, December 8, 2014 6:55 PM
> To: Zhang, Helin; dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang;
> olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org
> Subject: RE: [PATCH v2 2/2] mbuf: assign valid bit values for some RX and TX
> flags
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Helin
> > Sent: Saturday, December 06, 2014 1:08 AM
> > To: Ananyev, Konstantin; dev-VfR2kkLFssw@public.gmane.org
> > Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang;
> > olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org
> > Subject: RE: [PATCH v2 2/2] mbuf: assign valid bit values for some RX
> > and TX flags
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Helin
> > > Sent: Saturday, December 6, 2014 8:40 AM
> > > To: Ananyev, Konstantin; dev-VfR2kkLFssw@public.gmane.org
> > > Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang;
> > > olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org
> > > Subject: RE: [PATCH v2 2/2] mbuf: assign valid bit values for some
> > > RX and TX flags
> > >
> > > OK. I will send out another patch according to your comments. Thanks a lot!
> > >
> > > Regards,
> > > Helin
> > >
> > > > -----Original Message-----
> > > > From: Ananyev, Konstantin
> > > > Sent: Friday, December 5, 2014 6:50 PM
> > > > To: Zhang, Helin; dev-VfR2kkLFssw@public.gmane.org
> > > > Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang;
> > > > olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org
> > > > Subject: RE: [PATCH v2 2/2] mbuf: assign valid bit values for some
> > > > RX and TX flags
> > > >
> > > > Hi Helin,
> > > >
> > > > > -----Original Message-----
> > > > > From: Zhang, Helin
> > > > > Sent: Friday, December 05, 2014 1:46 AM
> > > > > To: dev-VfR2kkLFssw@public.gmane.org
> > > > > Cc: Cao, Waterman; Cao, Min; Wu, Jingjing; Liu, Jijiang;
> > > > > Ananyev, Konstantin; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; Zhang, Helin
> > > > > Subject: [PATCH v2 2/2] mbuf: assign valid bit values for some
> > > > > RX and TX 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.
> > > > >
> > > > > Signed-off-by: Helin Zhang <helin.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > > ---
> > > > >  lib/librte_mbuf/rte_mbuf.c |  9 ++++-----
> > > > > lib/librte_mbuf/rte_mbuf.h
> > > > > | 19 +++++++++----------
> > > > >  2 files changed, 13 insertions(+), 15 deletions(-)
> > > > >
> > > > > v2 changes:
> > > > > * Removed error flag of 'ECIPE' processing only in 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.
> > > > >
> > > > > diff --git a/lib/librte_mbuf/rte_mbuf.c
> > > > > b/lib/librte_mbuf/rte_mbuf.c index 87c2963..3ce7c8d 100644
> > > > > --- a/lib/librte_mbuf/rte_mbuf.c
> > > > > +++ b/lib/librte_mbuf/rte_mbuf.c
> > > > > @@ -210,11 +210,10 @@ 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_OVERSIZE: return "PKT_RX_OVERSIZE";
> > > > > +	case PKT_RX_HBUF_OVERFLOW: return
> > > "PKT_RX_HBUF_OVERFLOW";
> > > > > +	case PKT_RX_MAC_ERR: return "PKT_RX_MAC_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
> > > > > 2e5fce5..c9591c0 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,10 @@ 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_OVERSIZE      (1ULL << 16)  /**< Num of desc of
> an
> > > RX
> > > > pkt oversize. */
> > > > > +#define PKT_RX_HBUF_OVERFLOW (1ULL << 17)  /**< Header buffer
> > > > overflow. */
> > > > > +#define PKT_RX_MAC_ERR       (1ULL << 18)  /**< MAC error. */
> > > > >  /* add new RX flags here */
> > > >
> > > > I still think there is no point to have several flags to indicate
> > > > HW error for the packet.
> > > > As I suggested before we can collapse 3 of them (OVERSIZE,
> > > > HBUF_OVERFLOW,
> > > > MAC_ERR) into one.
> > > > As I remember, Oliver even suggested to drop such packets.
> > > > As was said above - if is not a whole packet SW can't do much with it
> anyway.
> > > > The only thing such bad packets can probably be used for - some
> > > > sort of debugging.
> > > > So we probably can combine both things:
> > > > - in normal operation just drop such packet
> > > > - if PMD_DEBUG_RX is enabled, then write a log record, set
> > > > RX_HW_ERR and deliver a packet to the upper layer.
> > I still do not want to drop the bad packet here, as it may affect vector
> processing.
> > At least it should be investigated how much impact on vector RX. I
> > prefer to let up-layer software do that.
> 
> But right now, i40e doesn't have any vector RX support.
> You probably meant implications with scatter RX, no?
I was thinking we may need to add the similar things in igb and ixgbe (possibly vector).

> Another thing - if you HW can't receive packets normally, then something really
> wrong is going on.
> In such situation, you probably wouldn't worry about your RX performance
That may need an if() for each received packets without any error, though might
not affect performance number.

> anyway :) But I suppose it is a good first step, let's just collapse 3 error flags
> into one for now.
> Then  we can decide should we drop such packets or not.
> Konstantin
Agree to think more on this point!

> 
> >
> > > >
> > > > >
> > > > >  /* add new TX flags here */
> > > > > @@ -141,13 +140,13 @@ extern "C" {
> > > > >  #define PKT_TX_IP_CKSUM      (1ULL << 54) /**< IP cksum of TX
> pkt.
> > > > computed by NIC. */
> > > > >  #define PKT_TX_IPV4_CSUM     PKT_TX_IP_CKSUM /**< Alias of
> > > > PKT_TX_IP_CKSUM. */
> > > > >
> > > > > -/** Tell the NIC it's an IPv4 packet. Required for L4 checksum
> > > > > offload or
> > > TSO.
> > > > */
> > > > > -#define PKT_TX_IPV4          PKT_RX_IPV4_HDR
> > > > > +/** Tell the NIC it's an IPv4 packet. */
> > > > > +#define PKT_TX_IPV4          (1ULL << 55) /**< TX packet is a IPV4
> > > > packet. */
> > > > >
> > > > > -/** Tell the NIC it's an IPv6 packet. Required for L4 checksum
> > > > > offload or
> > > TSO.
> > > > */
> > > > > -#define PKT_TX_IPV6          PKT_RX_IPV6_HDR
> > > > > +/** Tell the NIC it's an IPv6 packet. */
> > > > > +#define PKT_TX_IPV6          (1ULL << 56) /**< TX packet is a IPV6
> > > > packet. */
> > > > >
> > > > > -#define PKT_TX_VLAN_PKT      (1ULL << 55) /**< TX packet is a
> 802.1q
> > > > VLAN packet. */
> > > > > +#define PKT_TX_VLAN_PKT      (1ULL << 57) /**< TX packet is a
> VLAN
> > > > packet. */
> > > >
> > > > I don't think these changes should be part of that patch.
> > > > They violate another patch that Frank sent before.
> > > >
> > > > Konstantin
> > > >
> > > > >
> > > > >  /* Use final bit of flags to indicate a control mbuf */
> > > > >  #define CTRL_MBUF_FLAG       (1ULL << 63) /**< Mbuf contains
> > > control
> > > > data */
> > > > > --
> > > > > 1.9.3

Regards,
Helin

[PATCH v3] mbuf: fix of enabling all newly added RX error flags

[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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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;
 
 	return flags;
 }
-- 
1.9.3

Re: [PATCH v3] mbuf: fix of enabling all newly added RX error flags

[Ananyev, Konstantin]

Hi Helin,

> -----Original Message-----
> From: Zhang, Helin
> Sent: Saturday, December 06, 2014 1:34 AM
> To: dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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

Re: [PATCH v3] mbuf: fix of enabling all newly added RX error flags

[Zhang, Helin]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, December 8, 2014 6:45 PM
> To: Zhang, Helin; dev-VfR2kkLFssw@public.gmane.org
> Cc: Cao, Waterman; Cao, Min; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org
> 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-VfR2kkLFssw@public.gmane.org
> > Cc: Cao, Waterman; Cao, Min; olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  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

Re: [PATCH v3] mbuf: fix of enabling all newly added RX error flags

[Thomas Monjalon]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
[...]
> --- 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.

-- 
Thomas

Re: [PATCH v3] mbuf: fix of enabling all newly added RX error flags

[Zhang, Helin]

Hi Thomas

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Monday, December 8, 2014 6:51 PM
> To: Zhang, Helin
> Cc: dev-VfR2kkLFssw@public.gmane.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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> [...]
> > --- 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

Re: [PATCH v3] mbuf: fix of enabling all newly added RX error flags

[Thomas Monjalon]

2014-12-09 02:14, Zhang, Helin:
> From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> > 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > [...]
> > > --- 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

-- 
Thomas

[PATCH v4] mbuf: fix of enabling all newly added RX error flags

[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 uses 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_mbuf/rte_mbuf.c      |  7 ++-----
 lib/librte_mbuf/rte_mbuf.h      |  9 +++------
 lib/librte_pmd_i40e/i40e_rxtx.c | 37 ++++++++++++++++++++-----------------
 3 files changed, 25 insertions(+), 28 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.

v4 changes:
* Renamed 'PKT_RX_EIP_CKSUM_BAD' to 'PKT_RX_OUTER_IP_CKSUM_BAD'.
* Fixed the bug of checking error bits of 'Descriptor oversize' and
  'Header buffer oversize'.
* Added debug logs for each RX error.

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 1b14e02..7611a38 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_OUTER_IP_CKSUM_BAD: return "PKT_RX_OUTER_IP_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..eefe8a6 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -83,12 +83,7 @@ extern "C" {
 #define PKT_RX_RSS_HASH      (1ULL << 1)  /**< RX packet with RSS hash result. */
 #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_IP_CKSUM_BAD  (1ULL << 4)  /**< IP (or inner IP) header checksum 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_OUTER_IP_CKSUM_BAD (1ULL << 15)  /**< Outer 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..43f67c3 100644
--- a/lib/librte_pmd_i40e/i40e_rxtx.c
+++ b/lib/librte_pmd_i40e/i40e_rxtx.c
@@ -68,6 +68,7 @@
 #define I40E_TX_MAX_BURST  32
 
 #define I40E_DMA_MEM_ALIGN 4096
+#define I40E_RX_ERR_MASK   0x3F
 
 #define I40E_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
 					ETH_TXQ_FLAGS_NOOFFLOADS)
@@ -118,30 +119,32 @@ i40e_rxd_error_to_pkt_flags(uint64_t qword)
 	uint64_t flags = 0;
 	uint64_t error_bits = (qword >> I40E_RXD_QW1_ERROR_SHIFT);
 
-#define I40E_RX_ERR_BITS 0x3f
-	if (likely((error_bits & I40E_RX_ERR_BITS) == 0))
+	if (likely((error_bits & I40E_RX_ERR_MASK) == 0))
 		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;
+		PMD_DRV_LOG(DEBUG, "Hardware MAC error");
 		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)))
+	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)))
+		PMD_DRV_LOG(DEBUG, "IP or inner IP checksum error");
+	}
+	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;
+		PMD_DRV_LOG(DEBUG, "L4 integrity error");
+	}
+	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_EIPE_SHIFT))) {
+		flags |= PKT_RX_OUTER_IP_CKSUM_BAD;
+		PMD_DRV_LOG(DEBUG, "Outer IP checksum error");
+	}
+	if (unlikely(error_bits & ((1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT) |
+				(1 << I40E_RX_DESC_ERROR_HBO_SHIFT)))) {
+		flags |= PKT_RX_ERR;
+		PMD_DRV_LOG(DEBUG, "Header buffer overflow, or descriptor "
+							"number oversize");
+	}
 
 	return flags;
 }
-- 
1.9.3

Re: [PATCH v4] mbuf: fix of enabling all newly added RX error flags

[Thomas Monjalon]

2014-12-10 16:55, 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 uses 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  lib/librte_mbuf/rte_mbuf.c      |  7 ++-----
>  lib/librte_mbuf/rte_mbuf.h      |  9 +++------
>  lib/librte_pmd_i40e/i40e_rxtx.c | 37 ++++++++++++++++++++-----------------
>  3 files changed, 25 insertions(+), 28 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.
> 
> v4 changes:
> * Renamed 'PKT_RX_EIP_CKSUM_BAD' to 'PKT_RX_OUTER_IP_CKSUM_BAD'.
> * Fixed the bug of checking error bits of 'Descriptor oversize' and
>   'Header buffer oversize'.
> * Added debug logs for each RX error.
[...]
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -83,12 +83,7 @@ extern "C" {
>  #define PKT_RX_RSS_HASH      (1ULL << 1)  /**< RX packet with RSS hash result. */
>  #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_IP_CKSUM_BAD  (1ULL << 4)  /**< IP (or inner IP) header checksum error. */

It can be also an outer IP header in case the device don't support
tunneling or is not configured to detect it.
-- 
Thomas

>  #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_OUTER_IP_CKSUM_BAD (1ULL << 15)  /**< Outer 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 */

Re: [PATCH v4] mbuf: fix of enabling all newly added RX error flags

[Zhang, Helin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Wednesday, December 10, 2014 5:35 PM
> To: Zhang, Helin
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH v4] mbuf: fix of enabling all newly added RX
> error flags
> 
> 2014-12-10 16:55, 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 uses 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  lib/librte_mbuf/rte_mbuf.c      |  7 ++-----
> >  lib/librte_mbuf/rte_mbuf.h      |  9 +++------
> >  lib/librte_pmd_i40e/i40e_rxtx.c | 37
> > ++++++++++++++++++++-----------------
> >  3 files changed, 25 insertions(+), 28 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.
> >
> > v4 changes:
> > * Renamed 'PKT_RX_EIP_CKSUM_BAD' to 'PKT_RX_OUTER_IP_CKSUM_BAD'.
> > * Fixed the bug of checking error bits of 'Descriptor oversize' and
> >   'Header buffer oversize'.
> > * Added debug logs for each RX error.
> [...]
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -83,12 +83,7 @@ extern "C" {
> >  #define PKT_RX_RSS_HASH      (1ULL << 1)  /**< RX packet with RSS
> hash result. */
> >  #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_IP_CKSUM_BAD  (1ULL << 4)  /**< IP (or inner IP)
> > +header checksum error. */
> 
> It can be also an outer IP header in case the device don't support tunneling or is
> not configured to detect it.
For non-tunneling case, no outer/inner at all, it just has the IP header. The bit flag
indicates the IP header checksum error.
For tunneling case, this bit flag indicates the inner IP header checksum error, another
one for outer IP header checksum error.
So I don't think this bit can be treated as outer.

Regards,
Helin

> --
> Thomas
> 
> >  #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_OUTER_IP_CKSUM_BAD (1ULL << 15)  /**< Outer 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 */

Re: [PATCH v4] mbuf: fix of enabling all newly added RX error flags

[Thomas Monjalon]

2014-12-10 13:50, Zhang, Helin:
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> > Sent: Wednesday, December 10, 2014 5:35 PM
> > To: Zhang, Helin
> > Cc: dev-VfR2kkLFssw@public.gmane.org
> > Subject: Re: [dpdk-dev] [PATCH v4] mbuf: fix of enabling all newly added RX
> > error flags
> > 
> > 2014-12-10 16:55, 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 uses 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  lib/librte_mbuf/rte_mbuf.c      |  7 ++-----
> > >  lib/librte_mbuf/rte_mbuf.h      |  9 +++------
> > >  lib/librte_pmd_i40e/i40e_rxtx.c | 37
> > > ++++++++++++++++++++-----------------
> > >  3 files changed, 25 insertions(+), 28 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.
> > >
> > > v4 changes:
> > > * Renamed 'PKT_RX_EIP_CKSUM_BAD' to 'PKT_RX_OUTER_IP_CKSUM_BAD'.
> > > * Fixed the bug of checking error bits of 'Descriptor oversize' and
> > >   'Header buffer oversize'.
> > > * Added debug logs for each RX error.
> > [...]
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -83,12 +83,7 @@ extern "C" {
> > >  #define PKT_RX_RSS_HASH      (1ULL << 1)  /**< RX packet with RSS
> > hash result. */
> > >  #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_IP_CKSUM_BAD  (1ULL << 4)  /**< IP (or inner IP)
> > > +header checksum error. */
> > 
> > It can be also an outer IP header in case the device don't support tunneling or is
> > not configured to detect it.
> 
> For non-tunneling case, no outer/inner at all, it just has the IP header. The bit flag
> indicates the IP header checksum error.
> For tunneling case, this bit flag indicates the inner IP header checksum error, another
> one for outer IP header checksum error.
> So I don't think this bit can be treated as outer.

I think you didn't understand my comment.
I talk about NICs which don't have tunneling support.
In this case, the outer IP header is seen as a simple IP header.
So, depending on which port is receiving a tunneled packet, this
flag or the dedicated one can be used for outer IP checksum.

I just suggest to remove the part "(or inner IP)" of the comment.
Do you agree?

-- 
Thomas

Re: [PATCH v4] mbuf: fix of enabling all newly added RX error flags

[Zhang, Helin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Wednesday, December 10, 2014 11:26 PM
> To: Zhang, Helin
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH v4] mbuf: fix of enabling all newly added RX
> error flags
> 
> 2014-12-10 13:50, Zhang, Helin:
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> > > Sent: Wednesday, December 10, 2014 5:35 PM
> > > To: Zhang, Helin
> > > Cc: dev-VfR2kkLFssw@public.gmane.org
> > > Subject: Re: [dpdk-dev] [PATCH v4] mbuf: fix of enabling all newly
> > > added RX error flags
> > >
> > > 2014-12-10 16:55, 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
> uses 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > ---
> > > >  lib/librte_mbuf/rte_mbuf.c      |  7 ++-----
> > > >  lib/librte_mbuf/rte_mbuf.h      |  9 +++------
> > > >  lib/librte_pmd_i40e/i40e_rxtx.c | 37
> > > > ++++++++++++++++++++-----------------
> > > >  3 files changed, 25 insertions(+), 28 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.
> > > >
> > > > v4 changes:
> > > > * Renamed 'PKT_RX_EIP_CKSUM_BAD' to
> 'PKT_RX_OUTER_IP_CKSUM_BAD'.
> > > > * Fixed the bug of checking error bits of 'Descriptor oversize' and
> > > >   'Header buffer oversize'.
> > > > * Added debug logs for each RX error.
> > > [...]
> > > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > > @@ -83,12 +83,7 @@ extern "C" {
> > > >  #define PKT_RX_RSS_HASH      (1ULL << 1)  /**< RX packet with
> RSS
> > > hash result. */
> > > >  #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_IP_CKSUM_BAD  (1ULL << 4)  /**< IP (or inner IP)
> > > > +header checksum error. */
> > >
> > > It can be also an outer IP header in case the device don't support
> > > tunneling or is not configured to detect it.
> >
> > For non-tunneling case, no outer/inner at all, it just has the IP
> > header. The bit flag indicates the IP header checksum error.
> > For tunneling case, this bit flag indicates the inner IP header
> > checksum error, another one for outer IP header checksum error.
> > So I don't think this bit can be treated as outer.
> 
> I think you didn't understand my comment.
> I talk about NICs which don't have tunneling support.
> In this case, the outer IP header is seen as a simple IP header.
> So, depending on which port is receiving a tunneled packet, this flag or the
> dedicated one can be used for outer IP checksum.
I think I did understand your point. For those port which does not support tunneling,
if a 'tunneling' packet received, it never knows that's tunneling packet, it always treats
it as a general IP packet. The "inner" IP is just part of its data. For this case, no outer
or inner at all, just an IP header.

> 
> I just suggest to remove the part "(or inner IP)" of the comment.
> Do you agree?
I got it, actually I wanted to describe it as (or inner IP for tunneling), as the macro name
does not tell it could be inner IP header checksum error for tunneling case.

Regards,
Helin
> 
> --
> Thomas

Re: [PATCH v4] mbuf: fix of enabling all newly added RX error flags

[Olivier MATZ]

Hi Helin,

On 12/10/2014 11:29 PM, Zhang, Helin wrote:
>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>>>> @@ -83,12 +83,7 @@ extern "C" {
>>>>>   #define PKT_RX_RSS_HASH      (1ULL << 1)  /**< RX packet with
>> RSS
>>>> hash result. */
>>>>>   #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_IP_CKSUM_BAD  (1ULL << 4)  /**< IP (or inner IP)
>>>>> +header checksum error. */
>>>>
>>>> It can be also an outer IP header in case the device don't support
>>>> tunneling or is not configured to detect it.
>>>
>>> For non-tunneling case, no outer/inner at all, it just has the IP
>>> header. The bit flag indicates the IP header checksum error.
>>> For tunneling case, this bit flag indicates the inner IP header
>>> checksum error, another one for outer IP header checksum error.
>>> So I don't think this bit can be treated as outer.
>>
>> I think you didn't understand my comment.
>> I talk about NICs which don't have tunneling support.
>> In this case, the outer IP header is seen as a simple IP header.
>> So, depending on which port is receiving a tunneled packet, this flag or the
>> dedicated one can be used for outer IP checksum.
> I think I did understand your point. For those port which does not support tunneling,
> if a 'tunneling' packet received, it never knows that's tunneling packet, it always treats
> it as a general IP packet. The "inner" IP is just part of its data. For this case, no outer
> or inner at all, just an IP header.
>
>>
>> I just suggest to remove the part "(or inner IP)" of the comment.
>> Do you agree?
> I got it, actually I wanted to describe it as (or inner IP for tunneling), as the macro name
> does not tell it could be inner IP header checksum error for tunneling case.

I still don't understand how to use that flag. Let's imagine an
application that processes an IP packet:

   ip_input(m) /* receive a packet after ethernet header is stripped */
   {
     if (m->ol_flags & PKT_RX_IP_CKSUM_BAD) {
       log("packet dropped");
       rte_pktmbuf_free(m);
       return;
     }
     /* continue IP header processing,maybe route the packet? */
     ...

This kind of code works since a long time with dpdk on ixgbe, even
if you receive a tunnel packet.

In my understanding, with your patch, if you receive a tunnel packet on
i40e, the flag PKT_RX_IP_CKSUM_BAD is about the inner header, which
should not be checked by a router. This would make the code above
not working anymore. Am I correct?

By the way (it's a bit out of topic), as we already noticed on the
list some times, in the future we should add another flags
PKT_RX_IP_CKSUM_VERIFIED in addition to PKT_RX_IP_CKSUM_BAD because
many drivers do not support hardware checksum, or only supports it in
specific conditions (ex: no IP options, or no vlan, ...). We should
think about it for 2.0.

Regards,
Olivier

Re: [PATCH v4] mbuf: fix of enabling all newly added RX error flags

[Zhang, Helin]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Thursday, December 11, 2014 7:16 PM
> To: Zhang, Helin; Thomas Monjalon
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH v4] mbuf: fix of enabling all newly added RX
> error flags
> 
> Hi Helin,
> 
> On 12/10/2014 11:29 PM, Zhang, Helin wrote:
> >>>>> --- a/lib/librte_mbuf/rte_mbuf.h
> >>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
> >>>>> @@ -83,12 +83,7 @@ extern "C" {
> >>>>>   #define PKT_RX_RSS_HASH      (1ULL << 1)  /**< RX packet with
> >> RSS
> >>>> hash result. */
> >>>>>   #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_IP_CKSUM_BAD  (1ULL << 4)  /**< IP (or inner IP)
> >>>>> +header checksum error. */
> >>>>
> >>>> It can be also an outer IP header in case the device don't support
> >>>> tunneling or is not configured to detect it.
> >>>
> >>> For non-tunneling case, no outer/inner at all, it just has the IP
> >>> header. The bit flag indicates the IP header checksum error.
> >>> For tunneling case, this bit flag indicates the inner IP header
> >>> checksum error, another one for outer IP header checksum error.
> >>> So I don't think this bit can be treated as outer.
> >>
> >> I think you didn't understand my comment.
> >> I talk about NICs which don't have tunneling support.
> >> In this case, the outer IP header is seen as a simple IP header.
> >> So, depending on which port is receiving a tunneled packet, this flag
> >> or the dedicated one can be used for outer IP checksum.
> > I think I did understand your point. For those port which does not
> > support tunneling, if a 'tunneling' packet received, it never knows
> > that's tunneling packet, it always treats it as a general IP packet.
> > The "inner" IP is just part of its data. For this case, no outer or inner at all,
> just an IP header.
> >
> >>
> >> I just suggest to remove the part "(or inner IP)" of the comment.
> >> Do you agree?
> > I got it, actually I wanted to describe it as (or inner IP for
> > tunneling), as the macro name does not tell it could be inner IP header
> checksum error for tunneling case.
> 
> I still don't understand how to use that flag. Let's imagine an application that
> processes an IP packet:
> 
>    ip_input(m) /* receive a packet after ethernet header is stripped */
>    {
>      if (m->ol_flags & PKT_RX_IP_CKSUM_BAD) {
>        log("packet dropped");
>        rte_pktmbuf_free(m);
>        return;
>      }
>      /* continue IP header processing,maybe route the packet? */
>      ...
> 
> This kind of code works since a long time with dpdk on ixgbe, even if you receive
> a tunnel packet.
I have the similar understanding of yours, though I am not sure how the real users use it.
I think the real users always want to have more error information at runtime, then they
know the root cause and how to deal with it.
For checksum errors, they are in packets which come from peer. If this type of errors is
detected, the end users can check what happens on the peer, but not debug on itself.

> 
> In my understanding, with your patch, if you receive a tunnel packet on i40e,
> the flag PKT_RX_IP_CKSUM_BAD is about the inner header, which should not
> be checked by a router. This would make the code above not working anymore.
> Am I correct?
For a tunnel packet received, I think both inner and outer checksum errors should
be checked. And even the inner is more important than outer, as the inner IP could
be the real IP packet which is wanted to be processed.

> 
> By the way (it's a bit out of topic), as we already noticed on the list some times,
> in the future we should add another flags PKT_RX_IP_CKSUM_VERIFIED in
> addition to PKT_RX_IP_CKSUM_BAD because many drivers do not support
> hardware checksum, or only supports it in specific conditions (ex: no IP options,
> or no vlan, ...). We should think about it for 2.0.
Good reason for new flags. But I think it may need another bit for outer IP checksum?
Is there any other choice to indicate the checksum is not offloaded somewhere else?
Or can it adds a bit flag like PKT_RX_IP_CKSUM_NOT_OFFLOADED?

Regards,
Helin

> 
> Regards,
> Olivier