From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ferruh Yigit <ferruh.yigit@intel.com>
Subject: Re: [PATCH v2 1/3] app/testpmd: code refactory for
	macswap
Date: Mon, 10 Dec 2018 17:44:24 +0000
Message-ID: <c4d459cf-5bac-19b4-c1f2-cb849b6e1e4e@intel.com>
References: <20181122172632.6229-1-qi.z.zhang@intel.com>
 <20181122173805.79555-1-qi.z.zhang@intel.com>
 <20181122173805.79555-2-qi.z.zhang@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Cc: dev@dpdk.org, wenzhuo.lu@intel.com, bernard.iremonger@intel.com,
 Yongseok Koh <yskoh@mellanox.com>
To: Qi Zhang <qi.z.zhang@intel.com>, bruce.richardson@intel.com,
 keith.wiles@intel.com, konstantin.ananyev@intel.com
Return-path: <dev-bounces@dpdk.org>
Received: from mga07.intel.com (mga07.intel.com [134.134.136.100])
 by dpdk.org (Postfix) with ESMTP id 902C07D05
 for <dev@dpdk.org>; Mon, 10 Dec 2018 18:44:31 +0100 (CET)
In-Reply-To: <20181122173805.79555-2-qi.z.zhang@intel.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 11/22/2018 5:38 PM, Qi Zhang wrote:
> Move macswap workload to dedicate function, so we can further enable
> platform specific optimized version.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>

<...>

> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#ifndef _L2FWD_H_
> +#define _L2FWD_H_

Looks like copy-paste artifact, there are a few more in patchset.

<...>

> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#ifndef _L2FWD_COMMON_H_
> +#define _L2FWD_COMMON_H_
> +
> +static inline uint64_t
> +ol_flags_init(uint64_t tx_offload)
> +{
> +	uint64_t ol_flags = 0;
> +
> +	ol_flags |= (tx_offload & DEV_TX_OFFLOAD_VLAN_INSERT) ?
> +			PKT_TX_VLAN_PKT : 0;

'PKT_TX_VLAN_PKT' is depreciated and replaced with 'PKT_TX_VLAN'. I think it is
better to keep as it is in this patch, since mainly it copies from one place to
another, but can you update this in new patch in this patchset?

> +	ol_flags |= (tx_offload & DEV_TX_OFFLOAD_QINQ_INSERT) ?
> +			PKT_TX_QINQ_PKT : 0;

Same here, 'PKT_TX_QINQ_PKT' replaced with 'PKT_TX_QINQ'.

> +	ol_flags |= (tx_offload & DEV_TX_OFFLOAD_MACSEC_INSERT) ?
> +			PKT_TX_MACSEC : 0;
> +
> +	return ol_flags;
> +}
> +
> +static inline void
> +mbuf_field_set(struct rte_mbuf *mb, uint64_t ol_flags,
> +		uint16_t vlan, uint16_t vlan_outer)
> +{
> +	mb->ol_flags &= IND_ATTACHED_MBUF | EXT_ATTACHED_MBUF;

I guess above line is to prevent those bits overwritten, but with '|='
assignment below I think they will be preserved already, do we need above line?
cc'ed Yongseok.

> +	mb->ol_flags |= ol_flags;
> +	mb->l2_len = sizeof(struct ether_hdr);
> +	mb->l3_len = sizeof(struct ipv4_hdr);
> +	mb->vlan_tci = vlan;
> +	mb->vlan_tci_outer = vlan_outer;

Setting 'vlan_tci' or 'vlan_tci_outer' makes sense only if 'PKT_TX_VLAN' and
'PKT_TX_QINQ' set, since there is already an check for them above, does it make
sense to do these assignment in them, for better performance.