From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 82395A00C2; Wed, 28 Sep 2022 10:19:56 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2C5AE4113C; Wed, 28 Sep 2022 10:19:56 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id E2EF440F16 for ; Wed, 28 Sep 2022 10:19:54 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 3061F62; Wed, 28 Sep 2022 11:19:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 3061F62 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1664353194; bh=p1I/XvxTbUVlf0yuocc24hfmagI8XpaIwN6N2mBABzg=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=dsNau0ORTFFFyryXBwx1GKcuq2lPDML2zUjWMuny/eKtsgN/M2ZazNGmFdVgwE/OZ dQodxHx6bwebpANBPJVGb1INqND5mgxCFHf2617Jaz9RBcBAFIwMU/mfVbCMqZtFgV RejHV19FX3yOuN4Et7wMmLuBos2esCLMLRzIz1fs= Message-ID: <2c0476ed-f729-edf8-5c28-04f45acc5503@oktetlabs.ru> Date: Wed, 28 Sep 2022 11:19:53 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 Subject: Re: [PATCH v2 1/1] ethdev: support congestion management Content-Language: en-US To: skori@marvell.com, Ferruh Yigit , Thomas Monjalon , Ray Kinsella Cc: dev@dpdk.org, Jerin Jacob , "david.marchand@redhat.com" References: <20220713130340.2886839-1-jerinj@marvell.com> <20220919121534.1058884-1-skori@marvell.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220919121534.1058884-1-skori@marvell.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > > 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 [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 > + > +/** > + * @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 > + > +#include > +#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 > #include > #include > #include > @@ -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]