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

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

Hi Olivier, Konstantin

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Wednesday, November 26, 2014 10:12 PM
> 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,
> 
> 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