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

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Helin,

On 11/28/2014 09:07 AM, Zhang, Helin wrote:
> 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.

ok.

But it would be nice to have a better definition of this flag: does
external mean outer header? For instance, when you receive a
Ether/IP1/UDP/vxlan/Ether/IP2/xxx, does the flag concerns IP1 or IP2?

If it's IP1, it's really strange compared to the current behavior (the
flag PKT_RX_IP_CKSUM_BAD refers to IP1).

> #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.

Again, a statistic counter would do the job which if it's just to
provide a hint to the application.

I wonder in which case this flag can happen. If you fill the ring with
mbufs that are large enough compared to your ethernet network, this
should not happen in normal conditions. I really don't believe that
an application receiving an mbuf with this flag would stop the driver,
then refill the rings it with larger mbufs.

Last but not least: If it's really useful, should we have the same
behavior on other drivers like ixgbe? I think we really need to care
about not having different ways to use the different drivers.

To me, the only argument in favor of keeping this flag is when the mbuf
contains a part of the data that could be dumped by a user for debug
purposes.

> #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.

Same for this one.

> #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.

ok

> #define PKT_RX_MAC_ERR       (0ULL << 0)  /**< MAC error. */
> [Helin] This indicates a real hardware error happens.

And what is the content of the mbuf data in this case? Does the
application really need an mbuf?


Regards,
Olivier