[dpdk-dev,RFC,2/4] ethdev: introduce Rx queue offloads API
Checks
Commit Message
Introduce a new API to configure Rx offloads.
The new API will re-use existing DEV_RX_OFFLOAD_* flags
to enable the different offloads. This will ease the process
of adding a new Rx offloads, as no ABI breakage is involved.
In addition, the offload configuration can be done per queue,
instead of per port.
The Rx queue offload API can be used only with devices which advertize
the RTE_ETH_DEV_RXQ_OFFLOAD capability.
The old Rx offloads API is kept for the meanwhile, in order to enable a
smooth transition for PMDs and application to the new API.
Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
lib/librte_ether/rte_ethdev.h | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
Comments
Hi,
> Introduce a new API to configure Rx offloads.
>
> The new API will re-use existing DEV_RX_OFFLOAD_* flags
> to enable the different offloads. This will ease the process
> of adding a new Rx offloads, as no ABI breakage is involved.
> In addition, the offload configuration can be done per queue,
> instead of per port.
>
> The Rx queue offload API can be used only with devices which advertize
> the RTE_ETH_DEV_RXQ_OFFLOAD capability.
Not sure why do we need that?
Why (till the deprication) user can't use both old rxmode and new offload flags?
I.E. at rte_ethdev_rx_queue_setup() we always convert rxmode to new offload flags
(as I can see you already have a function for it), then we call dev_ops->rx_queue_setup().
That way both new and old ethdev API should work.
Of course that mean that all PMDs have to support new way to setup rx offloads...
But I suppose that have to be done anyway.
Same for TX path.
Konstantin
>
> The old Rx offloads API is kept for the meanwhile, in order to enable a
> smooth transition for PMDs and application to the new API.
>
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
> lib/librte_ether/rte_ethdev.h | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index f7a44578c..ce33c61c4 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -357,7 +357,14 @@ struct rte_eth_rxmode {
> jumbo_frame : 1, /**< Jumbo Frame Receipt enable. */
> hw_strip_crc : 1, /**< Enable CRC stripping by hardware. */
> enable_scatter : 1, /**< Enable scatter packets rx handler */
> - enable_lro : 1; /**< Enable LRO */
> + enable_lro : 1, /**< Enable LRO */
> + ignore : 1;
> + /**
> + * When set the rxmode offloads should be ignored,
> + * instead the Rx offloads will be set on rte_eth_rxq_conf.
> + * This bit is temporary till rxmode Rx offloads API will
> + * be deprecated.
> + */
> };
>
> /**
> @@ -691,6 +698,12 @@ struct rte_eth_rxq_conf {
> uint16_t rx_free_thresh; /**< Drives the freeing of RX descriptors. */
> uint8_t rx_drop_en; /**< Drop packets if no descriptors are available. */
> uint8_t rx_deferred_start; /**< Do not start queue with rte_eth_dev_start(). */
> + uint64_t offloads;
> + /**
> + * Enable Rx offloads using DEV_RX_OFFLOAD_* flags.
> + * Supported only for devices which advertize the
> + * RTE_ETH_DEV_RXQ_OFFLOAD capability.
> + */
> };
>
> #define ETH_TXQ_FLAGS_NOMULTSEGS 0x0001 /**< nb_segs=1 for all mbufs */
> @@ -907,6 +920,19 @@ struct rte_eth_conf {
> #define DEV_RX_OFFLOAD_QINQ_STRIP 0x00000020
> #define DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000040
> #define DEV_RX_OFFLOAD_MACSEC_STRIP 0x00000080
> +#define DEV_RX_OFFLOAD_HEADER_SPLIT 0x00000100
> +#define DEV_RX_OFFLOAD_VLAN_FILTER 0x00000200
> +#define DEV_RX_OFFLOAD_VLAN_EXTEND 0x00000400
> +#define DEV_RX_OFFLOAD_JUMBO_FRAME 0x00000800
> +#define DEV_RX_OFFLOAD_CRC_STRIP 0x00001000
> +#define DEV_RX_OFFLOAD_SCATTER 0x00002000
> +#define DEV_RX_OFFLOAD_LRO 0x00004000
> +#define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> + DEV_RX_OFFLOAD_UDP_CKSUM | \
> + DEV_RX_OFFLOAD_TCP_CKSUM)
> +#define DEV_RX_OFFLOAD_VLAN (DEV_RX_OFFLOAD_VLAN_STRIP | \
> + DEV_RX_OFFLOAD_VLAN_FILTER | \
> + DEV_RX_OFFLOAD_VLAN_EXTEND)
>
> /**
> * TX offload capabilities of a device.
> @@ -1723,6 +1749,8 @@ struct rte_eth_dev_data {
> #define RTE_ETH_DEV_BONDED_SLAVE 0x0004
> /** Device supports device removal interrupt */
> #define RTE_ETH_DEV_INTR_RMV 0x0008
> +/** Device supports the rte_eth_rxq_conf offloads API */
> +#define RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
>
> /**
> * @internal
> --
> 2.12.0
Wednesday, August 23, 2017 3:21 PM, Ananyev, Konstantin:
> Hi,
>
> > Introduce a new API to configure Rx offloads.
> >
> > The new API will re-use existing DEV_RX_OFFLOAD_* flags to enable the
> > different offloads. This will ease the process of adding a new Rx
> > offloads, as no ABI breakage is involved.
> > In addition, the offload configuration can be done per queue, instead
> > of per port.
> >
> > The Rx queue offload API can be used only with devices which advertize
> > the RTE_ETH_DEV_RXQ_OFFLOAD capability.
>
> Not sure why do we need that?
> Why (till the deprication) user can't use both old rxmode and new offload
> flags?
> I.E. at rte_ethdev_rx_queue_setup() we always convert rxmode to new
> offload flags (as I can see you already have a function for it), then we call
> dev_ops->rx_queue_setup().
> That way both new and old ethdev API should work.
> Of course that mean that all PMDs have to support new way to setup rx
> offloads...
> But I suppose that have to be done anyway.
> Same for TX path.
The reason for this capability is to enable the different PMDs to *gradually move* to the new API.
Offloads management on the PMD is complex, it has to deal with the internal of the device on which they work. For example some devices probably
have offloads configuration they can do only per port. Another example can be that some PMDs enable Tx offloads from the PMD specific parameters (like mlx5).
This is why I believe that the task to move the PMD to the new API should be done by the PMD developers/maintainers.
This work provides the means (the copy functions and the capability bits) for both APIs to co-exists and to be used on top of PMDs which support either old or new API.
Once the majority of the PMD made the transition, we can remove and deprecate this cap (and the old API).
> Konstantin
>
> >
> > The old Rx offloads API is kept for the meanwhile, in order to enable
> > a smooth transition for PMDs and application to the new API.
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > ---
> > lib/librte_ether/rte_ethdev.h | 30 +++++++++++++++++++++++++++++-
> > 1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h index f7a44578c..ce33c61c4 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -357,7 +357,14 @@ struct rte_eth_rxmode {
> > jumbo_frame : 1, /**< Jumbo Frame Receipt enable. */
> > hw_strip_crc : 1, /**< Enable CRC stripping by hardware. */
> > enable_scatter : 1, /**< Enable scatter packets rx handler */
> > - enable_lro : 1; /**< Enable LRO */
> > + enable_lro : 1, /**< Enable LRO */
> > + ignore : 1;
> > + /**
> > + * When set the rxmode offloads should be ignored,
> > + * instead the Rx offloads will be set on rte_eth_rxq_conf.
> > + * This bit is temporary till rxmode Rx offloads API will
> > + * be deprecated.
> > + */
> > };
> >
> > /**
> > @@ -691,6 +698,12 @@ struct rte_eth_rxq_conf {
> > uint16_t rx_free_thresh; /**< Drives the freeing of RX descriptors.
> */
> > uint8_t rx_drop_en; /**< Drop packets if no descriptors are
> available. */
> > uint8_t rx_deferred_start; /**< Do not start queue with
> > rte_eth_dev_start(). */
> > + uint64_t offloads;
> > + /**
> > + * Enable Rx offloads using DEV_RX_OFFLOAD_* flags.
> > + * Supported only for devices which advertize the
> > + * RTE_ETH_DEV_RXQ_OFFLOAD capability.
> > + */
> > };
> >
> > #define ETH_TXQ_FLAGS_NOMULTSEGS 0x0001 /**< nb_segs=1 for all
> mbufs
> > */ @@ -907,6 +920,19 @@ struct rte_eth_conf { #define
> > DEV_RX_OFFLOAD_QINQ_STRIP 0x00000020 #define
> > DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000040
> > #define DEV_RX_OFFLOAD_MACSEC_STRIP 0x00000080
> > +#define DEV_RX_OFFLOAD_HEADER_SPLIT 0x00000100
> > +#define DEV_RX_OFFLOAD_VLAN_FILTER 0x00000200
> > +#define DEV_RX_OFFLOAD_VLAN_EXTEND 0x00000400
> > +#define DEV_RX_OFFLOAD_JUMBO_FRAME 0x00000800
> > +#define DEV_RX_OFFLOAD_CRC_STRIP 0x00001000
> > +#define DEV_RX_OFFLOAD_SCATTER 0x00002000
> > +#define DEV_RX_OFFLOAD_LRO 0x00004000
> > +#define DEV_RX_OFFLOAD_CHECKSUM
> (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> > + DEV_RX_OFFLOAD_UDP_CKSUM | \
> > + DEV_RX_OFFLOAD_TCP_CKSUM)
> > +#define DEV_RX_OFFLOAD_VLAN (DEV_RX_OFFLOAD_VLAN_STRIP | \
> > + DEV_RX_OFFLOAD_VLAN_FILTER | \
> > + DEV_RX_OFFLOAD_VLAN_EXTEND)
> >
> > /**
> > * TX offload capabilities of a device.
> > @@ -1723,6 +1749,8 @@ struct rte_eth_dev_data { #define
> > RTE_ETH_DEV_BONDED_SLAVE 0x0004
> > /** Device supports device removal interrupt */
> > #define RTE_ETH_DEV_INTR_RMV 0x0008
> > +/** Device supports the rte_eth_rxq_conf offloads API */ #define
> > +RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
> >
> > /**
> > * @internal
> > --
> > 2.12.0
07/08/2017 12:54, Shahaf Shuler:
> Introduce a new API to configure Rx offloads.
>
> The new API will re-use existing DEV_RX_OFFLOAD_* flags
> to enable the different offloads. This will ease the process
> of adding a new Rx offloads, as no ABI breakage is involved.
> In addition, the offload configuration can be done per queue,
> instead of per port.
>
> The Rx queue offload API can be used only with devices which advertize
> the RTE_ETH_DEV_RXQ_OFFLOAD capability.
>
> The old Rx offloads API is kept for the meanwhile, in order to enable a
> smooth transition for PMDs and application to the new API.
>
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
[...]
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -357,7 +357,14 @@ struct rte_eth_rxmode {
> jumbo_frame : 1, /**< Jumbo Frame Receipt enable. */
> hw_strip_crc : 1, /**< Enable CRC stripping by hardware. */
> enable_scatter : 1, /**< Enable scatter packets rx handler */
> - enable_lro : 1; /**< Enable LRO */
> + enable_lro : 1, /**< Enable LRO */
> + ignore : 1;
> + /**
> + * When set the rxmode offloads should be ignored,
> + * instead the Rx offloads will be set on rte_eth_rxq_conf.
> + * This bit is temporary till rxmode Rx offloads API will
> + * be deprecated.
> + */
Who is responsible to set the "ignore" flag?
Should it be documented in queue config functions?
> +/** Device supports the rte_eth_rxq_conf offloads API */
> +#define RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
Otherwise, looks good
On 8/7/2017 11:54 AM, Shahaf Shuler wrote:
> Introduce a new API to configure Rx offloads.
>
> The new API will re-use existing DEV_RX_OFFLOAD_* flags
> to enable the different offloads. This will ease the process
> of adding a new Rx offloads, as no ABI breakage is involved.
> In addition, the offload configuration can be done per queue,
> instead of per port.
If a device doesn't have capability to set the offload per queue how
should it behave, I think it is good to define this.
>
> The Rx queue offload API can be used only with devices which advertize
> the RTE_ETH_DEV_RXQ_OFFLOAD capability.
>
> The old Rx offloads API is kept for the meanwhile, in order to enable a
> smooth transition for PMDs and application to the new API.
>
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
<...>
> @@ -357,7 +357,14 @@ struct rte_eth_rxmode {
> jumbo_frame : 1, /**< Jumbo Frame Receipt enable. */
> hw_strip_crc : 1, /**< Enable CRC stripping by hardware. */
> enable_scatter : 1, /**< Enable scatter packets rx handler */
> - enable_lro : 1; /**< Enable LRO */
> + enable_lro : 1, /**< Enable LRO */
> + ignore : 1;
what do you think making this variable more verbose, like
"ignore_rx_offloads"
"dev_conf.rxmode.ignore" doesn't say on its own what is ignored.
> + /**
> + * When set the rxmode offloads should be ignored,
> + * instead the Rx offloads will be set on rte_eth_rxq_conf.
> + * This bit is temporary till rxmode Rx offloads API will
> + * be deprecated.
> + */
> };
<...>
> +/** Device supports the rte_eth_rxq_conf offloads API */
> +#define RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
Since this is temporary flag and with current implementation this is
local to library, should we put this into public header?
Later when all PMDs implemented this new method and we want to remove
the flag, can we remove them or do we have to keep them reserved for any
conflict for further new values?
I guess this should be part of missing pmd-ethdev interface file
(rte_ethdev_pmd.h ?).
>
> /**
> * @internal
>
On 8/7/2017 11:54 AM, Shahaf Shuler wrote:
> Introduce a new API to configure Rx offloads.
>
> The new API will re-use existing DEV_RX_OFFLOAD_* flags
> to enable the different offloads. This will ease the process
> of adding a new Rx offloads, as no ABI breakage is involved.
> In addition, the offload configuration can be done per queue,
> instead of per port.
>
> The Rx queue offload API can be used only with devices which advertize
> the RTE_ETH_DEV_RXQ_OFFLOAD capability.
>
> The old Rx offloads API is kept for the meanwhile, in order to enable a
> smooth transition for PMDs and application to the new API.
>
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
<...>
> /**
> @@ -691,6 +698,12 @@ struct rte_eth_rxq_conf {
> uint16_t rx_free_thresh; /**< Drives the freeing of RX descriptors. */
> uint8_t rx_drop_en; /**< Drop packets if no descriptors are available. */
> uint8_t rx_deferred_start; /**< Do not start queue with rte_eth_dev_start(). */
> + uint64_t offloads;
Also there is a documentation aims to help PMDs on developing features
[1], currently for Rx offload features it documents rxmode fields as to
be used.
Can you please update that document too, according new API, new field to
use, both for Rx and Tx.
[1]
doc/guides/nics/features.rst
Tuesday, August 29, 2017 3:50 PM, Ferruh Yigit:
> On 8/7/2017 11:54 AM, Shahaf Shuler wrote:
> > Introduce a new API to configure Rx offloads.
> >
> > The new API will re-use existing DEV_RX_OFFLOAD_* flags to enable the
> > different offloads. This will ease the process of adding a new Rx
> > offloads, as no ABI breakage is involved.
> > In addition, the offload configuration can be done per queue, instead
> > of per port.
>
> If a device doesn't have capability to set the offload per queue how should it
> behave, I think it is good to define this.
Yes, will add documentation.
How about If device cannot set offloads per queue, then the queue_setup function should return with ENOTSUP ?
>
> >
> > The Rx queue offload API can be used only with devices which advertize
> > the RTE_ETH_DEV_RXQ_OFFLOAD capability.
> >
> > The old Rx offloads API is kept for the meanwhile, in order to enable
> > a smooth transition for PMDs and application to the new API.
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
>
> <...>
>
> > @@ -357,7 +357,14 @@ struct rte_eth_rxmode {
> > jumbo_frame : 1, /**< Jumbo Frame Receipt enable. */
> > hw_strip_crc : 1, /**< Enable CRC stripping by hardware. */
> > enable_scatter : 1, /**< Enable scatter packets rx handler */
> > - enable_lro : 1; /**< Enable LRO */
> > + enable_lro : 1, /**< Enable LRO */
> > + ignore : 1;
>
> what do you think making this variable more verbose, like
> "ignore_rx_offloads"
>
> "dev_conf.rxmode.ignore" doesn't say on its own what is ignored.
Maybe ignore_offloads ? Rx is quite explicit from rxomde.
>
> > + /**
> > + * When set the rxmode offloads should be ignored,
> > + * instead the Rx offloads will be set on rte_eth_rxq_conf.
> > + * This bit is temporary till rxmode Rx offloads API will
> > + * be deprecated.
> > + */
> > };
>
> <...>
>
> > +/** Device supports the rte_eth_rxq_conf offloads API */ #define
> > +RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
> Since this is temporary flag and with current implementation this is local to
> library, should we put this into public header?
>
> Later when all PMDs implemented this new method and we want to remove
> the flag, can we remove them or do we have to keep them reserved for any
> conflict for further new values?
>
> I guess this should be part of missing pmd-ethdev interface file
> (rte_ethdev_pmd.h ?).
Yes it is better fits to inner interface between ethdev and PMDs.
Wondering, do we have other motivation to have such header?
>
> >
> > /**
> > * @internal
> >
@@ -357,7 +357,14 @@ struct rte_eth_rxmode {
jumbo_frame : 1, /**< Jumbo Frame Receipt enable. */
hw_strip_crc : 1, /**< Enable CRC stripping by hardware. */
enable_scatter : 1, /**< Enable scatter packets rx handler */
- enable_lro : 1; /**< Enable LRO */
+ enable_lro : 1, /**< Enable LRO */
+ ignore : 1;
+ /**
+ * When set the rxmode offloads should be ignored,
+ * instead the Rx offloads will be set on rte_eth_rxq_conf.
+ * This bit is temporary till rxmode Rx offloads API will
+ * be deprecated.
+ */
};
/**
@@ -691,6 +698,12 @@ struct rte_eth_rxq_conf {
uint16_t rx_free_thresh; /**< Drives the freeing of RX descriptors. */
uint8_t rx_drop_en; /**< Drop packets if no descriptors are available. */
uint8_t rx_deferred_start; /**< Do not start queue with rte_eth_dev_start(). */
+ uint64_t offloads;
+ /**
+ * Enable Rx offloads using DEV_RX_OFFLOAD_* flags.
+ * Supported only for devices which advertize the
+ * RTE_ETH_DEV_RXQ_OFFLOAD capability.
+ */
};
#define ETH_TXQ_FLAGS_NOMULTSEGS 0x0001 /**< nb_segs=1 for all mbufs */
@@ -907,6 +920,19 @@ struct rte_eth_conf {
#define DEV_RX_OFFLOAD_QINQ_STRIP 0x00000020
#define DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000040
#define DEV_RX_OFFLOAD_MACSEC_STRIP 0x00000080
+#define DEV_RX_OFFLOAD_HEADER_SPLIT 0x00000100
+#define DEV_RX_OFFLOAD_VLAN_FILTER 0x00000200
+#define DEV_RX_OFFLOAD_VLAN_EXTEND 0x00000400
+#define DEV_RX_OFFLOAD_JUMBO_FRAME 0x00000800
+#define DEV_RX_OFFLOAD_CRC_STRIP 0x00001000
+#define DEV_RX_OFFLOAD_SCATTER 0x00002000
+#define DEV_RX_OFFLOAD_LRO 0x00004000
+#define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
+ DEV_RX_OFFLOAD_UDP_CKSUM | \
+ DEV_RX_OFFLOAD_TCP_CKSUM)
+#define DEV_RX_OFFLOAD_VLAN (DEV_RX_OFFLOAD_VLAN_STRIP | \
+ DEV_RX_OFFLOAD_VLAN_FILTER | \
+ DEV_RX_OFFLOAD_VLAN_EXTEND)
/**
* TX offload capabilities of a device.
@@ -1723,6 +1749,8 @@ struct rte_eth_dev_data {
#define RTE_ETH_DEV_BONDED_SLAVE 0x0004
/** Device supports device removal interrupt */
#define RTE_ETH_DEV_INTR_RMV 0x0008
+/** Device supports the rte_eth_rxq_conf offloads API */
+#define RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
/**
* @internal