> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, December 11, 2018 1:44 AM
> To: Zhang, Qi Z <qi.z.zh...@intel.com>; Richardson, Bruce
> <bruce.richard...@intel.com>; Wiles, Keith <keith.wi...@intel.com>; Ananyev,
> Konstantin <konstantin.anan...@intel.com>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo...@intel.com>; Iremonger, Bernard
> <bernard.iremon...@intel.com>; Yongseok Koh <ys...@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.zh...@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
