[v2,1/3] app/testpmd: code refactory for macswap

  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>
---
 app/test-pmd/macswap.c        | 32 ++------------------------------
 app/test-pmd/macswap.h        | 40 ++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/macswap_common.h | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 30 deletions(-)
 create mode 100644 app/test-pmd/macswap.h
 create mode 100644 app/test-pmd/macswap_common.h
  

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.
  

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, December 11, 2018 1:44 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Wiles, Keith <keith.wiles@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Yongseok Koh <yskoh@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/3] app/testpmd: code refactory for
> macswap
> 
> 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?

Ok, I will replace.

> 
> > +	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.

I think above line also clean up other bits besides IND_ATTACHED_MBUF | EXT_ATTACHED_MBUF
But I don't know if it is necessary, so I just keep it the same way as before.

> 
> > +	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.

Good point, we can skip these memory write if PKT_TX_VLAN and PKT_TX_QINQ is not set.

Thanks
Qi