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: Mon, 10 Oct 2016 16:08:17 +0200
Message-ID: <1995417.Vz0qMdTBBe@xps13>
References: <20160928111052.9968-1-tomaszx.kulasek@intel.com>
 <20160930090039.10164-1-tomaszx.kulasek@intel.com>
 <20160930090039.10164-2-tomaszx.kulasek@intel.com>
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-f42.google.com (mail-lf0-f42.google.com
 [209.85.215.42]) by dpdk.org (Postfix) with ESMTP id A5D8FFFA
 for <dev@dpdk.org>; Mon, 10 Oct 2016 16:08:20 +0200 (CEST)
Received: by mail-lf0-f42.google.com with SMTP id b75so126518429lfg.3
 for <dev@dpdk.org>; Mon, 10 Oct 2016 07:08:20 -0700 (PDT)
In-Reply-To: <20160930090039.10164-2-tomaszx.kulasek@intel.com>
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,

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?

[...]
> -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