[RFC] ethdev: introduce protocol type based header split
Checks
Commit Message
From: Xuan Ding <xuan.ding@intel.com>
Header split consists of splitting a received packet into two separate
regions based on the packet content. Splitting is usually between the
packet header that can be posted to a dedicated buffer and the packet
payload that can be posted to a different buffer. This kind of splitting
is useful in some use cases, such as GPU. GPU can directly process the
payload part and improve the performance significantly.
Currently, Rx buffer split supports length and offset based packet split.
This is not suitable for some NICs that do split based on protocol types.
Tunneling makes the conversion from offset to protocol inaccurate.
This patch extends the current buffer split to support protocol based
header split. A new proto field is introduced in the rte_eth_rxseg_split
structure reserved field to specify header split type.
With Rx offload flag RTE_ETH_RX_OFFLOAD_HEADER_SPLIT enabled and protocol
type configured, PMD will split the ingress packets into two separate
regions. Currently, L2/L3/L4 level header split is supported.
Signed-off-by: Xuan Ding <xuan.ding@intel.com>
Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
---
lib/ethdev/rte_ethdev.c | 2 +-
lib/ethdev/rte_ethdev.h | 17 ++++++++++++++++-
2 files changed, 17 insertions(+), 2 deletions(-)
Comments
03/03/2022 07:01, xuan.ding@intel.com:
> From: Xuan Ding <xuan.ding@intel.com>
>
> Header split consists of splitting a received packet into two separate
> regions based on the packet content. Splitting is usually between the
> packet header that can be posted to a dedicated buffer and the packet
> payload that can be posted to a different buffer. This kind of splitting
> is useful in some use cases, such as GPU. GPU can directly process the
> payload part and improve the performance significantly.
>
> Currently, Rx buffer split supports length and offset based packet split.
> This is not suitable for some NICs that do split based on protocol types.
> Tunneling makes the conversion from offset to protocol inaccurate.
>
> This patch extends the current buffer split to support protocol based
> header split. A new proto field is introduced in the rte_eth_rxseg_split
> structure reserved field to specify header split type.
>
> With Rx offload flag RTE_ETH_RX_OFFLOAD_HEADER_SPLIT enabled and protocol
> type configured, PMD will split the ingress packets into two separate
> regions. Currently, L2/L3/L4 level header split is supported.
>
> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> ---
> lib/ethdev/rte_ethdev.c | 2 +-
> lib/ethdev/rte_ethdev.h | 17 ++++++++++++++++-
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 70c850a2f1..d37c8f9d7e 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1784,7 +1784,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> &dev_info);
> if (ret != 0)
> return ret;
> - } else {
> + } else if (!(rx_conf->offloads & RTE_ETH_RX_OFFLOAD_HEADER_SPLIT)) {
> RTE_ETHDEV_LOG(ERR, "No Rx segmentation offload configured\n");
> return -EINVAL;
> }
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index c2d1f9a972..6743648c22 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1202,7 +1202,8 @@ struct rte_eth_rxseg_split {
> struct rte_mempool *mp; /**< Memory pool to allocate segment from. */
> uint16_t length; /**< Segment data length, configures split point. */
> uint16_t offset; /**< Data offset from beginning of mbuf data buffer. */
> - uint32_t reserved; /**< Reserved field. */
> + uint16_t proto;
If it is not explicitly documented, it cannot be accepted.
What happens if we have a non-0 proto and length/offset defined?
> + uint16_t reserved; /**< Reserved field. */
> };
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice.
This is not a structure.
> + * This enum indicates the header split protocol type
> + */
> +enum rte_eth_rx_header_split_protocol_type {
> + RTE_ETH_RX_HEADER_SPLIT_DEFAULT = 0,
> + RTE_ETH_RX_HEADER_SPLIT_INNER_L2,
> + RTE_ETH_RX_HEADER_SPLIT_OUTER_L2,
> + RTE_ETH_RX_HEADER_SPLIT_IP,
> + RTE_ETH_RX_HEADER_SPLIT_TCP_UDP,
> + RTE_ETH_RX_HEADER_SPLIT_SCTP
> +};
Lack of documentation.
Where the split should happen? before or after the header?
What means DEFAULT?
What means IP, TCP_UDP and SCTP? Is it inner or outer?
On Thu, 3 Mar 2022 06:01:36 +0000
xuan.ding@intel.com wrote:
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index c2d1f9a972..6743648c22 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1202,7 +1202,8 @@ struct rte_eth_rxseg_split {
> struct rte_mempool *mp; /**< Memory pool to allocate segment from. */
> uint16_t length; /**< Segment data length, configures split point. */
> uint16_t offset; /**< Data offset from beginning of mbuf data buffer. */
> - uint32_t reserved; /**< Reserved field. */
> + uint16_t proto;
> + uint16_t reserved; /**< Reserved field. */
> };
This feature suffers from a common bad design pattern.
You can't just start using reserved fields unless the previous versions
enforced that the field was a particular value (usually zero).
There is no guarantee that application will initialize these reserved
fields and now using them risks breaking the API/ABI. It looks like
rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg,
Would have had to check in previous release.
This probably has to wait until 22.11 next API release.
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, March 4, 2022 12:16 AM
> To: Ding, Xuan <xuan.ding@intel.com>
> Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
> andrew.rybchenko@oktetlabs.ru; dev@dpdk.org; viacheslavo@nvidia.com;
> Zhang, Qi Z <qi.z.zhang@intel.com>; Yu, Ping <ping.yu@intel.com>; Wang,
> YuanX <yuanx.wang@intel.com>
> Subject: Re: [RFC] ethdev: introduce protocol type based header split
>
> On Thu, 3 Mar 2022 06:01:36 +0000
> xuan.ding@intel.com wrote:
>
> > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> > c2d1f9a972..6743648c22 100644
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -1202,7 +1202,8 @@ struct rte_eth_rxseg_split {
> > struct rte_mempool *mp; /**< Memory pool to allocate segment from.
> */
> > uint16_t length; /**< Segment data length, configures split point. */
> > uint16_t offset; /**< Data offset from beginning of mbuf data buffer. */
> > - uint32_t reserved; /**< Reserved field. */
> > + uint16_t proto;
> > + uint16_t reserved; /**< Reserved field. */
> > };
>
> This feature suffers from a common bad design pattern.
> You can't just start using reserved fields unless the previous versions enforced
> that the field was a particular value (usually zero).
Yes, agree, that's a mistake there is no document for the reserved field in the previous release, and usually, it should be zero,
And I think one of the typical purposes of the reserved field is to make life easy for new feature adding without breaking ABI.
So, should we just take the risk as I guess it might not be a big deal in real cases?
Thanks
Qi
>
> There is no guarantee that application will initialize these reserved fields and
> now using them risks breaking the API/ABI. It looks like
>
> rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg,
>
> Would have had to check in previous release.
>
> This probably has to wait until 22.11 next API release.
> From: Zhang, Qi Z [mailto:qi.z.zhang@intel.com]
> Sent: Friday, 4 March 2022 10.58
>
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Friday, March 4, 2022 12:16 AM
> >
> > On Thu, 3 Mar 2022 06:01:36 +0000
> > xuan.ding@intel.com wrote:
> >
> > > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index
> > > c2d1f9a972..6743648c22 100644
> > > --- a/lib/ethdev/rte_ethdev.h
> > > +++ b/lib/ethdev/rte_ethdev.h
> > > @@ -1202,7 +1202,8 @@ struct rte_eth_rxseg_split {
> > > struct rte_mempool *mp; /**< Memory pool to allocate segment
> from.
> > */
> > > uint16_t length; /**< Segment data length, configures split
> point. */
> > > uint16_t offset; /**< Data offset from beginning of mbuf data
> buffer. */
> > > - uint32_t reserved; /**< Reserved field. */
> > > + uint16_t proto;
> > > + uint16_t reserved; /**< Reserved field. */
> > > };
> >
> > This feature suffers from a common bad design pattern.
> > You can't just start using reserved fields unless the previous
> versions enforced
> > that the field was a particular value (usually zero).
>
> Yes, agree, that's a mistake there is no document for the reserved
> field in the previous release, and usually, it should be zero,
> And I think one of the typical purposes of the reserved field is to
> make life easy for new feature adding without breaking ABI.
> So, should we just take the risk as I guess it might not be a big deal
> in real cases?
>
In this specific case, I think it can be done with very low risk in real cases.
Assuming that splitting based on fixed length and protocol header parsing is mutually exclusive, the PMDs can simply ignore the "proto" field (and log a warning about it) if the length field is non-zero. This will provide backwards compatibility with applications not zeroing out the 32 bit "reserved" field.
> Thanks
> Qi
>
>
>
> >
> > There is no guarantee that application will initialize these reserved
> fields and
> > now using them risks breaking the API/ABI. It looks like
> >
> > rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split
> *rx_seg,
> >
> > Would have had to check in previous release.
> >
> > This probably has to wait until 22.11 next API release.
On Fri, 4 Mar 2022 09:58:11 +0000
"Zhang, Qi Z" <qi.z.zhang@intel.com> wrote:
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Friday, March 4, 2022 12:16 AM
> > To: Ding, Xuan <xuan.ding@intel.com>
> > Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > andrew.rybchenko@oktetlabs.ru; dev@dpdk.org; viacheslavo@nvidia.com;
> > Zhang, Qi Z <qi.z.zhang@intel.com>; Yu, Ping <ping.yu@intel.com>; Wang,
> > YuanX <yuanx.wang@intel.com>
> > Subject: Re: [RFC] ethdev: introduce protocol type based header split
> >
> > On Thu, 3 Mar 2022 06:01:36 +0000
> > xuan.ding@intel.com wrote:
> >
> > > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> > > c2d1f9a972..6743648c22 100644
> > > --- a/lib/ethdev/rte_ethdev.h
> > > +++ b/lib/ethdev/rte_ethdev.h
> > > @@ -1202,7 +1202,8 @@ struct rte_eth_rxseg_split {
> > > struct rte_mempool *mp; /**< Memory pool to allocate segment from.
> > */
> > > uint16_t length; /**< Segment data length, configures split point. */
> > > uint16_t offset; /**< Data offset from beginning of mbuf data buffer. */
> > > - uint32_t reserved; /**< Reserved field. */
> > > + uint16_t proto;
> > > + uint16_t reserved; /**< Reserved field. */
> > > };
> >
> > This feature suffers from a common bad design pattern.
> > You can't just start using reserved fields unless the previous versions enforced
> > that the field was a particular value (usually zero).
>
> Yes, agree, that's a mistake there is no document for the reserved field in the previous release, and usually, it should be zero,
> And I think one of the typical purposes of the reserved field is to make life easy for new feature adding without breaking ABI.
> So, should we just take the risk as I guess it might not be a big deal in real cases?
There is a cost/benefit tradeoff here. Although HW vendors would like to enable
more features, it really is not that much of an impact to wait until next LTS
for users.
Yes, the API/ABI rules are restrictive, but IMHO it is about learning how to
handle SW upgrades in a more user friendly manner. It was hard for the Linux
kernel to learn how to do this, but after 10 years they mostly have it right.
If this were a bug (especially a security bug), then the rules could be lifted.
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: 2022年3月3日 16:55
> To: Ding, Xuan <xuan.ding@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; andrew.rybchenko@oktetlabs.ru;
> dev@dpdk.org; viacheslavo@nvidia.com; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Yu, Ping <ping.yu@intel.com>; Ding, Xuan <xuan.ding@intel.com>; Wang,
> YuanX <yuanx.wang@intel.com>; ajit.khaparde@broadcom.com;
> jerinj@marvell.com
> Subject: Re: [RFC] ethdev: introduce protocol type based header split
>
> 03/03/2022 07:01, xuan.ding@intel.com:
> > From: Xuan Ding <xuan.ding@intel.com>
> >
> > Header split consists of splitting a received packet into two separate
> > regions based on the packet content. Splitting is usually between the
> > packet header that can be posted to a dedicated buffer and the packet
> > payload that can be posted to a different buffer. This kind of
> > splitting is useful in some use cases, such as GPU. GPU can directly
> > process the payload part and improve the performance significantly.
> >
> > Currently, Rx buffer split supports length and offset based packet split.
> > This is not suitable for some NICs that do split based on protocol types.
> > Tunneling makes the conversion from offset to protocol inaccurate.
> >
> > This patch extends the current buffer split to support protocol based
> > header split. A new proto field is introduced in the
> > rte_eth_rxseg_split structure reserved field to specify header split type.
> >
> > With Rx offload flag RTE_ETH_RX_OFFLOAD_HEADER_SPLIT enabled and
> > protocol type configured, PMD will split the ingress packets into two
> > separate regions. Currently, L2/L3/L4 level header split is supported.
> >
> > Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> > Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> > ---
> > lib/ethdev/rte_ethdev.c | 2 +-
> > lib/ethdev/rte_ethdev.h | 17 ++++++++++++++++-
> > 2 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index
> > 70c850a2f1..d37c8f9d7e 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -1784,7 +1784,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t
> rx_queue_id,
> > &dev_info);
> > if (ret != 0)
> > return ret;
> > - } else {
> > + } else if (!(rx_conf->offloads &
> RTE_ETH_RX_OFFLOAD_HEADER_SPLIT))
> > +{
> > RTE_ETHDEV_LOG(ERR, "No Rx segmentation offload
> configured\n");
> > return -EINVAL;
> > }
> > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> > c2d1f9a972..6743648c22 100644
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -1202,7 +1202,8 @@ struct rte_eth_rxseg_split {
> > struct rte_mempool *mp; /**< Memory pool to allocate segment from.
> */
> > uint16_t length; /**< Segment data length, configures split point. */
> > uint16_t offset; /**< Data offset from beginning of mbuf data buffer. */
> > - uint32_t reserved; /**< Reserved field. */
> > + uint16_t proto;
>
> If it is not explicitly documented, it cannot be accepted.
Thanks for your suggestion. The documentation will be enriched in next version.
Let me give a brief introduction here.
>
> What happens if we have a non-0 proto and length/offset defined?
As Morten said, the proto field is exclude from the length field here.
For buffer split, the length/offset is needed.
For header split, the proto is needed.
As for offset field in header split, by default it is zero, it can also be
configured to decide the beginning of mbuf data buffer.
In conclusion, non-0 proto indicates PMDs can do header split.
Length defines PMDs can do buffer split.
>
> > + uint16_t reserved; /**< Reserved field. */
> > };
> [...]
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice.
>
> This is not a structure.
>
> > + * This enum indicates the header split protocol type */ enum
> > +rte_eth_rx_header_split_protocol_type {
> > + RTE_ETH_RX_HEADER_SPLIT_DEFAULT = 0,
> > + RTE_ETH_RX_HEADER_SPLIT_INNER_L2,
> > + RTE_ETH_RX_HEADER_SPLIT_OUTER_L2,
> > + RTE_ETH_RX_HEADER_SPLIT_IP,
> > + RTE_ETH_RX_HEADER_SPLIT_TCP_UDP,
> > + RTE_ETH_RX_HEADER_SPLIT_SCTP
> > +};
>
> Lack of documentation.
> Where the split should happen? before or after the header?
When header split is configured, the split happens at the boundary of header and payload.
So, after the header, before payload.
> What means DEFAULT?
DEFAULT means no header split protocol type was defined.
As this time, even header split offload is configured in Rx queue,
the PMD won't do header split. Actually, using NONE is more accurate.
> What means IP, TCP_UDP and SCTP? Is it inner or outer?
Since header split happens after the header, so the IP/TCP/UDP/SCTP defines
the header type.
When take inner and outer into consideration, The definition should be refined here.
For example:
rte_eth_rx_header_split_protocol_type {
RTE_ETH_RX_HEADER_SPLIT_NONE = 0,
RTE_ETH_RX_HEADER_SPLIT_MAC,
RTE_ETH_RX_HEADER_SPLIT_IPV4,
RTE_ETH_RX_HEADER_SPLIT_IPV6,
RTE_ETH_RX_HEADER_SPLIT_L3,
RTE_ETH_RX_HEADER_SPLIT_TCP,
RTE_ETH_RX_HEADER_SPLIT_UDP,
RTE_ETH_RX_HEADER_SPLIT_SCTP,
RTE_ETH_RX_HEADER_SPLIT_L4,
RTE_ETH_RX_HEADER_SPLIT_INNER_MAC,
RTE_ETH_RX_HEADER_SPLIT_INNER_IPV4,
RTE_ETH_RX_HEADER_SPLIT_INNER_IPV6,
RTE_ETH_RX_HEADER_SPLIT_INNER_L3,
RTE_ETH_RX_HEADER_SPLIT_INNER_TCP,
RTE_ETH_RX_HEADER_SPLIT_INNER_UDP,
RTE_ETH_RX_HEADER_SPLIT_INNER_SCTP,
RTE_ETH_RX_HEADER_SPLIT_INNER_L4,
};
Considering some NICs don’t distinguish the L2/L3/L4 in header split,
a separate L2/L3/L4 is also defined.
Thanks,
Xuan
>
From: Wenxuan Wu <wenxuanx.wu@intel.com>
Buffer split consists of splitting a received packet into two separate
regions based on the packet content. It is useful in some scenarios,
such as GPU acceleration. The splitting will help to enable true zero
copy and hence improve the performance significantly.
This patchset extends the current buffer split to support multi protocol
headers split. When Rx queue is configured with buffer split feature,
packets received will be directly splitted into two different mempools.
v6->v7:
*fix supported header protocol check.
*add rxhdrs commands and parameters.
v5->v6:
*Change doc and designi of struct rte_eth_rxseg_split, support multi
segments protocol_hdr configuration.
v4->v5:
* Use protocol and mbuf_offset based buffer split instead of header split.
* Use RTE_PTYPE* instead of enum rte_eth_rx_header_split_protocol_type.
* Improve the description of rte_eth_rxseg_split.proto.
v3->v4:
* Use RTE_ETH_RX_HEADER_SPLIT_NONE instead of 0.
v2->v3:
* Fix a PMD bug.
* Add rx queue header split check.
* Revise the log and doc.
v1->v2:
* Add support for all header split protocol types.
Wenxuan Wu (3):
ethdev: introduce protocol header based buffer split
net/ice: support buffer split in Rx path
app/testpmd: add rxhdrs commands and parameters
app/test-pmd/cmdline.c | 127 ++++++++++++++-
app/test-pmd/config.c | 81 ++++++++++
app/test-pmd/parameters.c | 15 +-
app/test-pmd/testpmd.c | 6 +-
app/test-pmd/testpmd.h | 6 +
drivers/net/ice/ice_ethdev.c | 10 +-
drivers/net/ice/ice_rxtx.c | 220 ++++++++++++++++++++++----
drivers/net/ice/ice_rxtx.h | 16 ++
drivers/net/ice/ice_rxtx_vec_common.h | 3 +
lib/ethdev/rte_ethdev.c | 40 ++++-
lib/ethdev/rte_ethdev.h | 28 +++-
11 files changed, 505 insertions(+), 47 deletions(-)
@@ -1784,7 +1784,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
&dev_info);
if (ret != 0)
return ret;
- } else {
+ } else if (!(rx_conf->offloads & RTE_ETH_RX_OFFLOAD_HEADER_SPLIT)) {
RTE_ETHDEV_LOG(ERR, "No Rx segmentation offload configured\n");
return -EINVAL;
}
@@ -1202,7 +1202,8 @@ struct rte_eth_rxseg_split {
struct rte_mempool *mp; /**< Memory pool to allocate segment from. */
uint16_t length; /**< Segment data length, configures split point. */
uint16_t offset; /**< Data offset from beginning of mbuf data buffer. */
- uint32_t reserved; /**< Reserved field. */
+ uint16_t proto;
+ uint16_t reserved; /**< Reserved field. */
};
/**
@@ -1664,6 +1665,20 @@ struct rte_eth_conf {
RTE_ETH_RX_OFFLOAD_QINQ_STRIP)
#define DEV_RX_OFFLOAD_VLAN RTE_DEPRECATED(DEV_RX_OFFLOAD_VLAN) RTE_ETH_RX_OFFLOAD_VLAN
+/**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice.
+ * This enum indicates the header split protocol type
+ */
+enum rte_eth_rx_header_split_protocol_type {
+ RTE_ETH_RX_HEADER_SPLIT_DEFAULT = 0,
+ RTE_ETH_RX_HEADER_SPLIT_INNER_L2,
+ RTE_ETH_RX_HEADER_SPLIT_OUTER_L2,
+ RTE_ETH_RX_HEADER_SPLIT_IP,
+ RTE_ETH_RX_HEADER_SPLIT_TCP_UDP,
+ RTE_ETH_RX_HEADER_SPLIT_SCTP
+};
+
/*
* If new Rx offload capabilities are defined, they also must be
* mentioned in rte_rx_offload_names in rte_ethdev.c file.