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

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

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