mbuf: add packet offload Rx flag for keep CRC
Checks
Commit Message
After discussion[1], the drivers do not include the CRC in the packet
length calculation. This will cause users to be confused about whether
the mbuf contains CRC data. This patch adds a packet offload Rx flag,
indicating that CRC data exists at the end of the mbuf chain.
[1] https://inbox.dpdk.org/dev/20240206011030.2007689-1-haijie1@huawei.com/
Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
---
lib/mbuf/rte_mbuf.c | 3 ++-
lib/mbuf/rte_mbuf_core.h | 8 +++++++-
2 files changed, 9 insertions(+), 2 deletions(-)
Comments
> From: Dengdui Huang [mailto:huangdengdui@huawei.com]
> Sent: Friday, 24 January 2025 11.00
>
> After discussion[1], the drivers do not include the CRC in the packet
> length calculation. This will cause users to be confused about whether
> the mbuf contains CRC data. This patch adds a packet offload Rx flag,
> indicating that CRC data exists at the end of the mbuf chain.
>
> [1] https://inbox.dpdk.org/dev/20240206011030.2007689-1-
> haijie1@huawei.com/
>
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> ---
Mbufs with F_RX_KEEP_CRC requires much more than this.
If the packet length omits the 4 byte Ethernet CRC, and the last segment only holds the CRC, rte_mbuf_check() will fail and cause panic in rte_mbuf_sanity_check().
And many functions working on segments, such as rte_pktmbuf_copy(), linearize(), etc. need to be patched to check for F_RX_KEEP_CRC when working on the packet. This will degrade performance, and we are also talking about frequently used dataplane functions.
Furthermore, if we really need to support KEEP_CRC with segmented packets, we need to add test cases with the CRC partially in the last segment, and with only the CRC in the last segment, for functions and libraries supporting segmented packets. Regardless if the packet length includes the 4 bytes CRC or not.
KEEP_CRC looks exotic to me, and am worried that full support for KEEP_CRC will impact performance and would be essentially untested in a bunch of libraries. I don't want exotic features impacting the performance of frequently used dataplane functions.
Can you please remind me of the use cases for KEEP_CRC?
Perhaps support for KEEP_CRC could be a build time option (default omitted, for performance and test coverage reasons)?
Alternatively, support for KEEP_CRC could be limited to non-segmented packets?
Or, how about a completely different approach:
Drivers supporting KEEP_CRC can strip the 4 byte CRC (like stripping a VLAN tag) and store it in an mbuf dynfield.
On Fri, 24 Jan 2025 17:59:57 +0800
Dengdui Huang <huangdengdui@huawei.com> wrote:
> diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
> index 559d5ad8a7..c828200ea1 100644
> --- a/lib/mbuf/rte_mbuf.c
> +++ b/lib/mbuf/rte_mbuf.c
> @@ -771,7 +771,7 @@ const char *rte_get_rx_ol_flag_name(uint64_t mask)
> case RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD: return "RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD";
> case RTE_MBUF_F_RX_OUTER_L4_CKSUM_INVALID:
> return "RTE_MBUF_F_RX_OUTER_L4_CKSUM_INVALID";
> -
> + case RTE_MBUF_F_RX_KEEP_CRC: return "RTE_MBUF_F_RX_KEEP_CRC";
> default: return NULL;
> }
DPDK style is to add break line after the case statement.
Please do it for both cases.
On 2025/1/24 22:34, Morten Brørup wrote:
>> From: Dengdui Huang [mailto:huangdengdui@huawei.com]
>> Sent: Friday, 24 January 2025 11.00
>>
>> After discussion[1], the drivers do not include the CRC in the packet
>> length calculation. This will cause users to be confused about whether
>> the mbuf contains CRC data. This patch adds a packet offload Rx flag,
>> indicating that CRC data exists at the end of the mbuf chain.
>>
>> [1] https://inbox.dpdk.org/dev/20240206011030.2007689-1-
>> haijie1@huawei.com/
>>
>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
>> ---
>
> Mbufs with F_RX_KEEP_CRC requires much more than this.
>
> If the packet length omits the 4 byte Ethernet CRC, and the last segment only holds the CRC, rte_mbuf_check() will fail and cause panic in rte_mbuf_sanity_check().
> And many functions working on segments, such as rte_pktmbuf_copy(), linearize(), etc. need to be patched to check for F_RX_KEEP_CRC when working on the packet. This will degrade performance, and we are also talking about frequently used dataplane functions.
>
Currently, when the CRC data is stored at the end of a packet, neither data_len nor pkt_len contains the CRC length. Therefore, using rte_pktmbuf_copy() and linearization() for packets containing CRC data is also problematic.
> Furthermore, if we really need to support KEEP_CRC with segmented packets, we need to add test cases with the CRC partially in the last segment, and with only the CRC in the last segment, for functions and libraries supporting segmented packets. Regardless if the packet length includes the 4 bytes CRC or not.
>
> KEEP_CRC looks exotic to me, and am worried that full support for KEEP_CRC will impact performance and would be essentially untested in a bunch of libraries. I don't want exotic features impacting the performance of frequently used dataplane functions.
> Can you please remind me of the use cases for KEEP_CRC?
>
> Perhaps support for KEEP_CRC could be a build time option (default omitted, for performance and test coverage reasons)?
>
> Alternatively, support for KEEP_CRC could be limited to non-segmented packets?
>
> Or, how about a completely different approach:
> Drivers supporting KEEP_CRC can strip the 4 byte CRC (like stripping a VLAN tag) and store it in an mbuf dynfield.
>
>
It's a good idea to store it in mbuf dynfield. As mentioned above, storing CRC data at the end of the mbuf is very complex and currently imperfect. Can this feature be re-implemented in this simpler way?
On 2025/1/25 1:34, Stephen Hemminger wrote:
> On Fri, 24 Jan 2025 17:59:57 +0800
> Dengdui Huang <huangdengdui@huawei.com> wrote:
>
>> diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
>> index 559d5ad8a7..c828200ea1 100644
>> --- a/lib/mbuf/rte_mbuf.c
>> +++ b/lib/mbuf/rte_mbuf.c
>> @@ -771,7 +771,7 @@ const char *rte_get_rx_ol_flag_name(uint64_t mask)
>> case RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD: return "RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD";
>> case RTE_MBUF_F_RX_OUTER_L4_CKSUM_INVALID:
>> return "RTE_MBUF_F_RX_OUTER_L4_CKSUM_INVALID";
>> -
>> + case RTE_MBUF_F_RX_KEEP_CRC: return "RTE_MBUF_F_RX_KEEP_CRC";
>> default: return NULL;
>> }
>
> DPDK style is to add break line after the case statement.
> Please do it for both cases.
OK,I made a mistake.
@@ -771,7 +771,7 @@ const char *rte_get_rx_ol_flag_name(uint64_t mask)
case RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD: return "RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD";
case RTE_MBUF_F_RX_OUTER_L4_CKSUM_INVALID:
return "RTE_MBUF_F_RX_OUTER_L4_CKSUM_INVALID";
-
+ case RTE_MBUF_F_RX_KEEP_CRC: return "RTE_MBUF_F_RX_KEEP_CRC";
default: return NULL;
}
}
@@ -818,6 +818,7 @@ rte_get_rx_ol_flag_list(uint64_t mask, char *buf, size_t buflen)
NULL },
{ RTE_MBUF_F_RX_OUTER_L4_CKSUM_UNKNOWN, RTE_MBUF_F_RX_OUTER_L4_CKSUM_MASK,
"RTE_MBUF_F_RX_OUTER_L4_CKSUM_UNKNOWN" },
+ { RTE_MBUF_F_RX_KEEP_CRC, RTE_MBUF_F_RX_KEEP_CRC, NULL },
};
const char *name;
unsigned int i;
@@ -180,9 +180,15 @@ extern "C" {
#define RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD (1ULL << 22)
#define RTE_MBUF_F_RX_OUTER_L4_CKSUM_INVALID ((1ULL << 21) | (1ULL << 22))
+/**
+ * Indicates that there is CRC data at the end of the mbuf chain.
+ * Note: pkt_len and date_len fields are not adjusted for CRC data.
+ */
+#define RTE_MBUF_F_RX_KEEP_CRC (1ULL << 23)
+
/* add new RX flags here, don't forget to update RTE_MBUF_F_FIRST_FREE */
-#define RTE_MBUF_F_FIRST_FREE (1ULL << 23)
+#define RTE_MBUF_F_FIRST_FREE (1ULL << 24)
#define RTE_MBUF_F_LAST_FREE (1ULL << 40)
/* add new TX flags here, don't forget to update RTE_MBUF_F_LAST_FREE */