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 12/01/2014 02:57 AM, Zhang, Helin wrote:
>>> #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?
> 'E' means 'external', it indicates the (most) outer IP header checksum error. If you
> don't think this name is not so clear, I can change it to 'PKT_RX_OUTER_IP_CHSUM_BAD'.
> For inner IP header checksum error, it will be indicated by PKT_RX_IP_CKSUM_BAD.
> 
>>
>> If it's IP1, it's really strange compared to the current behavior (the flag
>> PKT_RX_IP_CKSUM_BAD refers to IP1).

Ok.
But the real sense of my question was about the behavior which seems
different than with previous hardware. Today, if you receive the packet
Ether/IP1/UDP/vxlan/Ether/IP2/xxx on an ixgbe, the flag
PKT_RX_IP_CKSUM_BAD can be set if the checksum of IP1 is wrong. From
your explanation, I understand that PKT_RX_EIP_CKSUM_BAD would be set
for the same thing on i40e. Is it correct?


>>> #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.
> It seems that we do not maintain a counter for packets in PMD, if I am not wrong. Two
> ways current DPDK is using.
> One is hardware provide registers to do that, we can read it directly when needed.
> The other one is that applications or middle layer sw maintain its own statistics.

rte_eth_stats_get() gives the generic statistics
For specific error stats, rte_eth_xstats_get() can be used from an
application (the driver has to provide the full list of statistics).

>> 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.
> This is not because of it is lack of available RX descriptors. It is because of a hardware
> requirement. FVL hardware requires that it should not use more than 5 rx descriptors
> for receiving a single packet.

I still don't understand what the application should do when the flag
is set. Maybe you could provide an example in l2fwd or testpmd?

>> 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.
> I don't see the similar bit in ixgbe datasheet, but this restriction could be common
> for some other NICs, as it is reasonable from hardware perspective.

In ixgbe, there are other error cases:
- frames shorter than 64 bytes
- oversize (frames larger than MAXFRS)
- ... maybe others?

Should we have a flag for each situation? I think not.


>> 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.
> It is a bit different from previous one, it always has one header buffer, this flag means
> the buffer size is not enough for the header.
> These two flags are because of buffer size or number of buffers. The mbufs are prepared
> in application or up layer software. If these two flags occur, it is easier for up layer software
> to debug, and know different buffers are needed. They do not need to debug PMD, as they
> generally don't want to do.

You say it's easier for the software to debug, but I cannot see the
difference. When it's a statistics counter, you just have to use
rte_eth_xstats_get(), which is an equivalent of "ethtool -S iface"
which gives all the hardware statistics. It will work for any driver
and any application.

If we add these flags, the application have to know about all these
specific flags and how to handle them.

>>> #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?
> Mbuf contains both the data and other information. I prefer to let the up layer software to
> decide how to deal with the packet, no matter it is correct or bad. In addition, even hardware
> errors happened, it still can set a special bit to enable storing the whole packet to the mbuf,
> for debug purpose. Hardware error bit can be used for all hardware errors. As we do not have
> one there, why not add one?

You say "let the up layer software to decide how to deal with the
packet, no matter it is correct or bad". But what can do an application
with a packet if it does not know if the data is correct or bad?

Regards,
Olivier