From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f50.google.com (mail-wm0-f50.google.com [74.125.82.50]) by dpdk.org (Postfix) with ESMTP id 533502B9D for ; Thu, 2 Mar 2017 14:57:55 +0100 (CET) Received: by mail-wm0-f50.google.com with SMTP id v186so135826830wmd.0 for ; Thu, 02 Mar 2017 05:57:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=ynLwjQsAHg0iDZUBLIBE4N7+FZPf/g670onH+HG0K9U=; b=kPLVzuiEtYqvtvsp3SeIh269N1gkavPj7T/8i7m+LHgyuTMt4XIjs1ToQwq96z4ICG qz9y3vYEMG0bIj+43ivYb+K0eFKSr2y/VUs1C7H2cqr7ZlOXA99l3KIWYqS7ZHIXyxbn vWOjlgx0YNFdHr6JcVQlnHRIPPxHzLFmWosJura7LgF4eL6tpDPuoUN2+DYryU66Loct TE3CYB00IiLihgqfWNJC7q4iieAfdHOX82lNORdaEMVqa8bp4jowLSqORzOsVVWJkaWt GbeHbpdhOXuqG2GLD3mqgoEyoBVICgIA0aD2gKG+AcAc4cDpoY+UzjTedghH+qU3BKhx r97g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=ynLwjQsAHg0iDZUBLIBE4N7+FZPf/g670onH+HG0K9U=; b=irJPddJFyjHxGZlcrdRsy3uLpqHhu0kSl/FsHrKhbyfyvsYcIX06gPoNOo/AwJpNJr cowdYvsmh/LZKAUQuoykLappxAPPJjh8AYKrz1+8zX5m8ZfuWCV+/h17e2ALmyPL36eR aU3KkTHyfHderYUdFGvc5K0F6SnY8mRM43ol6bOZ+eCUJ1uNurYbK9HWeuNDX+sHcgri FAXsaGFaPT56XOlE4BWts1DNPfJ2b2yXlREVsm1DI6FTfnpGiF61/Il/y+xELGreG929 kGVi9bitVTbXKwvjh8F2IUjz4FGXhPiPe446ynCUIsUKNfKAqmFOg/Xf/NWmVkw5SJQ6 s1BA== X-Gm-Message-State: AMke39nAaEl4aCg+YWdT5NYAUJkw6pcOl4oL7V6B5aXpcpjLzvn8c1fcl95fWlvH+MG02sL+ X-Received: by 10.28.61.11 with SMTP id k11mr8670440wma.119.1488463074757; Thu, 02 Mar 2017 05:57:54 -0800 (PST) Received: from platinum (2a01cb0c03c651000226b0fffeed02fc.ipv6.abo.wanadoo.fr. [2a01:cb0c:3c6:5100:226:b0ff:feed:2fc]) by smtp.gmail.com with ESMTPSA id j184sm26833116wmd.31.2017.03.02.05.57.54 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 02 Mar 2017 05:57:54 -0800 (PST) Date: Thu, 2 Mar 2017 14:57:52 +0100 From: Olivier Matz To: Andrew Rybchenko Cc: , , , , , , , , , Message-ID: <20170302145752.38b2f820@platinum> In-Reply-To: <8966736f-3bcb-5096-5c2b-643c65751d5f@solarflare.com> References: <1479981261-19512-1-git-send-email-olivier.matz@6wind.com> <1488388752-1819-1-git-send-email-olivier.matz@6wind.com> <1488388752-1819-2-git-send-email-olivier.matz@6wind.com> <8966736f-3bcb-5096-5c2b-643c65751d5f@solarflare.com> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/6] ethdev: add descriptor status API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Mar 2017 13:57:55 -0000 Hi Andrew, Thank you for the review. Comments inline. On Wed, 1 Mar 2017 21:22:14 +0300, Andrew Rybchenko wrote: > On 03/01/2017 08:19 PM, Olivier Matz wrote: > > Introduce a new API to get the status of a descriptor. > > > > For Rx, it is almost similar to rx_descriptor_done API, except it > > differentiates "used" descriptors (which are hold by the driver and not > > returned to the hardware). > > > > For Tx, it is a new API. > > > > The descriptor_done() API, and probably the rx_queue_count() API could > > be replaced by this new API as soon as it is implemented on all PMDs. > > > > Signed-off-by: Olivier Matz > > --- > > lib/librte_ether/rte_ethdev.h | 86 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 86 insertions(+) > > > > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h > > index 97f3e2d..9ac9c61 100644 > > --- a/lib/librte_ether/rte_ethdev.h > > +++ b/lib/librte_ether/rte_ethdev.h > > @@ -1179,6 +1179,14 @@ typedef uint32_t (*eth_rx_queue_count_t)(struct rte_eth_dev *dev, > > typedef int (*eth_rx_descriptor_done_t)(void *rxq, uint16_t offset); > > /**< @internal Check DD bit of specific RX descriptor */ > > > > +typedef int (*eth_rx_descriptor_status_t)(struct rte_eth_dev *dev, > > + uint16_t rx_queue_id, uint16_t offset); > > +/**< @internal Check the status of a Rx descriptor */ > > + > > +typedef int (*eth_tx_descriptor_status_t)(struct rte_eth_dev *dev, > > + uint16_t tx_queue_id, uint16_t offset); > > +/**< @internal Check the status of a Tx descriptor */ > > + > > typedef int (*eth_fw_version_get_t)(struct rte_eth_dev *dev, > > char *fw_version, size_t fw_size); > > /**< @internal Get firmware information of an Ethernet device. */ > > @@ -1483,6 +1491,10 @@ struct eth_dev_ops { > > eth_queue_release_t rx_queue_release; /**< Release RX queue. */ > > eth_rx_queue_count_t rx_queue_count;/**< Get Rx queue count. */ > > eth_rx_descriptor_done_t rx_descriptor_done; /**< Check rxd DD bit. */ > > + eth_rx_descriptor_status_t rx_descriptor_status; > > + /**< Check the status of a Rx descriptor. */ > > + eth_tx_descriptor_status_t tx_descriptor_status; > > + /**< Check the status of a Tx descriptor. */ > > eth_rx_enable_intr_t rx_queue_intr_enable; /**< Enable Rx queue interrupt. */ > > eth_rx_disable_intr_t rx_queue_intr_disable; /**< Disable Rx queue interrupt. */ > > eth_tx_queue_setup_t tx_queue_setup;/**< Set up device TX queue. */ > > @@ -2768,6 +2780,80 @@ rte_eth_rx_descriptor_done(uint8_t port_id, uint16_t queue_id, uint16_t offset) > > dev->data->rx_queues[queue_id], offset); > > } > > > > +#define RTE_ETH_RX_DESC_AVAIL 0 /**< Desc available for hw. */ > > +#define RTE_ETH_RX_DESC_DONE 1 /**< Desc done, filled by hw. */ > > +#define RTE_ETH_RX_DESC_USED 2 /**< Desc used by driver. */ > > + > > +/** > > + * Check the status of a Rx descriptor in the queue > > I think it would be useful to highlight caller context. > Should it be the same CPU which receives packets from the queue? Yes, you are right it would be useful. I suggest the following sentences: This function should be called on a dataplane core like the Rx function. They should not be called concurrently on the same queue. > > > + * > > + * @param port_id > > + * The port identifier of the Ethernet device. > > + * @param queue_id > > + * The Rx queue identifier on this port. > > + * @param offset > > + * The offset of the descriptor starting from tail (0 is the next > > + * packet to be received by the driver). > > + * @return > > + * - (RTE_ETH_DESC_AVAIL): Descriptor is available for the hardware to > > + * receive a packet. > > + * - (RTE_ETH_DESC_DONE): Descriptor is done, it is filled by hw, but > > + * not yet processed by the driver (i.e. in the receive queue). > > + * - (RTE_ETH_DESC_USED): Descriptor is unavailable (hold by driver, > > + * not yet returned to hw). > > It looks like it is the most suitable for descriptors which are reserved > and never used. Can you give some more details about what is a reserved but never used descriptor? (same question for Tx) > > > + * - (-ENODEV) if *port_id* invalid. > > + * - (-EINVAL) bad descriptor offset. > > + * - (-ENOTSUP) if the device does not support this function. > > What should be returned if queue_id is invalid? I'd say -ENODEV too. On the other hand, adding these checks is maybe not a good idea as we are in dataplane. The previous API rx_descriptor_done() API was taking the queue pointer as parameter, like Rx/Tx functions. It's probably a better idea. > What should be returned if the queue is stopped? For the same performance reasons, I think we should just highlight in the API that this dataplane function should not be called on a stopped queue. > > > + */ > > +static inline int > > +rte_eth_rx_descriptor_status(uint8_t port_id, uint16_t queue_id, > > + uint16_t offset) > > +{ > > + struct rte_eth_dev *dev; > > + > > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > > + dev = &rte_eth_devices[port_id]; > > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_descriptor_status, -ENOTSUP); > > + > > May be it makes sense to range check queue_id here to avoid such code in > each PMD? If we keep this API, yes. If we switch to a queue pointer as proposed above, we will assume (and highlight in the API doc) that the pointer must be valid, like for Rx/Tx funcs. Olivier