From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f176.google.com (mail-wj0-f176.google.com [209.85.210.176]) by dpdk.org (Postfix) with ESMTP id CABE42C60 for ; Tue, 20 Dec 2016 13:58:31 +0100 (CET) Received: by mail-wj0-f176.google.com with SMTP id v7so175924582wjy.2 for ; Tue, 20 Dec 2016 04:58:31 -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:references:mime-version :content-disposition:in-reply-to; bh=De4TVfEjpC3BvPBfZoHU8Kz0Fhy4elLEnsxecvrtOqU=; b=C1qGfCBsSppO7HxlEaamzmR5tkc4vxcnIM4MO0Ggqjh/Fs87SHa6ZoaX/HziM+68gr UrgqxD1Bf5MnM9CGvhAYY6EiUoeFuG+y+4faNaGuY3XA3wQTt9zCe5m1/OXCPz3DMUlP ydRKqGNybaaZZquApoMOOJtXGxhwgmQTvBMAPMf0jHZAchiJH7ty7HUCubB/p//z5KS1 de156wu23B+Dl3H8tD4YukCVYmlQPRAvKJohSTZ1CvYv7wjNKv+1QsJawzxeaFyzTbTf 5Bj1MBOOVXCP17TynLs5aYy+6S0AiYa61nLbuHuXP8ErJByxDQO+2Jx9C48L+asnKrIn JjCQ== 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:references :mime-version:content-disposition:in-reply-to; bh=De4TVfEjpC3BvPBfZoHU8Kz0Fhy4elLEnsxecvrtOqU=; b=FbJ00FMX5kkk7L/hIHT62erZCmwx39kDVTynq4XC8YpsBSHnPFpg3e5VLYhMijIbFU 6gYiAWwcbnJCDyRgQ7faks/GA4KUjWVw01c1HRq93K98DkN3YHDGxLUovJQH/5fJjhgP p7K4qwBvlwSC6zxtWhyiPJm6zkQ6+QKColQdGul05HfeUujIK0noTxXQljHtF9llLZ1F sNtHpwxbSKju9zuOzZ5mFChSQVDRQlXVFJCauai4c8TtiwW0qBv7ULKkvnW8oEa/o1JK cnq6b5PUUvuLSMT+P9Ak1pFVXHSn09EC6VNnJGDFEbge9j+ridaBT/QV6kpPQhVl+6dY UcpQ== X-Gm-Message-State: AKaTC02HUkKFSDIUjgWv4Fgs0qbnQ/6GjS8NZhiPeX+h6C0nVpwFBYDvrhkHpwih0kuEWteq X-Received: by 10.194.123.170 with SMTP id mb10mr20738383wjb.158.1482238711486; Tue, 20 Dec 2016 04:58:31 -0800 (PST) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id u18sm22031273wmd.1.2016.12.20.04.58.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 20 Dec 2016 04:58:30 -0800 (PST) Date: Tue, 20 Dec 2016 13:58:23 +0100 From: Adrien Mazarguil To: "Ananyev, Konstantin" Cc: Billy McFall , "thomas.monjalon@6wind.com" , "Lu, Wenzhuo" , "dev@dpdk.org" , Stephen Hemminger Message-ID: <20161220125823.GU10340@6wind.com> References: <20161216124851.2640-1-bmcfall@redhat.com> <20161216124851.2640-2-bmcfall@redhat.com> <20161220112746.GT10340@6wind.com> <2601191342CEEE43887BDE71AB9772583F0F2D8A@irsmsx105.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0F2D8A@irsmsx105.ger.corp.intel.com> Subject: Re: [dpdk-dev] [PATCH 1/3] ethdev: New API to free consumed buffers in TX ring 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: Tue, 20 Dec 2016 12:58:32 -0000 On Tue, Dec 20, 2016 at 12:17:10PM +0000, Ananyev, Konstantin wrote: > > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil > > Sent: Tuesday, December 20, 2016 11:28 AM > > To: Billy McFall > > Cc: thomas.monjalon@6wind.com; Lu, Wenzhuo ; dev@dpdk.org; Stephen Hemminger > > > > Subject: Re: [dpdk-dev] [PATCH 1/3] ethdev: New API to free consumed buffers in TX ring > > > > Hi Billy, > > > > On Fri, Dec 16, 2016 at 07:48:49AM -0500, Billy McFall wrote: > > > Add a new API to force free consumed buffers on TX ring. API will return > > > the number of packets freed (0-n) or error code if feature not supported > > > (-ENOTSUP) or input invalid (-ENODEV). > > > > > > Because rte_eth_tx_buffer() may be used, and mbufs may still be held > > > in local buffer, the API also accepts *buffer and *sent. Before > > > attempting to free, rte_eth_tx_buffer_flush() is called to make sure > > > all mbufs are sent to Tx ring. rte_eth_tx_buffer_flush() is called even > > > if threshold is not met. > > > > > > Signed-off-by: Billy McFall > > > --- > > > lib/librte_ether/rte_ethdev.h | 56 +++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 56 insertions(+) > > > > > > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h > > > index 9678179..e3f2be4 100644 > > > --- a/lib/librte_ether/rte_ethdev.h > > > +++ b/lib/librte_ether/rte_ethdev.h > > > @@ -1150,6 +1150,9 @@ 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_tx_done_cleanup_t)(void *txq, uint32_t free_cnt); > > > +/**< @internal Force mbufs to be from TX ring. */ > > > + > > > typedef void (*eth_rxq_info_get_t)(struct rte_eth_dev *dev, > > > uint16_t rx_queue_id, struct rte_eth_rxq_info *qinfo); > > > > > > @@ -1467,6 +1470,7 @@ struct eth_dev_ops { > > > eth_rx_disable_intr_t rx_queue_intr_disable; > > > eth_tx_queue_setup_t tx_queue_setup;/**< Set up device TX queue.*/ > > > eth_queue_release_t tx_queue_release;/**< Release TX queue.*/ > > > + eth_tx_done_cleanup_t tx_done_cleanup;/**< Free tx ring mbufs */ > > > eth_dev_led_on_t dev_led_on; /**< Turn on LED. */ > > > eth_dev_led_off_t dev_led_off; /**< Turn off LED. */ > > > flow_ctrl_get_t flow_ctrl_get; /**< Get flow control. */ > > > @@ -2943,6 +2947,58 @@ rte_eth_tx_buffer(uint8_t port_id, uint16_t queue_id, > > > } > > > > > > /** > > > + * Request the driver to free mbufs currently cached by the driver. The > > > + * driver will only free the mbuf if it is no longer in use. > > > + * > > > + * @param port_id > > > + * The port identifier of the Ethernet device. > > > + * @param queue_id > > > + * The index of the transmit queue through which output packets must be > > > + * sent. > > > + * The value must be in the range [0, nb_tx_queue - 1] previously supplied > > > + * to rte_eth_dev_configure(). > > > + * @param free_cnt > > > + * Maximum number of packets to free. Use 0 to indicate all possible packets > > > + * should be freed. Note that a packet may be using multiple mbufs. > > > + * @param buffer > > > + * Buffer used to collect packets to be sent. If provided, the buffer will > > > + * be flushed, even if the current length is less than buffer->size. Pass NULL > > > + * if buffer has already been flushed. > > > + * @param sent > > > + * Pointer to return number of packets sent if buffer has packets to be sent. > > > + * If *buffer is supplied, *sent must also be supplied. > > > + * @return > > > + * Failure: < 0 > > > + * -ENODEV: Invalid interface > > > + * -ENOTSUP: Driver does not support function > > > + * Success: >= 0 > > > + * 0-n: Number of packets freed. More packets may still remain in ring that > > > + * are in use. > > > + */ > > > + > > > +static inline int > > > +rte_eth_tx_done_cleanup(uint8_t port_id, uint16_t queue_id, uint32_t free_cnt, > > > + struct rte_eth_dev_tx_buffer *buffer, uint16_t *sent) > > > +{ > > > + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > > > + > > > + /* Validate Input Data. Bail if not valid or not supported. */ > > > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > > > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP); > > > + > > > + /* > > > + * If transmit buffer is provided and there are still packets to be > > > + * sent, then send them before attempting to free pending mbufs. > > > + */ > > > + if (buffer && sent) > > > + *sent = rte_eth_tx_buffer_flush(port_id, queue_id, buffer); > > > + > > > + /* Call driver to free pending mbufs. */ > > > + return (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id], > > > + free_cnt); > > > +} > > > + > > > +/** > > > * Configure a callback for buffered packets which cannot be sent > > > * > > > * Register a specific callback to be called when an attempt is made to send > > > > Just a thought to follow-up on Stephen's comment to further simplify this > > API, how about not adding any new eth_dev_ops but instead defining what > > should happen during an empty TX burst call (tx_burst() with 0 packets). > > > > Several PMDs already have a check for this scenario and start by cleaning up > > completed packets anyway, they effectively partially implement this > > definition for free already. > > Many PMDs start by cleaning up only when number of free entries > drop below some point. > Also in that case the author would have to modify (and test) all existing TX routinies. > So I think a separate API call seems more plausible. Not necessarily, as I understand this API in its current form only suggests that a PMD should release a few mbufs from a queue if possible, without any guarantee, PMDs are not forced to comply. I think the threshold you mention is a valid reason not to release them, and it wouldn't change a thing to existing tx_burst() implementations in the meantime (only documentation). This threshold could also be bypassed rather painlessly in the "if (unlikely(nb_pkts == 0))" case that all PMDs already check for in a way or another. > Though I am agree with previous comment from Stephen that last two parameters > are redundant and would just overcomplicate things. > tin > > > > > The main difference with this API would be that you wouldn't know how many > > mbufs were freed and wouldn't collect them into an array. However most > > applications have one mbuf pool and/or know where they come from, so they > > can just query the pool or attempt to re-allocate from it after doing empty > > bursts in case of starvation. > > > > [1] http://dpdk.org/ml/archives/dev/2016-December/052469.html -- Adrien Mazarguil 6WIND