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

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

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 ;)

Regards,
Olivier