From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [PATCH v11 1/6] ethdev: add Tx preparation
Date: Thu, 27 Oct 2016 17:01:22 +0200
Message-ID: <1499338.8Le0ABsxDG@xps13>
References: <1477327917-18564-1-git-send-email-tomaszx.kulasek@intel.com>
 <1477486575-25148-1-git-send-email-tomaszx.kulasek@intel.com>
 <1477486575-25148-2-git-send-email-tomaszx.kulasek@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org
To: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wm0-f52.google.com (mail-wm0-f52.google.com [74.125.82.52])
 by dpdk.org (Postfix) with ESMTP id D31E63238
 for <dev@dpdk.org>; Thu, 27 Oct 2016 17:01:24 +0200 (CEST)
Received: by mail-wm0-f52.google.com with SMTP id n67so49576952wme.1
 for <dev@dpdk.org>; Thu, 27 Oct 2016 08:01:24 -0700 (PDT)
In-Reply-To: <1477486575-25148-2-git-send-email-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 Tomasz,

This is a major new function in the API and I still have some comments.

2016-10-26 14:56, Tomasz Kulasek:
> --- a/config/common_base
> +++ b/config/common_base
> +CONFIG_RTE_ETHDEV_TX_PREP=y

We cannot enable it until it is implemented in every drivers.

>  struct rte_eth_dev {
>  	eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
>  	eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
> +	eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit prepare function. */
>  	struct rte_eth_dev_data *data;  /**< Pointer to device data */
>  	const struct eth_driver *driver;/**< Driver for this device */
>  	const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */

Could you confirm why tx_pkt_prep is not in dev_ops?
I guess we want to have several implementations?

Shouldn't we have a const struct control_dev_ops and a struct datapath_dev_ops?

> +rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id, struct rte_mbuf **tx_pkts,
> +               uint16_t nb_pkts)

The word "prep" can be understood as "prepend".
Why not rte_eth_tx_prepare?

> +/**
> + * Fix pseudo header checksum
> + *
> + * This function fixes pseudo header checksum for TSO and non-TSO tcp/udp in
> + * provided mbufs packet data.
> + *
> + * - for non-TSO tcp/udp packets full pseudo-header checksum is counted and set
> + *   in packet data,
> + * - for TSO the IP payload length is not included in pseudo header.
> + *
> + * This function expects that used headers are in the first data segment of
> + * mbuf, are not fragmented and can be safely modified.

What happens otherwise?

> + *
> + * @param m
> + *   The packet mbuf to be fixed.
> + * @return
> + *   0 if checksum is initialized properly
> + */
> +static inline int
> +rte_phdr_cksum_fix(struct rte_mbuf *m)

Could we find a better name for this function?
- About the prefix, rte_ip_ ?
- About the scope, where this phdr_cksum is specified?
Isn't it an intel_phdr_cksum to match what hardware expects?
- About the verb, is it really fixing something broken?
Or just writing into a mbuf?
I would suggest rte_ip_intel_cksum_prepare.