From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nbfkord-smmo02.seg.att.com (nbfkord-smmo02.seg.att.com [209.65.160.78]) by dpdk.org (Postfix) with ESMTP id 303342C60 for ; Thu, 2 Mar 2017 15:19:36 +0100 (CET) Received: from unknown [193.34.186.16] (EHLO webmail.solarflare.com) by nbfkord-smmo02.seg.att.com(mxl_mta-7.2.4-7) with ESMTP id 8f928b85.2ae031254940.1014406.00-2490.2801528.nbfkord-smmo02.seg.att.com (envelope-from ); Thu, 02 Mar 2017 14:19:36 +0000 (UTC) X-MXL-Hash: 58b829f816d505dc-6116ae43db2b7993edb510371e969a9b2d7190fe Received: from unknown [193.34.186.16] (EHLO webmail.solarflare.com) by nbfkord-smmo02.seg.att.com(mxl_mta-7.2.4-7) over TLS secured channel with ESMTP id 3f928b85.0.1014401.00-2371.2801512.nbfkord-smmo02.seg.att.com (envelope-from ); Thu, 02 Mar 2017 14:19:33 +0000 (UTC) X-MXL-Hash: 58b829f539ce0106-b8237f0749f807088860f9075531a54af2dc74eb Received: from [192.168.38.17] (84.52.89.52) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Thu, 2 Mar 2017 14:19:24 +0000 To: Olivier Matz 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> <20170302145752.38b2f820@platinum> CC: , , , , , , , , , From: Andrew Rybchenko Message-ID: Date: Thu, 2 Mar 2017 17:19:21 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <20170302145752.38b2f820@platinum> X-Originating-IP: [84.52.89.52] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-22916.003 X-TM-AS-Result: No--23.885100-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-AnalysisOut: [v=2.1 cv=T6yKOq+Q c=1 sm=1 tr=0 a=8P+NB+fYZDP74ap4g4d9Kw==] X-AnalysisOut: [:17 a=RB3BGLmKESwA:10 a=6Iz7jQTuP9IA:10 a=zRKbQ67AAAAA:8 a] X-AnalysisOut: [=4YC7WVefAAAA:8 a=nuce4vytRVnmNZy8OloA:9 a=pILNOxqGKmIA:10] X-AnalysisOut: [ a=YUdKfl359-gGfhLQnRcA:9 a=owSA6z-aLvLtrzW3:21 a=_W_S_7Ve] X-AnalysisOut: [coQA:10 a=PA03WX8tBzeizutn5_OT:22 a=tPzOKt3quolVTVSLigK1:2] X-AnalysisOut: [2] X-Spam: [F=0.2000000000; CM=0.500; S=0.200(2015072901)] X-MAIL-FROM: X-SOURCE-IP: [193.34.186.16] Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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 14:19:36 -0000 Hi Olivier, On 03/02/2017 04:57 PM, Olivier Matz wrote: > 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. The first sentence looks fine. "They" (functions?, dataplane cores?) is unclear for me in the second. May be the first one is simply sufficient. >>> + * >>> + * @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) Our HW has a requirement to keep few descriptors always unused (i.e. some gap between tail and head). It is just a few descriptors, but invalid descriptor status may misguide application. E.g. if Rx queue size is 512 and offset 510, it will always be unused (since it is reserved). It is not an indication that core is too slow and can't keep the pace. >>> + * - (-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. I think so too since Rx burst callback (used nearby as above descriptor says) gets queue pointer. >> 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. OK. >>> + */ >>> +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. I've simply seen patches which add the queue id range check in the generic place. But I think switching to queue pointer is a better idea. Thanks, Andrew.