From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [PATCH v4 1/6] ethdev: add Tx preparation
Date: Thu, 13 Oct 2016 09:08:52 +0200
Message-ID: <1506813.gKiMgNqmq7@xps13>
References: <20160928111052.9968-1-tomaszx.kulasek@intel.com>
 <20160930090039.10164-2-tomaszx.kulasek@intel.com> <1995417.Vz0qMdTBBe@xps13>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org, konstantin.ananyev@intel.com
To: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-lf0-f41.google.com (mail-lf0-f41.google.com
 [209.85.215.41]) by dpdk.org (Postfix) with ESMTP id B9D9A2B9A
 for <dev@dpdk.org>; Thu, 13 Oct 2016 09:08:55 +0200 (CEST)
Received: by mail-lf0-f41.google.com with SMTP id x79so123774256lff.0
 for <dev@dpdk.org>; Thu, 13 Oct 2016 00:08:55 -0700 (PDT)
In-Reply-To: <1995417.Vz0qMdTBBe@xps13>
List-Id: patches and discussions about DPDK <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 Tomasz,

Any news?
Sorry to speed up, we are very very late for RC1.

2016-10-10 16:08, Thomas Monjalon:
> Hi,
> 
> Now that the feature seems to meet a consensus, I've looked at it more
> closely before integrating. Sorry if it appears like a late review.
> 
> 2016-09-30 11:00, Tomasz Kulasek:
> > Added API for `rte_eth_tx_prep`
> 
> I would love to read the usability and performance considerations here.
> No need for something as long as the cover letter. Just few lines
> about why it is needed and why it is a good choice for performance.
> 
> > uint16_t rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id,
> > 	struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
> > 
> > Added fields to the `struct rte_eth_desc_lim`:
> > 
> > 	uint16_t nb_seg_max;
> > 		/**< Max number of segments per whole packet. */
> > 
> > 	uint16_t nb_mtu_seg_max;
> > 		/**< Max number of segments per one MTU */
> [...]
> > +#else
> > +
> > +static inline uint16_t
> > +rte_eth_tx_prep(uint8_t port_id __rte_unused, uint16_t queue_id __rte_unused,
> > +		struct rte_mbuf **tx_pkts __rte_unused, uint16_t nb_pkts)
> 
> Doxygen is failing here.
> Have you tried to move __rte_unused before the identifier?
> 
> [...]
> > +#define PKT_TX_OFFLOAD_MASK (    \
> > +		PKT_TX_IP_CKSUM |        \
> > +		PKT_TX_L4_MASK |         \
> > +		PKT_TX_OUTER_IP_CKSUM |  \
> > +		PKT_TX_TCP_SEG |         \
> > +		PKT_TX_QINQ_PKT |        \
> > +		PKT_TX_VLAN_PKT)
> 
> We should really stop adding some public constants without the proper
> RTE prefix.
> And by the way, should not we move such flags into rte_net?

Do not worry with this comment. It was just a thought which could be
addressed in a separate patch by someone else.

> [...]
> > -SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include := rte_ip.h rte_tcp.h rte_udp.h rte_sctp.h rte_icmp.h rte_arp.h
> > +SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include := rte_ip.h rte_tcp.h rte_udp.h rte_sctp.h rte_icmp.h rte_arp.h rte_pkt.h
> 
> You can use the += operator on a new line for free :)
> 
> No more comments, the rest seems OK. Thanks