net/hns3: fix Rx packet truncation when KEEP CRC enabled
Checks
Commit Message
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.
So driver has to recaculate positively the CRC of these packets
on above cases.
In addition, to avoid impacting performance, KEEP_CRC is not
supported when NEON or SVE algorithm is used.
Fixes: 8973d7c4ca12 ("net/hns3: support keeping CRC")
Cc: stable@dpdk.org
Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
Signed-off-by: Jie Hai <haijie1@huawei.com>
---
drivers/net/hns3/hns3_rxtx.c | 116 ++++++++++++++++++++------
drivers/net/hns3/hns3_rxtx.h | 9 ++
drivers/net/hns3/hns3_rxtx_vec.c | 3 +-
drivers/net/hns3/hns3_rxtx_vec_neon.h | 19 -----
drivers/net/hns3/hns3_rxtx_vec_sve.c | 3 +-
5 files changed, 103 insertions(+), 47 deletions(-)
Comments
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.
Unless you have a specific usecase, or requirement to support the 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.
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.
>
> .
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?
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.
> .
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.
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?
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
On 2024/2/28 21:07, Ferruh Yigit wrote:
> 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
Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
Do you mean that we add this description in the KEEP_CRC feature document
and notify all drivers that support KEEP_CRC to follow this documentation?
If so, can you merge this patch first?
Then we send a RFC to disscuss it with all PMDs maintainer.
On 2/29/2024 3:58 AM, huangdengdui wrote:
>
>
> On 2024/2/28 21:07, Ferruh Yigit wrote:
>> 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
>
> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
>
I think it is clear that pkt_len and data_len should contain crc_len, we
can ask for more comments.
> Do you mean that we add this description in the KEEP_CRC feature document
> and notify all drivers that support KEEP_CRC to follow this documentation?
>
> If so, can you merge this patch first?
> Then we send a RFC to disscuss it with all PMDs maintainer.
>
Not for drivers, just a suggestion that if we should update feature
documentation with above information for users. So there is no
dependency to features document update.
On 2024/2/29 17:25, Ferruh Yigit wrote:
> On 2/29/2024 3:58 AM, huangdengdui wrote:
>>
>>
>> On 2024/2/28 21:07, Ferruh Yigit wrote:
>>> 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
>>
>> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
>>
>
> I think it is clear that pkt_len and data_len should contain crc_len, we
> can ask for more comments.
This patch doesn't change the logic for hns3 PMD and the implementation of
other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this patch first?
>
>> Do you mean that we add this description in the KEEP_CRC feature document
>> and notify all drivers that support KEEP_CRC to follow this documentation?
>>
>> If so, can you merge this patch first?
>> Then we send a RFC to disscuss it with all PMDs maintainer.
>>
>
> Not for drivers, just a suggestion that if we should update feature
> documentation with above information for users. So there is no
> dependency to features document update.
>
>
Sorry I'm more confused. What should we do next?
On 3/1/2024 6:55 AM, huangdengdui wrote:
>
>
> On 2024/2/29 17:25, Ferruh Yigit wrote:
>> On 2/29/2024 3:58 AM, huangdengdui wrote:
>>>
>>>
>>> On 2024/2/28 21:07, Ferruh Yigit wrote:
>>>> 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
>>>
>>> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
>>>
>>
>> I think it is clear that pkt_len and data_len should contain crc_len, we
>> can ask for more comments.
> This patch doesn't change the logic for hns3 PMD and the implementation of
> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this patch first?
>
If hns3 behaving against the documented behavior, I don't understand why
you are pushing for merging this patch, instead of fixing it.
Other drivers behavior is something else, not directly related to this
patch, but again if you can provide references we can discuss with their
maintainers.
>>
>>> Do you mean that we add this description in the KEEP_CRC feature document
>>> and notify all drivers that support KEEP_CRC to follow this documentation?
>>>
>>> If so, can you merge this patch first?
>>> Then we send a RFC to disscuss it with all PMDs maintainer.
>>>
>>
>> Not for drivers, just a suggestion that if we should update feature
>> documentation with above information for users. So there is no
>> dependency to features document update.
>>
>>
> Sorry I'm more confused. What should we do next?
There is already API documentation about KEEP_CRC, I think that is
already sufficient for driver developers.
I am just brainstorming if updating './doc/guides/nics/features.rst' can
help end user, but it is not an action or blocker for this patch.
Next step is to update this path.
On 2024/3/1 19:10, Ferruh Yigit wrote:
> On 3/1/2024 6:55 AM, huangdengdui wrote:
>>
>>
>> On 2024/2/29 17:25, Ferruh Yigit wrote:
>>> On 2/29/2024 3:58 AM, huangdengdui wrote:
>>>>
>>>>
>>>> On 2024/2/28 21:07, Ferruh Yigit wrote:
>>>>> 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
>>>>
>>>> Currently，this documentation does not describe whether pkt_len and data_len should contain crc_len.
>>>>
>>>
>>> I think it is clear that pkt_len and data_len should contain crc_len, we
>>> can ask for more comments.
>> This patch doesn't change the logic for hns3 PMD and the implementation of
>> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this patch first?
>>
>
> If hns3 behaving against the documented behavior, I don't understand why
> you are pushing for merging this patch, instead of fixing it.
>
>
> Other drivers behavior is something else, not directly related to this
> patch, but again if you can provide references we can discuss with their
> maintainers.
>
>
>>>
>>>> Do you mean that we add this description in the KEEP_CRC feature document
>>>> and notify all drivers that support KEEP_CRC to follow this documentation?
>>>>
>>>> If so, can you merge this patch first?
>>>> Then we send a RFC to disscuss it with all PMDs maintainer.
>>>>
>>>
>>> Not for drivers, just a suggestion that if we should update feature
>>> documentation with above information for users. So there is no
>>> dependency to features document update.
>>>
>>>
>> Sorry I'm more confused. What should we do next?
>
> There is already API documentation about KEEP_CRC, I think that is
> already sufficient for driver developers.
>
> I am just brainstorming if updating './doc/guides/nics/features.rst' can
> help end user, but it is not an action or blocker for this patch.
>
> Next step is to update this path.
>
Hi, Ferruh,
Thanks for your attention.
I have the following suggestions for the email discussion.
Please take the time to see if they make sense.
For pkt_len and data_len, there is no clear document indicating
whether the length should include the CRC.
However, according to the usage of the driver and the APP, it
is obvious that almost all drivers do not include the CRC by default.
The issue you raised about pkt_len/data_len supposedly containing CRC
and users not being able to get CRC has been around for a long
time, at least dating back to DPDK 18.11 when there was no hns3
driver. And there is no clear solution to this problem for now.
This issue is not caused by the current patch and is not related
to the problem to be solved by the current patch.
Therefore, it is recommended that the problem corresponding to the current
patch should be fixed first.
The problem that the pkt_len/data_len should contain the CRC
and the user should access the CRC can be discussed later.
I have the following two options:
1. Modify the corresponding document to specify that pkt_len/data_len
should contain CRC, and modify all related drivers. This requires the
participation and discussion of other driver developers.
2. Users can use rte_pktmbuf_dump() to print packet data.
The length of the packet data to be printed can be specified.
However, the API restricts that the length of the data required is
less than data_len. Therefore, users cannot obtain the CRC value.
However, the result of the rte_pktmbuf_tailroom() API can tell how
many bytes after data_len are accessible. Users can use rte_pktmbuf_tailroom
and keep_crc offload to obtain the CRC value of packets.
> .
Hi, Ferruh
Kindly ping for reply.
Thanks,
Jie Hai
On 2024/3/8 19:36, Jie Hai wrote:
> On 2024/3/1 19:10, Ferruh Yigit wrote:
>> On 3/1/2024 6:55 AM, huangdengdui wrote:
>>>
>>>
>>> On 2024/2/29 17:25, Ferruh Yigit wrote:
>>>> On 2/29/2024 3:58 AM, huangdengdui wrote:
>>>>>
>>>>>
>>>>> On 2024/2/28 21:07, Ferruh Yigit wrote:
>>>>>> 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
>>>>>
>>>>> Currently，this documentation does not describe whether pkt_len and
>>>>> data_len should contain crc_len.
>>>>>
>>>>
>>>> I think it is clear that pkt_len and data_len should contain
>>>> crc_len, we
>>>> can ask for more comments.
>>> This patch doesn't change the logic for hns3 PMD and the
>>> implementation of
>>> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this
>>> patch first?
>>>
>>
>> If hns3 behaving against the documented behavior, I don't understand why
>> you are pushing for merging this patch, instead of fixing it.
>>
>>
>> Other drivers behavior is something else, not directly related to this
>> patch, but again if you can provide references we can discuss with their
>> maintainers.
>>
>>
>>>>
>>>>> Do you mean that we add this description in the KEEP_CRC feature
>>>>> document
>>>>> and notify all drivers that support KEEP_CRC to follow this
>>>>> documentation?
>>>>>
>>>>> If so, can you merge this patch first?
>>>>> Then we send a RFC to disscuss it with all PMDs maintainer.
>>>>>
>>>>
>>>> Not for drivers, just a suggestion that if we should update feature
>>>> documentation with above information for users. So there is no
>>>> dependency to features document update.
>>>>
>>>>
>>> Sorry I'm more confused. What should we do next?
>>
>> There is already API documentation about KEEP_CRC, I think that is
>> already sufficient for driver developers.
>>
>> I am just brainstorming if updating './doc/guides/nics/features.rst' can
>> help end user, but it is not an action or blocker for this patch.
>>
>> Next step is to update this path.
>>
>
>
> Hi, Ferruh,
>
> Thanks for your attention.
> I have the following suggestions for the email discussion.
> Please take the time to see if they make sense.
>
> For pkt_len and data_len, there is no clear document indicating
> whether the length should include the CRC.
> However, according to the usage of the driver and the APP, it
> is obvious that almost all drivers do not include the CRC by default.
>
> The issue you raised about pkt_len/data_len supposedly containing CRC
> and users not being able to get CRC has been around for a long
> time, at least dating back to DPDK 18.11 when there was no hns3
> driver. And there is no clear solution to this problem for now.
>
> This issue is not caused by the current patch and is not related
> to the problem to be solved by the current patch.
> Therefore, it is recommended that the problem corresponding to the current
> patch should be fixed first.
>
> The problem that the pkt_len/data_len should contain the CRC
> and the user should access the CRC can be discussed later.
>
> I have the following two options:
>
> 1. Modify the corresponding document to specify that pkt_len/data_len
> should contain CRC, and modify all related drivers. This requires the
> participation and discussion of other driver developers.
>
> 2. Users can use rte_pktmbuf_dump() to print packet data.
> The length of the packet data to be printed can be specified.
> However, the API restricts that the length of the data required is
> less than data_len. Therefore, users cannot obtain the CRC value.
> However, the result of the rte_pktmbuf_tailroom() API can tell how
> many bytes after data_len are accessible. Users can use
> rte_pktmbuf_tailroom
> and keep_crc offload to obtain the CRC value of packets.
>
>> .
@@ -11,6 +11,7 @@
#include <rte_io.h>
#include <rte_net.h>
#include <rte_malloc.h>
+#include <rte_net_crc.h>
#if defined(RTE_ARCH_ARM64)
#include <rte_cpuflags.h>
#include <rte_vect.h>
@@ -1732,8 +1733,9 @@ hns3_rx_buf_len_calc(struct rte_mempool *mp, uint16_t *rx_buf_len)
}
static int
-hns3_rxq_conf_runtime_check(struct hns3_hw *hw, uint16_t buf_size,
- uint16_t nb_desc)
+hns3_rxq_conf_runtime_check(struct hns3_hw *hw,
+ const struct rte_eth_rxconf *conf,
+ uint16_t buf_size, uint16_t nb_desc)
{
struct rte_eth_dev *dev = &rte_eth_devices[hw->data->port_id];
eth_rx_burst_t pkt_burst = dev->rx_pkt_burst;
@@ -1766,6 +1768,14 @@ hns3_rxq_conf_runtime_check(struct hns3_hw *hw, uint16_t buf_size,
return -EINVAL;
}
}
+
+ if ((conf->offloads & RTE_ETH_RX_OFFLOAD_KEEP_CRC) &&
+ pkt_burst != hns3_recv_pkts_simple &&
+ pkt_burst != hns3_recv_scattered_pkts) {
+ hns3_err(hw, "KEEP_CRC offload is not supported in the current rx function.");
+ return -EINVAL;
+ }
+
return 0;
}
@@ -1802,7 +1812,7 @@ hns3_rx_queue_conf_check(struct hns3_hw *hw, const struct rte_eth_rxconf *conf,
}
if (hw->data->dev_started) {
- ret = hns3_rxq_conf_runtime_check(hw, *buf_size, nb_desc);
+ ret = hns3_rxq_conf_runtime_check(hw, conf, *buf_size, nb_desc);
if (ret) {
hns3_err(hw, "Rx queue runtime setup fail.");
return ret;
@@ -1917,6 +1927,8 @@ hns3_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t nb_desc,
memset(&rxq->err_stats, 0, sizeof(struct hns3_rx_bd_errors_stats));
memset(&rxq->dfx_stats, 0, sizeof(struct hns3_rx_dfx_stats));
+ if (hw->revision < PCI_REVISION_ID_HIP09_A)
+ rxq->keep_crc_fail_only_tcp = true;
/* CRC len set here is used for amending packet length */
if (dev->data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_KEEP_CRC)
rxq->crc_len = RTE_ETHER_CRC_LEN;
@@ -2400,6 +2412,47 @@ hns3_rx_ptp_timestamp_handle(struct hns3_rx_queue *rxq, struct rte_mbuf *mbuf,
pf->rx_timestamp = timestamp;
}
+static inline bool
+hns3_need_recalculate_crc(struct hns3_rx_queue *rxq, struct rte_mbuf *m)
+{
+ uint32_t ptype = m->packet_type;
+
+ if (m->pkt_len > HNS3_KEEP_CRC_OK_MIN_PKT_LEN)
+ return false;
+
+ if (!(RTE_ETH_IS_IPV4_HDR(ptype) || RTE_ETH_IS_IPV6_HDR(ptype)))
+ return false;
+
+ if (rxq->keep_crc_fail_only_tcp)
+ return (ptype & RTE_PTYPE_L4_MASK) == RTE_PTYPE_L4_TCP;
+
+ return true;
+}
+
+static inline void
+hns3_recalculate_crc(struct hns3_rx_queue *rxq, struct rte_mbuf *m)
+{
+ char *append_data;
+ uint32_t crc;
+
+ crc = rte_net_crc_calc(rte_pktmbuf_mtod(m, void *),
+ m->data_len, RTE_NET_CRC32_ETH);
+
+ /*
+ * The hns3 driver requires that mbuf size must be at least 512B.
+ * When CRC is stripped by hardware, the pkt_len must be less than
+ * or equal to 60B. Therefore, the space of the mbuf is enough
+ * to insert the CRC.
+ *
+ * In addition, after CRC is stripped by hardware, pkt_len and data_len
+ * do not contain the CRC length. Therefore, after CRC data is appended
+ * by PMD again, both pkt_len and data_len add the CRC length.
+ */
+ append_data = rte_pktmbuf_append(m, rxq->crc_len);
+ /* The CRC data is binary data and does not care about the byte order. */
+ rte_memcpy(append_data, (void *)&crc, rxq->crc_len);
+}
+
uint16_t
hns3_recv_pkts_simple(void *rx_queue,
struct rte_mbuf **rx_pkts,
@@ -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);
rxm->data_len = rxm->pkt_len;
rxm->port = rxq->port_id;
rxm->hash.rss = rte_le_to_cpu_32(rxd.rx.rss_hash);
@@ -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;
+ }
+
hns3_rxd_to_vlan_tci(rxq, rxm, l234_info, &rxd);
/* Increment bytes counter */
@@ -2662,10 +2720,10 @@ hns3_recv_scattered_pkts(void *rx_queue,
rxm->data_off = RTE_PKTMBUF_HEADROOM;
rxm->data_len = rte_le_to_cpu_16(rxd.rx.size);
+ rxm->next = NULL;
if (!(bd_base_info & BIT(HNS3_RXD_FE_B))) {
last_seg = rxm;
- rxm->next = NULL;
continue;
}
@@ -2679,23 +2737,6 @@ hns3_recv_scattered_pkts(void *rx_queue,
*/
first_seg->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len);
- /*
- * This is the last buffer of the received packet. If the CRC
- * is not stripped by the hardware:
- * - Subtract the CRC length from the total packet length.
- * - If the last buffer only contains the whole CRC or a part
- * of it, free the mbuf associated to the last buffer. If part
- * of the CRC is also contained in the previous mbuf, subtract
- * the length of that CRC part from the data length of the
- * previous mbuf.
- */
- rxm->next = NULL;
- if (unlikely(rxq->crc_len > 0)) {
- first_seg->pkt_len -= rxq->crc_len;
- recalculate_data_len(first_seg, last_seg, rxm, rxq,
- rxm->data_len);
- }
-
first_seg->port = rxq->port_id;
first_seg->hash.rss = rte_le_to_cpu_32(rxd.rx.rss_hash);
first_seg->ol_flags = RTE_MBUF_F_RX_RSS_HASH;
@@ -2721,10 +2762,35 @@ hns3_recv_scattered_pkts(void *rx_queue,
first_seg->packet_type = hns3_rx_calc_ptype(rxq,
l234_info, ol_info);
-
if (first_seg->packet_type == RTE_PTYPE_L2_ETHER_TIMESYNC)
rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP;
+ /*
+ * This is the last buffer of the received packet. If the CRC
+ * is not stripped by the hardware:
+ * - Subtract the CRC length from the total packet length.
+ * - If the last buffer only contains the whole CRC or a part
+ * of it, free the mbuf associated to the last buffer. If part
+ * of the CRC is also contained in the previous mbuf, subtract
+ * the length of that CRC part from the data length of the
+ * previous mbuf.
+ *
+ * In addition, the CRC is still stripped for a kind of packets:
+ * 1. All IP-TCP packet whose the length is less than and equal
+ * to 60 Byte (no CRC) on HIP08 hardware.
+ * 2. All IP packet whose the length is less than and equal to
+ * 60 Byte (no CRC) on other hardware.
+ * In this case, the PMD calculates the CRC and appends it to
+ * mbuf.
+ */
+ if (unlikely(rxq->crc_len > 0)) {
+ if (hns3_need_recalculate_crc(rxq, first_seg))
+ hns3_recalculate_crc(rxq, first_seg);
+ first_seg->pkt_len -= rxq->crc_len;
+ recalculate_data_len(first_seg, last_seg, rxm, rxq,
+ rxm->data_len);
+ }
+
hns3_rxd_to_vlan_tci(rxq, first_seg, l234_info, &rxd);
/* Increment bytes counter */
@@ -178,6 +178,8 @@
(HNS3_TXD_VLD_CMD | HNS3_TXD_FE_CMD | HNS3_TXD_DEFAULT_BDTYPE)
#define HNS3_TXD_SEND_SIZE_SHIFT 16
+#define HNS3_KEEP_CRC_OK_MIN_PKT_LEN 60
+
enum hns3_pkt_l2t_type {
HNS3_L2_TYPE_UNICAST,
HNS3_L2_TYPE_MULTICAST,
@@ -328,6 +330,13 @@ struct hns3_rx_queue {
/* 4 if RTE_ETH_RX_OFFLOAD_KEEP_CRC offload set, 0 otherwise */
uint8_t crc_len;
+ /*
+ * Indicate the range of packet type that fail to keep CRC.
+ * For HIP08 hardware, only the IP-TCP packet type is included.
+ * For other hardwares, all IP packet type is included.
+ */
+ uint8_t keep_crc_fail_only_tcp:1;
+
/*
* Indicate whether ignore the outer VLAN field in the Rx BD reported
* by the Hardware. Because the outer VLAN is the PVID if the PVID is
@@ -185,7 +185,8 @@ hns3_rx_check_vec_support(struct rte_eth_dev *dev)
struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
uint64_t offloads_mask = RTE_ETH_RX_OFFLOAD_TCP_LRO |
RTE_ETH_RX_OFFLOAD_VLAN |
- RTE_ETH_RX_OFFLOAD_TIMESTAMP;
+ RTE_ETH_RX_OFFLOAD_TIMESTAMP |
+ RTE_ETH_RX_OFFLOAD_KEEP_CRC;
if (dev->data->scattered_rx)
return -ENOTSUP;
@@ -148,14 +148,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
8, 9, 10, 11, /* rx.rss_hash to rte_mbuf.hash.rss */
};
- uint16x8_t crc_adjust = {
- 0, 0, /* ignore pkt_type field */
- rxq->crc_len, /* sub crc on pkt_len */
- 0, /* ignore high-16bits of pkt_len */
- rxq->crc_len, /* sub crc on data_len */
- 0, 0, 0, /* ignore non-length fields */
- };
-
/* compile-time verifies the shuffle mask */
RTE_BUILD_BUG_ON(offsetof(struct rte_mbuf, pkt_len) !=
offsetof(struct rte_mbuf, rx_descriptor_fields1) + 4);
@@ -171,7 +163,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
uint8x16_t pkt_mb1, pkt_mb2, pkt_mb3, pkt_mb4;
uint64x2_t mbp1, mbp2;
uint16x4_t bd_vld = {0};
- uint16x8_t tmp;
uint64_t stat;
/* calc how many bd valid */
@@ -225,16 +216,6 @@ hns3_recv_burst_vec(struct hns3_rx_queue *__restrict rxq,
pkt_mb3 = vqtbl2q_u8(pkt_mbuf3, shuf_desc_fields_msk);
pkt_mb4 = vqtbl2q_u8(pkt_mbuf4, shuf_desc_fields_msk);
- /* 4 packets remove crc */
- tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb1), crc_adjust);
- pkt_mb1 = vreinterpretq_u8_u16(tmp);
- tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb2), crc_adjust);
- pkt_mb2 = vreinterpretq_u8_u16(tmp);
- tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb3), crc_adjust);
- pkt_mb3 = vreinterpretq_u8_u16(tmp);
- tmp = vsubq_u16(vreinterpretq_u16_u8(pkt_mb4), crc_adjust);
- pkt_mb4 = vreinterpretq_u8_u16(tmp);
-
/* save packet info to rx_pkts mbuf */
vst1q_u8((void *)&sw_ring[pos + 0].mbuf->rx_descriptor_fields1,
pkt_mb1);
@@ -36,8 +36,7 @@ hns3_desc_parse_field_sve(struct hns3_rx_queue *rxq,
/* init rte_mbuf.rearm_data last 64-bit */
rx_pkts[i]->ol_flags = RTE_MBUF_F_RX_RSS_HASH;
rx_pkts[i]->hash.rss = rxdp[i].rx.rss_hash;
- rx_pkts[i]->pkt_len = rte_le_to_cpu_16(rxdp[i].rx.pkt_len) -
- rxq->crc_len;
+ rx_pkts[i]->pkt_len = rte_le_to_cpu_16(rxdp[i].rx.pkt_len);
rx_pkts[i]->data_len = rx_pkts[i]->pkt_len;
l234_info = rxdp[i].rx.l234_info;