Re: [PATCH] net/i40e: fix vlan insert code redundance

From: "Yang, Qiming" <qiming.yang@intel.com>
To: "Yigit, Ferruh" <ferruh.yigit@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "Wu, Jingjing" <jingjing.wu@intel.com>
Subject: Re: [PATCH] net/i40e: fix vlan insert code redundance
Date: Fri, 10 Feb 2017 11:21:14 +0000	[thread overview]
Message-ID: <F5DF4F0E3AFEF648ADC1C3C33AD4DBF16EDD22E0@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <0425f979-8198-4cc8-f70b-f1c9a037fd7f@intel.com>

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, February 10, 2017 6:25 PM
> To: Yang, Qiming <qiming.yang@intel.com>; dev@dpdk.org
> Cc: Wu, Jingjing <jingjing.wu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix vlan insert code redundance
> 
> On 2/10/2017 1:26 AM, Qiming Yang wrote:
> > This patch removed useless tx_flags in vlan insertion.
> 
> Overall this looks good, I wonder what was the initial intention of this code,
> understanding it helps to figure out if there is a hidden defect. 

Thank you for your remind. I'll investigate it.

> 
> This code not fixes a defect, but improves the code, is there any
> performance gain with this?

I'll do more test and give you a feedback. 

> If not, I am for deferring this to next release.
> 
> >
> > Fixes: 4861cde46116 ("i40e: new poll mode driver")
> >
> > Signed-off-by: Qiming Yang <qiming.yang@intel.com>
> > ---
> >  drivers/net/i40e/i40e_rxtx.c | 8 +-------
> > drivers/net/i40e/i40e_rxtx.h | 2 --
> >  2 files changed, 1 insertion(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > b/drivers/net/i40e/i40e_rxtx.c index 608685f..b91cd70 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -1026,7 +1026,6 @@ i40e_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts, uint16_t nb_pkts)
> >  	uint16_t nb_tx;
> >  	uint32_t td_cmd;
> >  	uint32_t td_offset;
> > -	uint32_t tx_flags;
> >  	uint32_t td_tag;
> >  	uint64_t ol_flags;
> >  	uint16_t nb_used;
> > @@ -1050,7 +1049,6 @@ i40e_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts, uint16_t nb_pkts)
> >  		td_cmd = 0;
> >  		td_tag = 0;
> >  		td_offset = 0;
> > -		tx_flags = 0;
> >
> >  		tx_pkt = *tx_pkts++;
> >  		RTE_MBUF_PREFETCH_TO_FREE(txe->mbuf);
> > @@ -1097,12 +1095,8 @@ i40e_xmit_pkts(void *tx_queue, struct
> rte_mbuf
> > **tx_pkts, uint16_t nb_pkts)
> >
> >  		/* Descriptor based VLAN insertion */
> >  		if (ol_flags & (PKT_TX_VLAN_PKT | PKT_TX_QINQ_PKT)) {
> > -			tx_flags |= tx_pkt->vlan_tci <<
> > -				I40E_TX_FLAG_L2TAG1_SHIFT;
> > -			tx_flags |= I40E_TX_FLAG_INSERT_VLAN;
> 
> The I40E_TX_FLAG_INSERT_VLAN flag also seems used only here, and can be
> removed.
> 
> Also I40E_TX_FLAG_CSUM and I40E_TX_FLAG_TSYN seems not used at all,
> understanding why they are introduced at first place can be useful.
> 
> >  			td_cmd |= I40E_TX_DESC_CMD_IL2TAG1;
> > -			td_tag = (tx_flags & I40E_TX_FLAG_L2TAG1_MASK) >>
> > -
> 	I40E_TX_FLAG_L2TAG1_SHIFT;
> > +			td_tag = tx_pkt->vlan_tci;
> >  		}
> >
> >  		/* Always enable CRC offload insertion */ diff --git
> > a/drivers/net/i40e/i40e_rxtx.h b/drivers/net/i40e/i40e_rxtx.h index
> > 9df8a56..3d4abdc 100644
> > --- a/drivers/net/i40e/i40e_rxtx.h
> > +++ b/drivers/net/i40e/i40e_rxtx.h
> > @@ -38,8 +38,6 @@
> >   * 32 bits tx flags, high 16 bits for L2TAG1 (VLAN),
> >   * low 16 bits for others.
> >   */
> > -#define I40E_TX_FLAG_L2TAG1_SHIFT 16
> > -#define I40E_TX_FLAG_L2TAG1_MASK  0xffff0000
> >  #define I40E_TX_FLAG_CSUM         ((uint32_t)(1 << 0))
> >  #define I40E_TX_FLAG_INSERT_VLAN  ((uint32_t)(1 << 1))
> >  #define I40E_TX_FLAG_TSYN         ((uint32_t)(1 << 2))
> >