Re: [dpdk-dev] [PATCH v2 1/3] app/testpmd: code refactory for macswap

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.