From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH v2] ethdev: introduce lock-free txq
	capability flag
Date: Sat, 08 Jul 2017 18:08:57 +0200
Message-ID: <187460180.BTNh70yjrI@xps>
References: <20170421122223.24194-1-jerin.jacob@caviumnetworks.com>
 <20170706062120.3895-1-jerin.jacob@caviumnetworks.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org, ferruh.yigit@intel.com, john.mcnamara@intel.com
To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Return-path: <dev-bounces@dpdk.org>
Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com
 [66.111.4.26]) by dpdk.org (Postfix) with ESMTP id 986EA235
 for <dev@dpdk.org>; Sat,  8 Jul 2017 18:08:58 +0200 (CEST)
In-Reply-To: <20170706062120.3895-1-jerin.jacob@caviumnetworks.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi Jerin,

Thanks for the update.
I think we can add this new flag in 17.08.
I prefer waiting John's review, especially for doc wording,
before applying it. I consider it does not hurt to add it post-rc1.

See below for my first comment on the doc.

06/07/2017 08:21, Jerin Jacob:
> Introducing the DEV_TX_OFFLOAD_MT_LOCKFREE TX capability flag.
> if a PMD advertises DEV_TX_OFFLOAD_MT_LOCKFREE capable, multiple threads
> can invoke rte_eth_tx_burst() concurrently on the same tx queue without
> SW lock. This PMD feature will be useful in the following use cases and
> found in the OCTEON family of NPUs.
> 
> 1) Remove explicit spinlock in some applications where lcores
> to TX queues are not mapped 1:1.
> example: OVS has such instance
> https://github.com/openvswitch/ovs/blob/master/lib/netdev-dpdk.c#L299
> https://github.com/openvswitch/ovs/blob/master/lib/netdev-dpdk.c#L1859
> See the the usage of tx_lock spinlock.
> 
> 2) In the eventdev use case, Avoid dedicating a separate TX core for
> transmitting and thus enables more scaling as all workers can
> send the packets.
> 
> v2:
> - Changed the flag name to DEV_TX_OFFLOAD_MT_LOCKFREE(Thomas)
> - Updated the documentation in doc/guides/prog_guide/poll_mode_drv.rst
> and rte_eth_tx_burst() doxgen comments(Thomas)
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
[...]
> +If the PMD is ``DEV_TX_OFFLOAD_MT_LOCKFREE`` capable, multiple threads can invoke ``rte_eth_tx_burst()``
> +concurrently on the same tx queue without SW lock.This PMD feature found in some NICs and

A space is missing after the dot.
Note: my preference is to start next sentence on a new line (in RST source).

> +useful in the following use cases if PMD supports it. See `Hardware Offload`_ for details.

This sentence is confusing. I would remove "if PMD supports it".
After "following use cases", should we add a colon?
The relation with `Hardware Offload`_ is not obvious.

> +*  Remove explicit spinlock in some applications where lcores to TX queues are not mapped 1:1.

Can we reword "lcores to TX queues"?
I suggest "lcores are not mapped to Tx queues with 1:1 relation".

> +*  In the eventdev use case, Avoid dedicating a separate TX core for transmitting and thus

Uppercase in the middle of the sentence spotted.

> +   enables more scaling as all workers can send the packets.