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

["Zhang, Helin" <helin.zhang@intel.com>]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Thursday, December 11, 2014 7:16 PM
> To: Zhang, Helin; Thomas Monjalon
> Cc: dev@dpdk.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