Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 2/28/2024 2:27 AM, huangdengdui wrote:
> 
> 
> On 2024/2/27 0:43, Ferruh Yigit wrote:
>> On 2/26/2024 3:16 AM, Jie Hai wrote:
>>> On 2024/2/23 21:53, Ferruh Yigit wrote:
>>>> On 2/20/2024 3:58 AM, Jie Hai wrote:
>>>>> Hi, Ferruh,
>>>>>
>>>>> Thanks for your review.
>>>>>
>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote:
>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote:
>>>>>>> From: Dengdui Huang <huangdengdui@huawei.com>
>>>>>>>
>>>>>>> When KEEP_CRC offload is enabled, some packets will be truncated and
>>>>>>> the CRC is still be stripped in following cases:
>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length
>>>>>>>      is less than or equal to 60B.
>>>>>>> 2. For other hardwares, the packet type is IP and the length
>>>>>>>      is less than or equal to 60B.
>>>>>>>
>>>>>>
>>>>>> If a device doesn't support the offload by some packets, it can be
>>>>>> option to disable offload for that device, instead of calculating it in
>>>>>> software and append it.
>>>>>
>>>>> The KEEP CRC feature of hns3 is faulty only in the specific packet
>>>>> type and small packet(<60B) case.
>>>>> What's more, the small ethernet packet is not common.
>>>>>
>>>>>> Unless you have a specific usecase, or requirement to support the
>>>>>> offload.
>>>>>
>>>>> Yes, some users of hns3 are already using this feature.
>>>>> So we cannot drop this offload
>>>>>
>>>>>> <...>
>>>>>>
>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>>>>>                goto pkt_err;
>>>>>>>              rxm->packet_type = hns3_rx_calc_ptype(rxq, l234_info,
>>>>>>> ol_info);
>>>>>>> -
>>>>>>>            if (rxm->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
>>>>>>>                rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
>>>>>>>    +        if (unlikely(rxq->crc_len > 0)) {
>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm))
>>>>>>> +                hns3_recalculate_crc(rxq, rxm);
>>>>>>> +            rxm->pkt_len -= rxq->crc_len;
>>>>>>> +            rxm->data_len -= rxq->crc_len;
>>>>>>>
>>>>>>
>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is
>>>>>> practically same as stripping CRC.
>>>>>>
>>>>>> We don't count CRC length in the statistics, but it should be
>>>>>> accessible
>>>>>> in the payload by the user.
>>>>> Our drivers are behaving exactly as you say.
>>>>>
>>>>
>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by
>>>> 'rxq->crc_len', can you please explain what above lines does?
>>>>
>>>>
>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue,
>>>          rxdp->rx.bd_base_info = 0;
>>>
>>>          rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>> -        rxm->pkt_len = (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) -
>>> -                rxq->crc_len;
>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
>>>
>>> In the previous code above, the 'pkt_len' is set to the length obtained
>>> from the BD. the length obtained from the BD already contains CRC length.
>>> But as you said above, the DPDK requires that the length of the mbuf
>>> does not contain CRC length . So we subtract 'rxq->crc_len' from
>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the logic, it
>>> just moves the code around.
>>>
>>
>> Nope, I am not saying mbuf length shouldn't contain CRC length, indeed
>> it is other way around and this is our confusion.
>>
>> CRC length shouldn't be in the statistics, I mean in received bytes stats.
>> Assume that received packet is 128 bytes and we know it has the CRC,
>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124)
>>
>> But mbuf->data_len & mbuf->pkt_len should have full frame length,
>> including CRC.
>>
>> As application explicitly requested to KEEP CRC, it will know last 4
>> bytes are CRC.
>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, so if
>> you reduce 'mbuf->data_len' by CRC size, application can't know if 4
>> bytes after 'mbuf->data_len' is valid CRC or not.
>>
> I agree with you.
> 
> But the implementation of other PMDs supported KEEP_CRC is like this.
> In addition, there are probably many users that are already using it.
> If we modify it, it may cause applications incompatible.
> 
> what do you think?
> 
This is documented in the ethdev [1], better to follow the documentation
for all PMDs, can you please highlight the relevant driver code, we can
discuss it with their maintainers.

Alternatively we can document this additionally in the KEEP_CRC feature
document if it helps for the applications.


[1]
https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257