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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, November 26, 2014 11:22 AM
> To: Ananyev, Konstantin; Zhang, Helin; dev@dpdk.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@dpdk.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@intel.com>
> >> ---
> >>   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