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

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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