diff mbox

[dpdk-dev] net/i40e: fix vlan insert code redundance

Message ID 1486689994-37746-1-git-send-email-qiming.yang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Qiming Yang Feb. 10, 2017, 1:26 a.m. UTC 
  This patch removed useless tx_flags in vlan insertion.

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

Comments

Ferruh Yigit Feb. 10, 2017, 10:25 a.m. UTC | #1 
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.

This code not fixes a defect, but improves the code, is there any
performance gain with this?
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))
>
Qiming Yang Feb. 10, 2017, 11:21 a.m. UTC | #2 
> -----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))
> >
Qiming Yang Feb. 13, 2017, 10:09 a.m. UTC | #3 
Hi, Ferruh

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yang, Qiming
> Sent: Friday, February 10, 2017 7:21 PM
> To: Yigit, Ferruh <ferruh.yigit@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.

I have aligned with Helin (he contribute these code), and make sure these code is useless, these code is left by history.

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

According to my test, this code will not bring any obvious performance gain, and no lose also.
And as you said this is not a bug fix, should I remove the fix line and modify the commit log?

> 
> > 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>
> > > ---
Ferruh Yigit Feb. 13, 2017, 10:14 a.m. UTC | #4 
On 2/13/2017 10:09 AM, Yang, Qiming wrote:
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yang, Qiming
>> Sent: Friday, February 10, 2017 7:21 PM
>> To: Yigit, Ferruh <ferruh.yigit@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.
> 
> I have aligned with Helin (he contribute these code), and make sure these code is useless, these code is left by history.
> 
>>
>>>
>>> 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.
> 
> According to my test, this code will not bring any obvious performance gain, and no lose also.
> And as you said this is not a bug fix, should I remove the fix line and modify the commit log?

Please do if you will send a new version already, no need to send a new
version just for title update.

Thanks,
ferruh

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

Patch

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