diff mbox series

[v3,1/3] ethdev: add description for KEEP CRC offload

Message ID 20240719090415.1513301-2-haijie1@huawei.com (mailing list archive)
State Changes Requested, archived
Delegated to: Stephen Hemminger
Headers
Series bugfix about KEEP CRC offload |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Jie Hai July 19, 2024, 9:04 a.m. UTC 
From: Dengdui Huang <huangdengdui@huawei.com>

The data exceeds the pkt_len in mbuf is inavailable for user.
When KEEP CRC offload is enabled, CRC field length should be
included in the pkt_len in mbuf. However, almost of drivers
supported KEEP CRC feature didn't add the CRC data length to
pkt_len. So it is very necessary to add comments for this.

Fixes: 70815c9ecadd ("ethdev: add new offload flag to keep CRC")
Cc: stable@dpdk.org

Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Jie Hai <haijie1@huawei.com>
---
 lib/ethdev/rte_ethdev.h | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Andrew Rybchenko Sept. 5, 2024, 6:33 a.m. UTC | #1 
On 7/19/24 12:04, Jie Hai wrote:
> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> The data exceeds the pkt_len in mbuf is inavailable for user.
> When KEEP CRC offload is enabled, CRC field length should be
> included in the pkt_len in mbuf. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to
> pkt_len. So it is very necessary to add comments for this.
> 
> Fixes: 70815c9ecadd ("ethdev: add new offload flag to keep CRC")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Jie Hai <haijie1@huawei.com>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Stephen Hemminger Nov. 22, 2024, 5:10 p.m. UTC | #2 
On Fri, 19 Jul 2024 17:04:13 +0800
Jie Hai <haijie1@huawei.com> wrote:

> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> The data exceeds the pkt_len in mbuf is inavailable for user.
                                          unavailable

> When KEEP CRC offload is enabled, CRC field length should be
> included in the pkt_len in mbuf. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to
> pkt_len. So it is very necessary to add comments for this.

All drivers must do the same thing, or this is a serious bug
in the drivers. Just changing a comment is not going to be helpful.

To fix this right:
 1. Do a test with one of the original drivers in DPDK that has this
    feature. I would suggest ixgbe, mlx5 or bnxt.

 2. Add a test to the PMD tests that validates this (if there is not
    one already).

 3. Put the documentation in a place where it shows up in user documentation.
    Either in doxygen comment or in doc/guides/nics

 4. Verify that all devices conform to the desired behavior

I can help, but only have some old mlx5 cards to test here.
Just putting comment in ethdev.h is not enough.
Stephen Hemminger Nov. 22, 2024, 5:35 p.m. UTC | #3 
On Fri, 19 Jul 2024 17:04:13 +0800
Jie Hai <haijie1@huawei.com> wrote:

> From: Dengdui Huang <huangdengdui@huawei.com>
> 
> The data exceeds the pkt_len in mbuf is inavailable for user.
> When KEEP CRC offload is enabled, CRC field length should be
> included in the pkt_len in mbuf. However, almost of drivers
> supported KEEP CRC feature didn't add the CRC data length to
> pkt_len. So it is very necessary to add comments for this.
> 
> Fixes: 70815c9ecadd ("ethdev: add new offload flag to keep CRC")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Jie Hai <haijie1@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

If you put the information in doc, users would see it.
Something like this:

diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
index 0508f118fe..63b0331b06 100644
--- a/doc/guides/nics/features.rst
+++ b/doc/guides/nics/features.rst
@@ -470,8 +470,9 @@ protocol operations. See security library and PMD documentation for more details
 CRC offload
 -----------
 
-Supports CRC stripping by hardware.
-A PMD assumed to support CRC stripping by default. PMD should advertise if it supports keeping CRC.
+Supports including the CRC in the received packet.
+A PMD is assumed to support CRC stripping by default,
+PMD should only advertise if it supports keeping CRC.
 
 * **[uses] rte_eth_rxconf,rte_eth_rxmode**: ``offloads:RTE_ETH_RX_OFFLOAD_KEEP_CRC``.
 
diff --git a/doc/guides/prog_guide/mbuf_lib.rst b/doc/guides/prog_guide/mbuf_lib.rst
index 4ad2a21f3f..bea9111ba4 100644
--- a/doc/guides/prog_guide/mbuf_lib.rst
+++ b/doc/guides/prog_guide/mbuf_lib.rst
@@ -207,6 +207,18 @@ The list of flags and their precise meaning is described in the mbuf API
 documentation (rte_mbuf.h). Also refer to the testpmd source code
 (specifically the csumonly.c file) for details.
 
+CRC offload
+~~~~~~~~~~~
+
+Normally the Ethernet Cyclic Redundancy Check (CRC) is *not* included in the mbuf.
+Some Poll Mode Driver's support keeping the received CRC in the mbuf.
+If a packet is received with keep CRC offload setting:
+- the CRC is in included in the mbuf pkt_len and data_len
+- the CRC is present but not checked
+- the mbuf should not be directly transmitted or the received CRC will be include
+  in the transmit
+
+
 Dynamic fields and flags
 ~~~~~~~~~~~~~~~~~~~~~~~~
Jie Hai Nov. 26, 2024, 7:47 a.m. UTC | #4 
Hi, Stephen Hemminger

Thanks for your review.
I will add doc and fix on drivers in the next version.
The test will be done later.

On 2024/11/23 1:10, Stephen Hemminger wrote:
> On Fri, 19 Jul 2024 17:04:13 +0800
> Jie Hai <haijie1@huawei.com> wrote:
> 
>> From: Dengdui Huang <huangdengdui@huawei.com>
>>
>> The data exceeds the pkt_len in mbuf is inavailable for user.
>                                            unavailable
> 
>> When KEEP CRC offload is enabled, CRC field length should be
>> included in the pkt_len in mbuf. However, almost of drivers
>> supported KEEP CRC feature didn't add the CRC data length to
>> pkt_len. So it is very necessary to add comments for this.
> 
> All drivers must do the same thing, or this is a serious bug
> in the drivers. Just changing a comment is not going to be helpful.
> 
> To fix this right:
>   1. Do a test with one of the original drivers in DPDK that has this
>      feature. I would suggest ixgbe, mlx5 or bnxt.
> I can test it on ixgbe and mlx5.
>   2. Add a test to the PMD tests that validates this (if there is not
>      one already).
> 
Maybe later and not come with this patchset.
>   3. Put the documentation in a place where it shows up in user documentation.
>      Either in doxygen comment or in doc/guides/nics
> 
Will add in the next version.
>   4. Verify that all devices conform to the desired behavior
> 
> I can help, but only have some old mlx5 cards to test here.
Thanks.
> Just putting comment in ethdev.h is not enough.
> 

Thanks,
Jie Hai

>
Stephen Hemminger Nov. 26, 2024, 11:51 p.m. UTC | #5 
On Tue, 26 Nov 2024 15:47:32 +0800
Jie Hai <haijie1@huawei.com> wrote:

> Hi, Stephen Hemminger
> 
> Thanks for your review.
> I will add doc and fix on drivers in the next version.
> The test will be done later.
> 
> On 2024/11/23 1:10, Stephen Hemminger wrote:
> > On Fri, 19 Jul 2024 17:04:13 +0800
> > Jie Hai <haijie1@huawei.com> wrote:
> >   
> >> From: Dengdui Huang <huangdengdui@huawei.com>
> >>
> >> The data exceeds the pkt_len in mbuf is inavailable for user.  
> >                                            unavailable
> >   
> >> When KEEP CRC offload is enabled, CRC field length should be
> >> included in the pkt_len in mbuf. However, almost of drivers
> >> supported KEEP CRC feature didn't add the CRC data length to
> >> pkt_len. So it is very necessary to add comments for this.  
> > 
> > All drivers must do the same thing, or this is a serious bug
> > in the drivers. Just changing a comment is not going to be helpful.
> > 
> > To fix this right:
> >   1. Do a test with one of the original drivers in DPDK that has this
> >      feature. I would suggest ixgbe, mlx5 or bnxt.
> > I can test it on ixgbe and mlx5.
> >   2. Add a test to the PMD tests that validates this (if there is not
> >      one already).
> >   
> Maybe later and not come with this patchset.
> >   3. Put the documentation in a place where it shows up in user documentation.
> >      Either in doxygen comment or in doc/guides/nics
> >   
> Will add in the next version.
> >   4. Verify that all devices conform to the desired behavior
> > 
> > I can help, but only have some old mlx5 cards to test here.  
> Thanks.
> > Just putting comment in ethdev.h is not enough.
> >   
> 
> Thanks,
> Jie Hai
> 
> >   

After more discussion, decided to not include this patch (only the two bugfixes).
The proper place to put it is in the programmer's guide, and also you have
discovered a messy place in the driver API. In 10 minutes, already found several
issues in other drivers around this. Like checking wrong bits, dead code, etc.
diff mbox series

Patch

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 548fada1c7ad..1f7237c48af6 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -1550,6 +1550,12 @@  struct rte_eth_conf {
  */
 #define RTE_ETH_RX_OFFLOAD_TIMESTAMP        RTE_BIT64(14)
 #define RTE_ETH_RX_OFFLOAD_SECURITY         RTE_BIT64(15)
+/**
+ * Keep CRC data in packet.
+ *
+ * Note: If this offload is enabled, the pkt_len in mbuf must include
+ * the CRC data length.
+ */
 #define RTE_ETH_RX_OFFLOAD_KEEP_CRC         RTE_BIT64(16)
 #define RTE_ETH_RX_OFFLOAD_SCTP_CKSUM       RTE_BIT64(17)
 #define RTE_ETH_RX_OFFLOAD_OUTER_UDP_CKSUM  RTE_BIT64(18)