diff mbox

[dpdk-dev] net/i40e: consider QinQ when setting MTU

Message ID 1493344523-58766-1-git-send-email-wenzhuo.lu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

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

Commit Message

Wenzhuo Lu April 28, 2017, 1:55 a.m. UTC 
  When counting max packet length from MTU, count
VLAN tag length twice for QinQ packets.

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c    | 2 +-
 drivers/net/i40e/i40e_ethdev_vf.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit April 28, 2017, 5:55 a.m. UTC | #1 
On 4/28/2017 2:55 AM, Wenzhuo Lu wrote:
> When counting max packet length from MTU, count
> VLAN tag length twice for QinQ packets.
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c    | 2 +-
>  drivers/net/i40e/i40e_ethdev_vf.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 3fa25dc..74041ae 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -10593,7 +10593,7 @@ static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
>  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
>  	struct rte_eth_dev_data *dev_data = pf->dev_data;
>  	uint32_t frame_size = mtu + ETHER_HDR_LEN
> -			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2;

Hi Wenzhuo,

Shouldn't we check if QinQ enabled before taking size account?
Jingjing Wu April 28, 2017, 8:27 a.m. UTC | #2 
> -----Original Message-----

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit

> Sent: Friday, April 28, 2017 1:55 PM

> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH] net/i40e: consider QinQ when setting MTU

> 

> On 4/28/2017 2:55 AM, Wenzhuo Lu wrote:

> > When counting max packet length from MTU, count VLAN tag length twice

> > for QinQ packets.

> >

> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

> > ---

> >  drivers/net/i40e/i40e_ethdev.c    | 2 +-

> >  drivers/net/i40e/i40e_ethdev_vf.c | 2 +-

> >  2 files changed, 2 insertions(+), 2 deletions(-)

> >

> > diff --git a/drivers/net/i40e/i40e_ethdev.c

> > b/drivers/net/i40e/i40e_ethdev.c index 3fa25dc..74041ae 100644

> > --- a/drivers/net/i40e/i40e_ethdev.c

> > +++ b/drivers/net/i40e/i40e_ethdev.c

> > @@ -10593,7 +10593,7 @@ static void i40e_set_default_mac_addr(struct

> rte_eth_dev *dev,

> >  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-

> >dev_private);

> >  	struct rte_eth_dev_data *dev_data = pf->dev_data;

> >  	uint32_t frame_size = mtu + ETHER_HDR_LEN

> > -			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;

> > +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2;

> 

> Hi Wenzhuo,

> 

> Shouldn't we check if QinQ enabled before taking size account?


Hi, Ferruh

Checking QinQ enabled here makes no sense, because we don't know
If the packets is carry single vlan or double vlan.

Acked-by: Jingjing Wu <jingjing.wu@intel.com>
Wenzhuo Lu April 28, 2017, 8:30 a.m. UTC | #3 
Hi Ferruh,

> -----Original Message-----

> From: Wu, Jingjing

> Sent: Friday, April 28, 2017 4:27 PM

> To: Yigit, Ferruh; Lu, Wenzhuo; dev@dpdk.org

> Subject: RE: [dpdk-dev] [PATCH] net/i40e: consider QinQ when setting MTU

> 

> 

> 

> > -----Original Message-----

> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit

> > Sent: Friday, April 28, 2017 1:55 PM

> > To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org

> > Subject: Re: [dpdk-dev] [PATCH] net/i40e: consider QinQ when setting

> > MTU

> >

> > On 4/28/2017 2:55 AM, Wenzhuo Lu wrote:

> > > When counting max packet length from MTU, count VLAN tag length

> > > twice for QinQ packets.

> > >

> > > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

> > > ---

> > >  drivers/net/i40e/i40e_ethdev.c    | 2 +-

> > >  drivers/net/i40e/i40e_ethdev_vf.c | 2 +-

> > >  2 files changed, 2 insertions(+), 2 deletions(-)

> > >

> > > diff --git a/drivers/net/i40e/i40e_ethdev.c

> > > b/drivers/net/i40e/i40e_ethdev.c index 3fa25dc..74041ae 100644

> > > --- a/drivers/net/i40e/i40e_ethdev.c

> > > +++ b/drivers/net/i40e/i40e_ethdev.c

> > > @@ -10593,7 +10593,7 @@ static void i40e_set_default_mac_addr(struct

> > rte_eth_dev *dev,

> > >  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-

> > >dev_private);

> > >  	struct rte_eth_dev_data *dev_data = pf->dev_data;

> > >  	uint32_t frame_size = mtu + ETHER_HDR_LEN

> > > -			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;

> > > +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2;

> >

> > Hi Wenzhuo,

> >

> > Shouldn't we check if QinQ enabled before taking size account?

> 

> Hi, Ferruh

> 

> Checking QinQ enabled here makes no sense, because we don't know If the

> packets is carry single vlan or double vlan.

> 

> Acked-by: Jingjing Wu <jingjing.wu@intel.com>

BTW, the patch aligned the behavior of kernel driver and DPDK.
Ferruh Yigit April 28, 2017, 12:38 p.m. UTC | #4 
On 4/28/2017 9:27 AM, Wu, Jingjing wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>> Sent: Friday, April 28, 2017 1:55 PM
>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/i40e: consider QinQ when setting MTU
>>
>> On 4/28/2017 2:55 AM, Wenzhuo Lu wrote:
>>> When counting max packet length from MTU, count VLAN tag length twice
>>> for QinQ packets.
>>>
>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>> ---
>>>  drivers/net/i40e/i40e_ethdev.c    | 2 +-
>>>  drivers/net/i40e/i40e_ethdev_vf.c | 2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/i40e/i40e_ethdev.c
>>> b/drivers/net/i40e/i40e_ethdev.c index 3fa25dc..74041ae 100644
>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>> @@ -10593,7 +10593,7 @@ static void i40e_set_default_mac_addr(struct
>> rte_eth_dev *dev,
>>>  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-
>>> dev_private);
>>>  	struct rte_eth_dev_data *dev_data = pf->dev_data;
>>>  	uint32_t frame_size = mtu + ETHER_HDR_LEN
>>> -			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
>>> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2;
>>
>> Hi Wenzhuo,
>>
>> Shouldn't we check if QinQ enabled before taking size account?
> 
> Hi, Ferruh
> 
> Checking QinQ enabled here makes no sense, because we don't know
> If the packets is carry single vlan or double vlan.

What do you think commenting code to say "I40E_VLAN_TAG_SIZE * 2" is for
QinQ?

It is not obvious and in the feature somebody not thinking about QinQ
use case may miss this, and do something wrong, even can fix this J

> 
> Acked-by: Jingjing Wu <jingjing.wu@intel.com>
>
Wenzhuo Lu May 2, 2017, 1:25 a.m. UTC | #5 
Hi Ferruh,


> -----Original Message-----

> From: Yigit, Ferruh

> Sent: Friday, April 28, 2017 8:38 PM

> To: Wu, Jingjing; Lu, Wenzhuo; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH] net/i40e: consider QinQ when setting MTU

> 

> On 4/28/2017 9:27 AM, Wu, Jingjing wrote:

> >

> >

> >> -----Original Message-----

> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit

> >> Sent: Friday, April 28, 2017 1:55 PM

> >> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org

> >> Subject: Re: [dpdk-dev] [PATCH] net/i40e: consider QinQ when setting

> >> MTU

> >>

> >> On 4/28/2017 2:55 AM, Wenzhuo Lu wrote:

> >>> When counting max packet length from MTU, count VLAN tag length

> >>> twice for QinQ packets.

> >>>

> >>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

> >>> ---

> >>>  drivers/net/i40e/i40e_ethdev.c    | 2 +-

> >>>  drivers/net/i40e/i40e_ethdev_vf.c | 2 +-

> >>>  2 files changed, 2 insertions(+), 2 deletions(-)

> >>>

> >>> diff --git a/drivers/net/i40e/i40e_ethdev.c

> >>> b/drivers/net/i40e/i40e_ethdev.c index 3fa25dc..74041ae 100644

> >>> --- a/drivers/net/i40e/i40e_ethdev.c

> >>> +++ b/drivers/net/i40e/i40e_ethdev.c

> >>> @@ -10593,7 +10593,7 @@ static void

> i40e_set_default_mac_addr(struct

> >> rte_eth_dev *dev,

> >>>  	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data-

> >>> dev_private);

> >>>  	struct rte_eth_dev_data *dev_data = pf->dev_data;

> >>>  	uint32_t frame_size = mtu + ETHER_HDR_LEN

> >>> -			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;

> >>> +			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2;

> >>

> >> Hi Wenzhuo,

> >>

> >> Shouldn't we check if QinQ enabled before taking size account?

> >

> > Hi, Ferruh

> >

> > Checking QinQ enabled here makes no sense, because we don't know If

> > the packets is carry single vlan or double vlan.

> 

> What do you think commenting code to say "I40E_VLAN_TAG_SIZE * 2" is for

> QinQ?

> 

> It is not obvious and in the feature somebody not thinking about QinQ use

> case may miss this, and do something wrong, even can fix this J

Thanks for your suggestion. I'll send a V2 to add the comments.

> 

> >

> > Acked-by: Jingjing Wu <jingjing.wu@intel.com>

> >
diff mbox

Patch

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 3fa25dc..74041ae 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -10593,7 +10593,7 @@  static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
 	struct rte_eth_dev_data *dev_data = pf->dev_data;
 	uint32_t frame_size = mtu + ETHER_HDR_LEN
-			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
+			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2;
 	int ret = 0;
 
 	/* check if mtu is within the allowed range */
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index cb34023..ac81546 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -2688,7 +2688,7 @@  static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
 	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 	struct rte_eth_dev_data *dev_data = vf->dev_data;
 	uint32_t frame_size = mtu + ETHER_HDR_LEN
-			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE;
+			      + ETHER_CRC_LEN + I40E_VLAN_TAG_SIZE * 2;
 	int ret = 0;
 
 	/* check if mtu is within the allowed range */