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 72DDAA00C4; Wed, 28 Sep 2022 14:21:07 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1EB5C4113D; Wed, 28 Sep 2022 14:21:07 +0200 (CEST) Received: from mail-qt1-f169.google.com (mail-qt1-f169.google.com [209.85.160.169]) by mails.dpdk.org (Postfix) with ESMTP id DD2CB4113C for ; Wed, 28 Sep 2022 14:21:05 +0200 (CEST) Received: by mail-qt1-f169.google.com with SMTP id g12so7794689qts.1 for ; Wed, 28 Sep 2022 05:21:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=oOlO8eWtAGZBHl/it5Jl+F51mNZvFOw3qaPem85ANIg=; b=oWEQo0yy2cZLnGFeUr+tuTAdhq8TWz4eciSbImxjIxD2PeHJkQi9KZtiuZrP52KXhy Azw4iSeZ4WDaIDumB17HhqDQPSWcxXYb+CeVea5cgeDO4DIAGKfCRQBw2DgKMcs07qIY Ixkg9VNywI50IvSYG83Jd8xaf3CK+oB1HSG830D7b43UUeG9geGUD+OywQDH/U7/P8Y1 zasYJisqmkL59CY53d29t9WnO4Ku742awAPPu98ELZsol6mlJekWEW1AtaW7mfCA+Q5R 4MBOm4bxW0ekrCY8iouGEFikAuFXqGe9WDMS6J7kjHsidGzs2rzElic5IHlZNawhAhQJ SvUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=oOlO8eWtAGZBHl/it5Jl+F51mNZvFOw3qaPem85ANIg=; b=3KkcUfSHVR8yRR1aPIFOfVFulZHsXqGXElv+ctpdiGF28F+WATCWzJzUSvdhEFb4Wh RwM+xhYbgtSgGmxn1Wc1jSYxfSRh5XvqQLx3VJKMWMWi2NJ+xG4Pr5E/FtAPk/cye1lO WWcn7K9ob14OYf/kOVQVe8PG/cJ6ynVZ4fght5N+afqjtLZRhKELdCmdV1RsropKCfCd eG7q/esnnTU/hflS+QSPdp8asq0grvX88aIJb1AZEfz9GmPrQGRdFsumNDGAAZCDpEVm 7lG1rK5TSIRenYsHRhZmWnwN9z9mxFIRxB8Nv+0pYJeU7daw0IWXcGpmtW+W6g2zzuRR neww== X-Gm-Message-State: ACrzQf2ZjctsFHyH0768BBhMYVj52yx6SPNGGmS0wZaupN+HMSCtT+o+ DbDpgO7PSTVL/qm8CiVg9byURyy71j6fEulgVbTitTEYdTxWTA== X-Google-Smtp-Source: AMsMyM5HycSDpNMCXADNYXuOiULmN5mtt0PqRv0w2JOFB3AGlP0HPw9njeyRhyXxeW8+y9XgaM8tbp9jIrz8C9J/qfY= X-Received: by 2002:a05:622a:282:b0:35d:4999:10cb with SMTP id z2-20020a05622a028200b0035d499910cbmr8502901qtw.191.1664367665161; Wed, 28 Sep 2022 05:21:05 -0700 (PDT) MIME-Version: 1.0 References: <20220713130340.2886839-1-jerinj@marvell.com> <20220919121534.1058884-1-skori@marvell.com> <2c0476ed-f729-edf8-5c28-04f45acc5503@oktetlabs.ru> In-Reply-To: <2c0476ed-f729-edf8-5c28-04f45acc5503@oktetlabs.ru> From: Jerin Jacob Date: Wed, 28 Sep 2022 17:50:38 +0530 Message-ID: Subject: Re: [PATCH v2 1/1] ethdev: support congestion management To: Andrew Rybchenko Cc: skori@marvell.com, Ferruh Yigit , Thomas Monjalon , Ray Kinsella , dev@dpdk.org, Jerin Jacob , "david.marchand@redhat.com" Content-Type: text/plain; charset="UTF-8" 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 On Wed, Sep 28, 2022 at 1:50 PM Andrew Rybchenko wrote: > > Cc David. See some kind of reincarnation of RTE_FUNC_PTR_* > macro below. > > > + > > +/** > > + * @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. No strong opinion. Will change to 32bit. > > --- /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. Ack. > > > + > > +#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. Yes. It has an additional log function. No strong opinion Will remove this macro. > > > +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. Will do. > > > + 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)); Ack > > > + 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. Will keep it 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. Will change as "Reserved for future fields. Always return as zero when rte_eth_cman_config_get() invoked" > > > + 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. Will change as "Reserved for future fields. Must be set to zero on get and set operation " > > > + 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. Ack > > + } 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'? Yes. We can keep as const. > > [snip]