From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by smtp.lore.kernel.org (Postfix) with ESMTP id F0087C433F5 for ; Thu, 19 May 2022 09:37:58 +0000 (UTC) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4F57540156; Thu, 19 May 2022 11:37:58 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 6790740150 for ; Thu, 19 May 2022 11:37:56 +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 E8AAF8A; Thu, 19 May 2022 12:37:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru E8AAF8A DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1652953076; bh=8FPsj+hUnzO/ZcmINcJnc1NoyqsnmeALQJC1i/AMP4A=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=kqqtev/LGnXyQs4sjsOdWc9nKTM+UjewWwmHBTdbTqTFKnSsN9ZEQAlFxpkg4WYXo N+nGm6fulnbd9qMzy8HDnPeznCJHGftd69ttYsG2adZJVfg7/Kyg4O2FXFXIgNHRzz jgZxcChhBeZz4PcPWxr4ceIwvRQUAJa0wvA+Syac= Message-ID: Date: Thu, 19 May 2022 12:37:55 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [RFC v1 3/7] ethdev: introduce Rx queue based limit watermark Content-Language: en-US To: Spike Du , matan@nvidia.com, viacheslavo@nvidia.com, orika@nvidia.com, thomas@monjalon.net Cc: dev@dpdk.org, rasland@nvidia.com References: <20220401032232.1267376-2-spiked@nvidia.com> <20220506035645.4101714-1-spiked@nvidia.com> <20220506035645.4101714-4-spiked@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220506035645.4101714-4-spiked@nvidia.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 On 5/6/22 06:56, Spike Du wrote: > LWM(limit watermark) is a per Rx queue attribute that notifies dpdk dpdk is not necessary about. I'm not sure that "attribute" can notify application. Please, reword the description. > application event of RTE_ETH_EVENT_RXQ_LIMIT_REACHED when the Rx > queue's usable descriptor is under the watermark. > To simplify its configuration, LWM is a percentage of Rx queue > descriptor size with valid value of [0,99]. > Setting LWM to 0 means disable it. ... which is the default. Can I request notification when no descriptors left? 1 seems to be close to the answer, but not in the case of big Rx rings. > Add LWM's configuration handle in eth_dev_ops. handle sounds bad here. May be "driver callback" or "driver method". > > Signed-off-by: Spike Du > --- > lib/ethdev/ethdev_driver.h | 7 +++++++ > lib/ethdev/rte_ethdev.c | 28 ++++++++++++++++++++++++++++ > lib/ethdev/rte_ethdev.h | 30 +++++++++++++++++++++++++++++- > lib/ethdev/version.map | 3 +++ > 4 files changed, 67 insertions(+), 1 deletion(-) > > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h > index 69d9dc2..1e9cdbf 100644 > --- a/lib/ethdev/ethdev_driver.h > +++ b/lib/ethdev/ethdev_driver.h > @@ -470,6 +470,10 @@ typedef int (*eth_rx_queue_setup_t)(struct rte_eth_dev *dev, > const struct rte_eth_rxconf *rx_conf, > struct rte_mempool *mb_pool); > > +typedef int (*eth_rx_queue_set_lwm_t)(struct rte_eth_dev *dev, > + uint16_t rx_queue_id, > + uint8_t lwm); > + Please, add full description including parameters and return values. > /** @internal Setup a transmit queue of an Ethernet device. */ > typedef int (*eth_tx_queue_setup_t)(struct rte_eth_dev *dev, > uint16_t tx_queue_id, > @@ -1283,6 +1287,9 @@ struct eth_dev_ops { > > /** Dump private info from device */ > eth_dev_priv_dump_t eth_dev_priv_dump; > + > + /** Set Rx queue limit watermark */ > + eth_rx_queue_set_lwm_t rx_queue_set_lwm; > }; > > /** > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c > index 29a3d80..1e4fc6a 100644 > --- a/lib/ethdev/rte_ethdev.c > +++ b/lib/ethdev/rte_ethdev.c > @@ -4414,6 +4414,34 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx, > queue_idx, tx_rate)); > } > > +int rte_eth_rx_queue_set_lwm(uint16_t port_id, uint16_t queue_idx, > + uint8_t lwm) > +{ > + struct rte_eth_dev *dev; > + struct rte_eth_dev_info dev_info; > + int ret; > + > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > + dev = &rte_eth_devices[port_id]; > + > + ret = rte_eth_dev_info_get(port_id, &dev_info); > + if (ret != 0) > + return ret; > + > + if (queue_idx > dev_info.max_rx_queues) { It should be >= > + RTE_ETHDEV_LOG(ERR, > + "Set queue rate limit:port %u: invalid queue ID=%u\n", > + port_id, queue_idx); > + return -EINVAL; > + } > + > + if (lwm > 99) > + return -EINVAL; > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_set_lwm, -ENOTSUP); > + return eth_err(port_id, (*dev->dev_ops->rx_queue_set_lwm)(dev, > + queue_idx, lwm)); > +} > + > RTE_INIT(eth_dev_init_fp_ops) > { > uint32_t i; > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h > index 04cff8e..f29e53b 100644 > --- a/lib/ethdev/rte_ethdev.h > +++ b/lib/ethdev/rte_ethdev.h > @@ -1249,8 +1249,12 @@ struct rte_eth_rxconf { > */ > union rte_eth_rxseg *rx_seg; > > - uint64_t reserved_64s[2]; /**< Reserved for future fields */ > + uint64_t reserved_64s; > + uint32_t reserved_32s; > + uint32_t lwm:8; > + uint32_t reserved_bits:24; I strong dislike bit fields for such purpose. It should be uint8_t field. Since we break ABI below anyway, we can break it here as well. > void *reserved_ptrs[2]; /**< Reserved for future fields */ > + No unrelated changes, please. > }; > > /** > @@ -3668,6 +3672,29 @@ int rte_eth_dev_set_vlan_ether_type(uint16_t port_id, > */ > int rte_eth_dev_set_vlan_pvid(uint16_t port_id, uint16_t pvid, int on); > > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice. > + * > + * Set Rx queue based limit watermark. > + * > + * @param port_id > + * The port identifier of the Ethernet device. > + * @param queue_idx > + * The index of the receive queue > + * @param lwm > + * The limit watermark percentage of Rx queue descriptor size. > + * The valid range is [0,99]. > + * Setting 0 means disable limit watermark. > + * > + * @return > + * - (0) if successful. > + * - negative if failed. Please, be precise with negative return values specification and its meaning. > + */ > +__rte_experimental > +int rte_eth_rx_queue_set_lwm(uint16_t port_id, uint16_t queue_idx, > + uint8_t lwm); > + > typedef void (*buffer_tx_error_fn)(struct rte_mbuf **unsent, uint16_t count, > void *userdata); > > @@ -3873,6 +3900,7 @@ enum rte_eth_event_type { > RTE_ETH_EVENT_DESTROY, /**< port is released */ > RTE_ETH_EVENT_IPSEC, /**< IPsec offload related event */ > RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */ > + RTE_ETH_EVENT_RXQ_LIMIT_REACHED,/**< RX queue limit reached */ RX -> Rx as I understand it is an ABI breakage. > RTE_ETH_EVENT_MAX /**< max value of this enum */ > }; > > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map > index 20391ab..8b85ad8 100644 > --- a/lib/ethdev/version.map > +++ b/lib/ethdev/version.map > @@ -279,6 +279,9 @@ EXPERIMENTAL { > rte_flow_async_action_handle_create; > rte_flow_async_action_handle_destroy; > rte_flow_async_action_handle_update; > + > + # added in 22.07 > + rte_eth_rx_queue_set_lwm; > }; > > INTERNAL {