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 at dpdk.org
>> Cc: Cao, Waterman; Cao, Min; Ananyev, Konstantin; olivier.matz at 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 at 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.
Regards,
Olivier
