Re: [PATCH v2 1/1] ethdev: support congestion management

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

Cc David. See some kind of reincarnation of RTE_FUNC_PTR_*
macro below.

On 9/19/22 15:15, skori@marvell.com wrote:
> From: Jerin Jacob <jerinj@marvell.com>
> 
> NIC HW controllers often come with congestion management support on
> various HW objects such as Rx queue depth or mempool queue depth.
> 
> Also, it can support various modes of operation such as RED
> (Random early discard), WRED etc on those HW objects.
> 
> This patch adds a framework to express such modes(enum rte_cman_mode)
> and introduce (enum rte_eth_cman_obj) to enumerate the different
> objects where the modes can operate on.
> 
> This patch adds RTE_CMAN_RED mode of operation and
> RTE_ETH_CMAN_OBJ_RX_QUEUE, RTE_ETH_CMAN_OBJ_RX_QUEUE_MEMPOOL object.
> 
> Introduced reserved fields in configuration structure
> backed by rte_eth_cman_config_init() to add new configuration
> parameters without ABI breakage.
> 
> Added rte_eth_cman_info_get() API to get the information such as
> supported modes and objects.
> 
> Added rte_eth_cman_config_init(), rte_eth_cman_config_set() APIs
> to configure congestion management on those object with associated mode.
> 
> Finally, Added rte_eth_cman_config_get() API to retrieve the
> applied configuration.
> 
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>

[snip]

> diff --git a/lib/eal/include/rte_cman.h b/lib/eal/include/rte_cman.h
> new file mode 100644
> index 0000000000..1d84ddf0fb
> --- /dev/null
> +++ b/lib/eal/include/rte_cman.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2022 Marvell International Ltd.
> + */
> +
> +#ifndef RTE_CMAN_H
> +#define RTE_CMAN_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <rte_bitops.h>
> +
> +/**
> + * @file
> + * Congestion management related parameters for DPDK.
> + */
> +
> +/** Congestion management modes */
> +enum rte_cman_mode {
> +	/**
> +	 * Congestion based on Random Early Detection.
> +	 *
> +	 * https://en.wikipedia.org/wiki/Random_early_detection
> +	 * http://www.aciri.org/floyd/papers/red/red.html
> +	 * @see struct rte_cman_red_params
> +	 */
> +	RTE_CMAN_RED = RTE_BIT64(0),

I believe enums with 64-bit values are bad.

> +};
> +
> +/**
> + * RED based congestion management configuration parameters.
> + */
> +struct rte_cman_red_params {
> +	/**
> +	 * Minimum threshold (min_th) value
> +	 *
> +	 * Value expressed as percentage. Value must be in 0 to 100(inclusive).
> +	 */
> +	uint8_t min_th;
> +	/**
> +	 * Maximum threshold (max_th) value
> +	 *
> +	 * Value expressed as percentage. Value must be in 0 to 100(inclusive).
> +	 */
> +	uint8_t max_th;
> +	/** Inverse of packet marking probability maximum value (maxp = 1 / maxp_inv) */
> +	uint16_t maxp_inv;
> +};
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* RTE_CMAN_H */

> diff --git a/lib/ethdev/meson.build b/lib/ethdev/meson.build
> index 47bb2625b0..59ad49114f 100644
> --- a/lib/ethdev/meson.build
> +++ b/lib/ethdev/meson.build
> @@ -7,6 +7,7 @@ sources = files(
>           'ethdev_profile.c',
>           'ethdev_trace_points.c',
>           'rte_class_eth.c',
> +        'rte_cman.c',
>           'rte_ethdev.c',
>           'rte_flow.c',
>           'rte_mtr.c',
> diff --git a/lib/ethdev/rte_cman.c b/lib/ethdev/rte_cman.c
> new file mode 100644
> index 0000000000..2093c247d1
> --- /dev/null
> +++ b/lib/ethdev/rte_cman.c
> @@ -0,0 +1,101 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2022 Marvell International Ltd.
> + */
> +
> +#include <stdint.h>
> +
> +#include <rte_errno.h>
> +#include "rte_ethdev.h"
> +#include "ethdev_driver.h"
> +
> +static int
> +eth_err(uint16_t port_id, int ret)
> +{
> +	if (ret == 0)
> +		return 0;
> +
> +	if (rte_eth_dev_is_removed(port_id))
> +		return -EIO;
> +
> +	return ret;
> +}

It is a dup from rte_ethdev.c. I think it should be moved to some 
internal header.

> +
> +#define RTE_CMAN_FUNC_ERR_RET(func)					\

There is nothing CMAN specific in the function except location.

Cc David who deprecated similar macros in the release cycle,
but this one have a log message.

> +do {									\
> +	if (func == NULL) {						\
> +		RTE_ETHDEV_LOG(ERR, "Function not implemented\n");	\
> +		return -ENOTSUP;					\
> +	}								\
> +} while (0)
> +
> +/* Get congestion management information for a port */
> +int
> +rte_eth_cman_info_get(uint16_t port_id, struct rte_eth_cman_info *info)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (info == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "congestion management info is NULL\n");
> +		return -EINVAL;
> +	}
> +
> +	RTE_CMAN_FUNC_ERR_RET(dev->dev_ops->cman_info_get);

I think we should memset(&info, 0, sizeof(*info)) here since
all drivers must do it anyway.

> +	return eth_err(port_id, (*dev->dev_ops->cman_info_get)(dev, info));
> +}
> +
> +/* Initialize congestion management structure with default values */
> +int
> +rte_eth_cman_config_init(uint16_t port_id, struct rte_eth_cman_config *config)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (config == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "congestion management config is NULL\n");
> +		return -EINVAL;
> +	}
> +
> +	RTE_CMAN_FUNC_ERR_RET(dev->dev_ops->cman_config_init);
> +	return eth_err(port_id, (*dev->dev_ops->cman_config_init)(dev, config));
> +}
> +
> +/* Configure congestion management on a port */
> +int
> +rte_eth_cman_config_set(uint16_t port_id, struct rte_eth_cman_config *config)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (config == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "congestion management config is NULL\n");
> +		return -EINVAL;
> +	}
> +
> +	RTE_CMAN_FUNC_ERR_RET(dev->dev_ops->cman_config_set);
> +	return eth_err(port_id, (*dev->dev_ops->cman_config_set)(dev, config));
> +}
> +
> +/* Retrieve congestion management configuration of a port */
> +int
> +rte_eth_cman_config_get(uint16_t port_id, struct rte_eth_cman_config *config)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (config == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "congestion management config is NULL\n");
> +		return -EINVAL;
> +	}
> +
> +	RTE_CMAN_FUNC_ERR_RET(dev->dev_ops->cman_config_get);

memset(&config, 0, sizeof(*config));

> +	return eth_err(port_id, (*dev->dev_ops->cman_config_get)(dev, config));
> +}
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index de9e970d4d..f4bb644c6a 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -160,6 +160,7 @@ extern "C" {
>   #define RTE_ETHDEV_DEBUG_TX
>   #endif
>   
> +#include <rte_cman.h>
>   #include <rte_compat.h>
>   #include <rte_log.h>
>   #include <rte_interrupts.h>
> @@ -5506,6 +5507,156 @@ typedef struct {
>   __rte_experimental
>   int rte_eth_dev_priv_dump(uint16_t port_id, FILE *file);
>   
> +/* Congestion management */
> +
> +/** Enumerate list of ethdev congestion management objects */
> +enum rte_eth_cman_obj {
> +	/** Congestion management based on Rx queue depth */
> +	RTE_ETH_CMAN_OBJ_RX_QUEUE = RTE_BIT64(0),
> +	/**
> +	 * Congestion management based on mempool depth associated with Rx queue
> +	 * @see rte_eth_rx_queue_setup()
> +	 */
> +	RTE_ETH_CMAN_OBJ_RX_QUEUE_MEMPOOL = RTE_BIT64(1),

Do we really want one more enum with 64-bit initialized member?
IMHO, it should be either defines or 32-bit.

> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change, or be removed, without prior notice
> + *
> + * A structure used to retrieve information of ethdev congestion management.
> + */
> +struct rte_eth_cman_info {
> +	/**
> +	 * Set of supported congestion management modes
> +	 * @see enum rte_cman_mode
> +	 */
> +	uint64_t modes_supported;
> +	/**
> +	 * Set of supported congestion management objects
> +	 * @see enum rte_eth_cman_obj
> +	 */
> +	uint64_t objs_supported;
> +	/** Reserved for future fields */

I think we should highlight that it is set to zeros on get
right now.

> +	uint8_t rsvd[8];
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change, or be removed, without prior notice
> + *
> + * A structure used to configure the ethdev congestion management.
> + */
> +struct rte_eth_cman_config {
> +	/** Congestion management object */
> +	enum rte_eth_cman_obj obj;
> +	/** Congestion management mode */
> +	enum rte_cman_mode mode;
> +	union {
> +		/**
> +		 * Rx queue to configure congestion management.
> +		 *
> +		 * Valid when object is RTE_ETH_CMAN_OBJ_RX_QUEUE or
> +		 * RTE_ETH_CMAN_OBJ_RX_QUEUE_MEMPOOL.
> +		 */
> +		uint16_t rx_queue;
> +		/** Reserved for future fields */

Must be zeros on get and set right now.

> +		uint8_t rsvd_obj_params[4];
> +	} obj_param;
> +	union {
> +		/**
> +		 * RED configuration parameters.
> +		 *
> +		 * Valid when mode is RTE_CMAN_RED.
> +		 */
> +		struct rte_cman_red_params red;
> +		/** Reserved for future fields */
> +		uint8_t rsvd_mode_params[4];

same here.

> +	} mode_param;
> +};
> +

[snip]

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Configure ethdev congestion management
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param config
> + *   A pointer to a structure of type *rte_eth_cman_config* to be configured.
> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if support for cman_config_set does not exist.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EINVAL) if bad parameter.
> + */
> +__rte_experimental
> +int rte_eth_cman_config_set(uint16_t port_id, struct rte_eth_cman_config *config);

Just a niot, but may be config should be 'const'?

[snip]