[2/8] ethdev: add dev op for IP reassembly configuration
Checks
Commit Message
A new ethernet device op is added to give application control over
the IP reassembly configuration. This operation is an optional
call from the application, default values are set by PMD and
exposed via rte_eth_dev_info.
Application should always first retreive the capabilities from
rte_eth_dev_info and then set the fields accordingly.
Signed-off-by: Akhil Goyal <gakhil@marvell.com>
---
lib/ethdev/ethdev_driver.h | 19 +++++++++++++++++++
lib/ethdev/rte_ethdev.c | 30 ++++++++++++++++++++++++++++++
lib/ethdev/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
lib/ethdev/version.map | 3 +++
4 files changed, 80 insertions(+)
Comments
> A new ethernet device op is added to give application control over
> the IP reassembly configuration. This operation is an optional
> call from the application, default values are set by PMD and
> exposed via rte_eth_dev_info.
> Application should always first retreive the capabilities from
> rte_eth_dev_info and then set the fields accordingly.
>
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> ---
> lib/ethdev/ethdev_driver.h | 19 +++++++++++++++++++
> lib/ethdev/rte_ethdev.c | 30 ++++++++++++++++++++++++++++++
> lib/ethdev/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> lib/ethdev/version.map | 3 +++
> 4 files changed, 80 insertions(+)
>
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index d95605a355..0ed53c14f3 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -990,6 +990,22 @@ typedef int (*eth_representor_info_get_t)(struct rte_eth_dev *dev,
> typedef int (*eth_rx_metadata_negotiate_t)(struct rte_eth_dev *dev,
> uint64_t *features);
>
> +/**
> + * @internal
> + * Set configuration parameters for enabling IP reassembly offload in hardware.
> + *
> + * @param dev
> + * Port (ethdev) handle
> + *
> + * @param[in] conf
> + * Configuration parameters for IP reassembly.
> + *
> + * @return
> + * Negative errno value on error, zero otherwise
> + */
> +typedef int (*eth_ip_reassembly_conf_set_t)(struct rte_eth_dev *dev,
> + struct rte_eth_ip_reass_params *conf);
> +
> /**
> * @internal A structure containing the functions exported by an Ethernet driver.
> */
> @@ -1186,6 +1202,9 @@ struct eth_dev_ops {
> * kinds of metadata to the PMD
> */
> eth_rx_metadata_negotiate_t rx_metadata_negotiate;
> +
> + /** Set IP reassembly configuration */
> + eth_ip_reassembly_conf_set_t ip_reassembly_conf_set;
> };
>
> /**
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index d9a03f12f9..ecc6c1fe37 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6473,6 +6473,36 @@ rte_eth_rx_metadata_negotiate(uint16_t port_id, uint64_t *features)
> (*dev->dev_ops->rx_metadata_negotiate)(dev, features));
> }
>
> +int
> +rte_eth_ip_reassembly_conf_set(uint16_t port_id,
> + struct rte_eth_ip_reass_params *conf)
> +{
> + struct rte_eth_dev *dev;
> +
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> + dev = &rte_eth_devices[port_id];
Should we check here that device is properly configured, but not started yet?
Another question - if we have reassembly_conf_set() would it make sense to
have also reassembly_conf_get?
So user can retrieve current ip_reassembly config values?
> +
> + if ((dev->data->dev_conf.rxmode.offloads &
> + RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY) == 0) {
> + RTE_ETHDEV_LOG(ERR,
> + "The port (ID=%"PRIu16") is not configured for IP reassembly\n",
> + port_id);
> + return -EINVAL;
> + }
> +
> +
> + if (conf == NULL) {
> + RTE_ETHDEV_LOG(ERR,
> + "Invalid IP reassembly configuration (NULL)\n");
> + return -EINVAL;
> + }
> +
> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->ip_reassembly_conf_set,
> + -ENOTSUP);
> + return eth_err(port_id,
> + (*dev->dev_ops->ip_reassembly_conf_set)(dev, conf));
> +}
> +
> RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
>
> RTE_INIT(ethdev_init_telemetry)
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index d9a03f12f9..ecc6c1fe37 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -6473,6 +6473,36 @@ rte_eth_rx_metadata_negotiate(uint16_t port_id,
> uint64_t *features)
> > (*dev->dev_ops->rx_metadata_negotiate)(dev, features));
> > }
> >
> > +int
> > +rte_eth_ip_reassembly_conf_set(uint16_t port_id,
> > + struct rte_eth_ip_reass_params *conf)
> > +{
> > + struct rte_eth_dev *dev;
> > +
> > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > + dev = &rte_eth_devices[port_id];
>
> Should we check here that device is properly configured, but not started yet?
Ok will add checks for dev->data->dev_configured and dev->data->dev_started
> Another question - if we have reassembly_conf_set() would it make sense to
> have also reassembly_conf_get?
> So user can retrieve current ip_reassembly config values?
>
The set/supported values can be retrieved using rte_eth_dev_info :: reass_capa
> > > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > > index d9a03f12f9..ecc6c1fe37 100644
> > > --- a/lib/ethdev/rte_ethdev.c
> > > +++ b/lib/ethdev/rte_ethdev.c
> > > @@ -6473,6 +6473,36 @@ rte_eth_rx_metadata_negotiate(uint16_t port_id,
> > uint64_t *features)
> > > (*dev->dev_ops->rx_metadata_negotiate)(dev, features));
> > > }
> > >
> > > +int
> > > +rte_eth_ip_reassembly_conf_set(uint16_t port_id,
> > > + struct rte_eth_ip_reass_params *conf)
> > > +{
> > > + struct rte_eth_dev *dev;
> > > +
> > > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > > + dev = &rte_eth_devices[port_id];
> >
> > Should we check here that device is properly configured, but not started yet?
> Ok will add checks for dev->data->dev_configured and dev->data->dev_started
>
> > Another question - if we have reassembly_conf_set() would it make sense to
> > have also reassembly_conf_get?
> > So user can retrieve current ip_reassembly config values?
> >
> The set/supported values can be retrieved using rte_eth_dev_info :: reass_capa
Hmm, I thought rte_eth_dev_info :: reass_capa reports
max supported values, not currently set values.
Did I misunderstand something?
> > > Another question - if we have reassembly_conf_set() would it make sense to
> > > have also reassembly_conf_get?
> > > So user can retrieve current ip_reassembly config values?
> > >
> > The set/supported values can be retrieved using rte_eth_dev_info ::
> reass_capa
>
> Hmm, I thought rte_eth_dev_info :: reass_capa reports
> max supported values, not currently set values.
> Did I misunderstand something?
>
Reassembly configuration is expected to be a one-time setting and is not expected
to change multiple times in the application.
You are correct that rte_eth_dev_info :: reass_capa reports max supported values
by the PMD.
But if somebody uses the _set API, dev_info values will be overwritten.
However, a get API can be added, if we have some use case.
IMO, we can add it later if it will be required.
> > > > Another question - if we have reassembly_conf_set() would it make sense to
> > > > have also reassembly_conf_get?
> > > > So user can retrieve current ip_reassembly config values?
> > > >
> > > The set/supported values can be retrieved using rte_eth_dev_info ::
> > reass_capa
> >
> > Hmm, I thought rte_eth_dev_info :: reass_capa reports
> > max supported values, not currently set values.
> > Did I misunderstand something?
> >
> Reassembly configuration is expected to be a one-time setting and is not expected
> to change multiple times in the application.
> You are correct that rte_eth_dev_info :: reass_capa reports max supported values
> by the PMD.
> But if somebody uses the _set API, dev_info values will be overwritten.
> However, a get API can be added, if we have some use case.
> IMO, we can add it later if it will be required.
Basically you forbid user to reconfigure this feature
during application life-time?
That sounds like a really strange approach to me and
Probably will affect its usability in a negative way.
Wonder why it has to be that restrictive?
Also with the model you suggest, what would happen after user will do:
dev_stop(); dev_configure();?
Would rte_eth_dev_info :: reass_capa be reset to initial values,
or user values will be preserved, or ...?
> > > > > Another question - if we have reassembly_conf_set() would it make sense
> to
> > > > > have also reassembly_conf_get?
> > > > > So user can retrieve current ip_reassembly config values?
> > > > >
> > > > The set/supported values can be retrieved using rte_eth_dev_info ::
> > > reass_capa
> > >
> > > Hmm, I thought rte_eth_dev_info :: reass_capa reports
> > > max supported values, not currently set values.
> > > Did I misunderstand something?
> > >
> > Reassembly configuration is expected to be a one-time setting and is not
> expected
> > to change multiple times in the application.
> > You are correct that rte_eth_dev_info :: reass_capa reports max supported
> values
> > by the PMD.
> > But if somebody uses the _set API, dev_info values will be overwritten.
> > However, a get API can be added, if we have some use case.
> > IMO, we can add it later if it will be required.
>
> Basically you forbid user to reconfigure this feature
> during application life-time?
> That sounds like a really strange approach to me and
> Probably will affect its usability in a negative way.
> Wonder why it has to be that restrictive?
> Also with the model you suggest, what would happen after user will do:
> dev_stop(); dev_configure();?
> Would rte_eth_dev_info :: reass_capa be reset to initial values,
> or user values will be preserved, or ...?
>
I am not restricting the user to not reconfigure the feature.
When dev_configure() is called again after dev_stop(), it will reset the previously
set values to max ones.
However, if you insist the get API can be added. No strong opinion on that.
Hi Konstantin,
> > > > > > Another question - if we have reassembly_conf_set() would it make
> sense
> > to
> > > > > > have also reassembly_conf_get?
> > > > > > So user can retrieve current ip_reassembly config values?
> > > > > >
> > > > > The set/supported values can be retrieved using rte_eth_dev_info ::
> > > > reass_capa
> > > >
> > > > Hmm, I thought rte_eth_dev_info :: reass_capa reports
> > > > max supported values, not currently set values.
> > > > Did I misunderstand something?
> > > >
> > > Reassembly configuration is expected to be a one-time setting and is not
> > expected
> > > to change multiple times in the application.
> > > You are correct that rte_eth_dev_info :: reass_capa reports max supported
> > values
> > > by the PMD.
> > > But if somebody uses the _set API, dev_info values will be overwritten.
> > > However, a get API can be added, if we have some use case.
> > > IMO, we can add it later if it will be required.
> >
> > Basically you forbid user to reconfigure this feature
> > during application life-time?
> > That sounds like a really strange approach to me and
> > Probably will affect its usability in a negative way.
> > Wonder why it has to be that restrictive?
> > Also with the model you suggest, what would happen after user will do:
> > dev_stop(); dev_configure();?
> > Would rte_eth_dev_info :: reass_capa be reset to initial values,
> > or user values will be preserved, or ...?
> >
> I am not restricting the user to not reconfigure the feature.
> When dev_configure() is called again after dev_stop(), it will reset the previously
> set values to max ones.
> However, if you insist the get API can be added. No strong opinion on that.
On another thought, setting dev_info :: reass_capa to a max value and not changing it
in reassembly_conf_set() will make more sense.
The most common case, would be to get the max values and if they are not good
Enough for the application, set lesser values using the new API.
I do not see a use case to get the current values set. However, it may be used for debugging
some driver issue related to these values. But, I believe that can be managed internally
in the PMD. Do you suspect any other use case for get API?
> > > > > > > Another question - if we have reassembly_conf_set() would it make
> > sense
> > > to
> > > > > > > have also reassembly_conf_get?
> > > > > > > So user can retrieve current ip_reassembly config values?
> > > > > > >
> > > > > > The set/supported values can be retrieved using rte_eth_dev_info ::
> > > > > reass_capa
> > > > >
> > > > > Hmm, I thought rte_eth_dev_info :: reass_capa reports
> > > > > max supported values, not currently set values.
> > > > > Did I misunderstand something?
> > > > >
> > > > Reassembly configuration is expected to be a one-time setting and is not
> > > expected
> > > > to change multiple times in the application.
> > > > You are correct that rte_eth_dev_info :: reass_capa reports max supported
> > > values
> > > > by the PMD.
> > > > But if somebody uses the _set API, dev_info values will be overwritten.
> > > > However, a get API can be added, if we have some use case.
> > > > IMO, we can add it later if it will be required.
> > >
> > > Basically you forbid user to reconfigure this feature
> > > during application life-time?
> > > That sounds like a really strange approach to me and
> > > Probably will affect its usability in a negative way.
> > > Wonder why it has to be that restrictive?
> > > Also with the model you suggest, what would happen after user will do:
> > > dev_stop(); dev_configure();?
> > > Would rte_eth_dev_info :: reass_capa be reset to initial values,
> > > or user values will be preserved, or ...?
> > >
> > I am not restricting the user to not reconfigure the feature.
> > When dev_configure() is called again after dev_stop(), it will reset the previously
> > set values to max ones.
> > However, if you insist the get API can be added. No strong opinion on that.
>
> On another thought, setting dev_info :: reass_capa to a max value and not changing it
> in reassembly_conf_set() will make more sense.
Yes, agree.
> The most common case, would be to get the max values and if they are not good
> Enough for the application, set lesser values using the new API.
> I do not see a use case to get the current values set. However, it may be used for debugging
> some driver issue related to these values. But, I believe that can be managed internally
> in the PMD. Do you suspect any other use case for get API?
I think it would be really plausible for both user and ethdev layer to have an ability to get
values that are currently in place.
@@ -990,6 +990,22 @@ typedef int (*eth_representor_info_get_t)(struct rte_eth_dev *dev,
typedef int (*eth_rx_metadata_negotiate_t)(struct rte_eth_dev *dev,
uint64_t *features);
+/**
+ * @internal
+ * Set configuration parameters for enabling IP reassembly offload in hardware.
+ *
+ * @param dev
+ * Port (ethdev) handle
+ *
+ * @param[in] conf
+ * Configuration parameters for IP reassembly.
+ *
+ * @return
+ * Negative errno value on error, zero otherwise
+ */
+typedef int (*eth_ip_reassembly_conf_set_t)(struct rte_eth_dev *dev,
+ struct rte_eth_ip_reass_params *conf);
+
/**
* @internal A structure containing the functions exported by an Ethernet driver.
*/
@@ -1186,6 +1202,9 @@ struct eth_dev_ops {
* kinds of metadata to the PMD
*/
eth_rx_metadata_negotiate_t rx_metadata_negotiate;
+
+ /** Set IP reassembly configuration */
+ eth_ip_reassembly_conf_set_t ip_reassembly_conf_set;
};
/**
@@ -6473,6 +6473,36 @@ rte_eth_rx_metadata_negotiate(uint16_t port_id, uint64_t *features)
(*dev->dev_ops->rx_metadata_negotiate)(dev, features));
}
+int
+rte_eth_ip_reassembly_conf_set(uint16_t port_id,
+ struct rte_eth_ip_reass_params *conf)
+{
+ struct rte_eth_dev *dev;
+
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+ dev = &rte_eth_devices[port_id];
+
+ if ((dev->data->dev_conf.rxmode.offloads &
+ RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY) == 0) {
+ RTE_ETHDEV_LOG(ERR,
+ "The port (ID=%"PRIu16") is not configured for IP reassembly\n",
+ port_id);
+ return -EINVAL;
+ }
+
+
+ if (conf == NULL) {
+ RTE_ETHDEV_LOG(ERR,
+ "Invalid IP reassembly configuration (NULL)\n");
+ return -EINVAL;
+ }
+
+ RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->ip_reassembly_conf_set,
+ -ENOTSUP);
+ return eth_err(port_id,
+ (*dev->dev_ops->ip_reassembly_conf_set)(dev, conf));
+}
+
RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
RTE_INIT(ethdev_init_telemetry)
@@ -5218,6 +5218,34 @@ int rte_eth_representor_info_get(uint16_t port_id,
__rte_experimental
int rte_eth_rx_metadata_negotiate(uint16_t port_id, uint64_t *features);
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Set IP reassembly configuration parameters if device rx offload
+ * flag (RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY) is enabled and the PMD
+ * supports IP reassembly offload. User should first check the
+ * reass_capa in rte_eth_dev_info before setting the configuration.
+ * The values of configuration parameters must not exceed the device
+ * capabilities. The use of this API is optional and if called, it
+ * should be called before rte_eth_dev_start().
+ *
+ * @param port_id
+ * The port identifier of the device.
+ * @param conf
+ * A pointer to rte_eth_ip_reass_params structure.
+ * @return
+ * - (-ENOTSUP) if offload configuration is not supported by device.
+ * - (-EINVAL) if offload is not enabled in rte_eth_conf.
+ * - (-ENODEV) if *port_id* invalid.
+ * - (-EIO) if device is removed.
+ * - (0) on success.
+ */
+__rte_experimental
+int rte_eth_ip_reassembly_conf_set(uint16_t port_id,
+ struct rte_eth_ip_reass_params *conf);
+
+
#include <rte_ethdev_core.h>
/**
@@ -256,6 +256,9 @@ EXPERIMENTAL {
rte_flow_flex_item_create;
rte_flow_flex_item_release;
rte_flow_pick_transfer_proxy;
+
+ #added in 22.03
+ rte_eth_ip_reassembly_conf_set;
};
INTERNAL {