diff mbox series

[v2,6/7] net/e1000: set min and max MTU for igb devices

Message ID	1553259678-4515-7-git-send-email-ian.stokes@intel.com (mailing list archive)
State	Changes Requested, archived
Delegated to:	Ferruh Yigit
Headers	From: Ian Stokes <ian.stokes@intel.com> To: dev@dpdk.org Cc: stephen@networkplumber.org, Ian Stokes <ian.stokes@intel.com> Date: Fri, 22 Mar 2019 13:01:17 +0000 Message-Id: <1553259678-4515-7-git-send-email-ian.stokes@intel.com> In-Reply-To: <1553259678-4515-1-git-send-email-ian.stokes@intel.com> References: <1553259678-4515-1-git-send-email-ian.stokes@intel.com> Subject: [dpdk-dev] [PATCH v2 6/7] net/e1000: set min and max MTU for igb devices Precedence: list Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org>
Series	ethdev: add min/max MTU to device info \| [v2,0/7] ethdev: add min/max MTU to device info [v2,1/7] ethdev: add min/max MTU to device info [v2,2/7] net/i40e: set min and max MTU for i40e devices [v2,3/7] net/i40e: set min and max MTU for i40e VF devices [v2,4/7] net/ixgbe: set min and max MTU for ixgbe devices [v2,5/7] net/ixgbe: set min and max MTU for ixgbe VF devices [v2,6/7] net/e1000: set min and max MTU for igb devices [v2,7/7] app/testpmd: verify mtu with rte_eth_dev_info_get()

Checks

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

Commit Message

Ian Stokes March 22, 2019, 1:01 p.m. UTC

  This commit sets the min and max supported MTU values for igb devices
via the eth_igb_info_get() function. Min MTU supported is set to
ETHER_MIN_MTU and max mtu is calculated as the max packet length
supported minus the transport overhead. To aid in these calculations
a new MACRO 'E1000_ETH_OVERHEAD' has been introduced to consolidate
overhead calculation and avoid duplication.

Signed-off-by: Ian Stokes <ian.stokes@intel.com>
---
 drivers/net/e1000/e1000_ethdev.h | 6 ++++++
 drivers/net/e1000/igb_ethdev.c   | 7 +++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit March 25, 2019, 2:25 p.m. UTC | #1

On 3/22/2019 1:01 PM, Ian Stokes wrote:
> This commit sets the min and max supported MTU values for igb devices
> via the eth_igb_info_get() function. Min MTU supported is set to
> ETHER_MIN_MTU and max mtu is calculated as the max packet length
> supported minus the transport overhead. To aid in these calculations
> a new MACRO 'E1000_ETH_OVERHEAD' has been introduced to consolidate
> overhead calculation and avoid duplication.
> 
> Signed-off-by: Ian Stokes <ian.stokes@intel.com>
> ---
>  drivers/net/e1000/e1000_ethdev.h | 6 ++++++
>  drivers/net/e1000/igb_ethdev.c   | 7 +++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
> index 94edff08e..3e74cd8fe 100644
> --- a/drivers/net/e1000/e1000_ethdev.h
> +++ b/drivers/net/e1000/e1000_ethdev.h
> @@ -89,6 +89,12 @@
>  	ETH_RSS_IPV6_UDP_EX)
>  
>  /*
> + * The overhead from MTU to max frame size.
> + * Considering VLAN so a tag needs to be counted.
> + */
> +#define E1000_ETH_OVERHEAD (ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TAG_SIZE)

As an overhead, following drivers set:
i40e: HDR + CRC + 2 * VLAN
ixgbe: HDR + CRC
e1000: HDR + CRC + VLAN

I wonder if this difference is HW limitation, or driver limitation or just
implementation inconsistency.

Better to confirm it that it is not implementation inconsistency.

Wenzhuo, Konstantin, Beilei, Qi,

Can you please comment?

Thanks,
ferruh

Qi Zhang March 26, 2019, 1:58 p.m. UTC | #2

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, March 25, 2019 10:25 PM
> To: Stokes, Ian <ian.stokes@intel.com>; dev@dpdk.org
> Cc: stephen@networkplumber.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 6/7] net/e1000: set min and max MTU for igb
> devices
> 
> On 3/22/2019 1:01 PM, Ian Stokes wrote:
> > This commit sets the min and max supported MTU values for igb devices
> > via the eth_igb_info_get() function. Min MTU supported is set to
> > ETHER_MIN_MTU and max mtu is calculated as the max packet length
> > supported minus the transport overhead. To aid in these calculations a
> > new MACRO 'E1000_ETH_OVERHEAD' has been introduced to consolidate
> > overhead calculation and avoid duplication.
> >
> > Signed-off-by: Ian Stokes <ian.stokes@intel.com>
> > ---
> >  drivers/net/e1000/e1000_ethdev.h | 6 ++++++
> >  drivers/net/e1000/igb_ethdev.c   | 7 +++++--
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/e1000/e1000_ethdev.h
> > b/drivers/net/e1000/e1000_ethdev.h
> > index 94edff08e..3e74cd8fe 100644
> > --- a/drivers/net/e1000/e1000_ethdev.h
> > +++ b/drivers/net/e1000/e1000_ethdev.h
> > @@ -89,6 +89,12 @@
> >  	ETH_RSS_IPV6_UDP_EX)
> >
> >  /*
> > + * The overhead from MTU to max frame size.
> > + * Considering VLAN so a tag needs to be counted.
> > + */
> > +#define E1000_ETH_OVERHEAD (ETHER_HDR_LEN + ETHER_CRC_LEN +
> > +VLAN_TAG_SIZE)
> 
> As an overhead, following drivers set:
> i40e: HDR + CRC + 2 * VLAN
> ixgbe: HDR + CRC
> e1000: HDR + CRC + VLAN
> 
> I wonder if this difference is HW limitation, or driver limitation or just
> implementation inconsistency.

I think this is implementation inconsistency 

The NIC only accept Max Frame Size.

The problem here is seems all of three setup are not perfect.

HDR + CRC + 2 * VLAN - it may allow non vlan or single vlan packet that exceed mtu. 
HDR + CRC - it may reject vlan or double vlan packet that follow mtu.  
HDR + CRC + VLAN ,  it may reject double vlan packet that follow mtu

I agree it's better to keep consistent on all drivers, but before this, we may need to decide which one we should take :)

Regards
Qi

> 
> Better to confirm it that it is not implementation inconsistency.
> 
> Wenzhuo, Konstantin, Beilei, Qi,
> 
> Can you please comment?
> 
> Thanks,
> ferruh

Ananyev, Konstantin March 26, 2019, 2:02 p.m. UTC | #3

Hi Qi,

> >
> > On 3/22/2019 1:01 PM, Ian Stokes wrote:
> > > This commit sets the min and max supported MTU values for igb devices
> > > via the eth_igb_info_get() function. Min MTU supported is set to
> > > ETHER_MIN_MTU and max mtu is calculated as the max packet length
> > > supported minus the transport overhead. To aid in these calculations a
> > > new MACRO 'E1000_ETH_OVERHEAD' has been introduced to consolidate
> > > overhead calculation and avoid duplication.
> > >
> > > Signed-off-by: Ian Stokes <ian.stokes@intel.com>
> > > ---
> > >  drivers/net/e1000/e1000_ethdev.h | 6 ++++++
> > >  drivers/net/e1000/igb_ethdev.c   | 7 +++++--
> > >  2 files changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/e1000/e1000_ethdev.h
> > > b/drivers/net/e1000/e1000_ethdev.h
> > > index 94edff08e..3e74cd8fe 100644
> > > --- a/drivers/net/e1000/e1000_ethdev.h
> > > +++ b/drivers/net/e1000/e1000_ethdev.h
> > > @@ -89,6 +89,12 @@
> > >  	ETH_RSS_IPV6_UDP_EX)
> > >
> > >  /*
> > > + * The overhead from MTU to max frame size.
> > > + * Considering VLAN so a tag needs to be counted.
> > > + */
> > > +#define E1000_ETH_OVERHEAD (ETHER_HDR_LEN + ETHER_CRC_LEN +
> > > +VLAN_TAG_SIZE)
> >
> > As an overhead, following drivers set:
> > i40e: HDR + CRC + 2 * VLAN
> > ixgbe: HDR + CRC
> > e1000: HDR + CRC + VLAN
> >
> > I wonder if this difference is HW limitation, or driver limitation or just
> > implementation inconsistency.
> 
> I think this is implementation inconsistency
> 
> The NIC only accept Max Frame Size.
> 
> The problem here is seems all of three setup are not perfect.
> 
> HDR + CRC + 2 * VLAN - it may allow non vlan or single vlan packet that exceed mtu.

Hmm, wonder how?

> HDR + CRC - it may reject vlan or double vlan packet that follow mtu.
> HDR + CRC + VLAN ,  it may reject double vlan packet that follow mtu
> 
> I agree it's better to keep consistent on all drivers, but before this, we may need to decide which one we should take :)
> 
> Regards
> Qi
> 
> >
> > Better to confirm it that it is not implementation inconsistency.
> >
> > Wenzhuo, Konstantin, Beilei, Qi,
> >
> > Can you please comment?
> >
> > Thanks,
> > ferruh

Qi Zhang March 26, 2019, 2:09 p.m. UTC | #4

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, March 26, 2019 10:02 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Stokes, Ian <ian.stokes@intel.com>; dev@dpdk.org
> Cc: stephen@networkplumber.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing,
> Beilei <beilei.xing@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 6/7] net/e1000: set min and max MTU for igb
> devices
> 
> 
> Hi Qi,
> 
> > >
> > > On 3/22/2019 1:01 PM, Ian Stokes wrote:
> > > > This commit sets the min and max supported MTU values for igb
> > > > devices via the eth_igb_info_get() function. Min MTU supported is
> > > > set to ETHER_MIN_MTU and max mtu is calculated as the max packet
> > > > length supported minus the transport overhead. To aid in these
> > > > calculations a new MACRO 'E1000_ETH_OVERHEAD' has been introduced
> > > > to consolidate overhead calculation and avoid duplication.
> > > >
> > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com>
> > > > ---
> > > >  drivers/net/e1000/e1000_ethdev.h | 6 ++++++
> > > >  drivers/net/e1000/igb_ethdev.c   | 7 +++++--
> > > >  2 files changed, 11 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/e1000/e1000_ethdev.h
> > > > b/drivers/net/e1000/e1000_ethdev.h
> > > > index 94edff08e..3e74cd8fe 100644
> > > > --- a/drivers/net/e1000/e1000_ethdev.h
> > > > +++ b/drivers/net/e1000/e1000_ethdev.h
> > > > @@ -89,6 +89,12 @@
> > > >  	ETH_RSS_IPV6_UDP_EX)
> > > >
> > > >  /*
> > > > + * The overhead from MTU to max frame size.
> > > > + * Considering VLAN so a tag needs to be counted.
> > > > + */
> > > > +#define E1000_ETH_OVERHEAD (ETHER_HDR_LEN + ETHER_CRC_LEN +
> > > > +VLAN_TAG_SIZE)
> > >
> > > As an overhead, following drivers set:
> > > i40e: HDR + CRC + 2 * VLAN
> > > ixgbe: HDR + CRC
> > > e1000: HDR + CRC + VLAN
> > >
> > > I wonder if this difference is HW limitation, or driver limitation
> > > or just implementation inconsistency.
> >
> > I think this is implementation inconsistency
> >
> > The NIC only accept Max Frame Size.
> >
> > The problem here is seems all of three setup are not perfect.
> >
> > HDR + CRC + 2 * VLAN - it may allow non vlan or single vlan packet that exceed
> mtu.
> 
> Hmm, wonder how?

I'm talking about the case:

Assume mtu = 1500,  we will set max frame size to 1500 + 14 + 4 + 2*4 = 1526
Let's assume a non vlan packet with 1522 size, so its l2 payload will be 1504 that exceed the mtu,  but it will still be accepted, does it break the configure?

> 
> > HDR + CRC - it may reject vlan or double vlan packet that follow mtu.
> > HDR + CRC + VLAN ,  it may reject double vlan packet that follow mtu
> >
> > I agree it's better to keep consistent on all drivers, but before
> > this, we may need to decide which one we should take :)
> >
> > Regards
> > Qi
> >
> > >
> > > Better to confirm it that it is not implementation inconsistency.
> > >
> > > Wenzhuo, Konstantin, Beilei, Qi,
> > >
> > > Can you please comment?
> > >
> > > Thanks,
> > > ferruh

Ananyev, Konstantin March 26, 2019, 2:18 p.m. UTC | #5

> >
> > Hi Qi,
> >
> > > >
> > > > On 3/22/2019 1:01 PM, Ian Stokes wrote:
> > > > > This commit sets the min and max supported MTU values for igb
> > > > > devices via the eth_igb_info_get() function. Min MTU supported is
> > > > > set to ETHER_MIN_MTU and max mtu is calculated as the max packet
> > > > > length supported minus the transport overhead. To aid in these
> > > > > calculations a new MACRO 'E1000_ETH_OVERHEAD' has been introduced
> > > > > to consolidate overhead calculation and avoid duplication.
> > > > >
> > > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com>
> > > > > ---
> > > > >  drivers/net/e1000/e1000_ethdev.h | 6 ++++++
> > > > >  drivers/net/e1000/igb_ethdev.c   | 7 +++++--
> > > > >  2 files changed, 11 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/e1000/e1000_ethdev.h
> > > > > b/drivers/net/e1000/e1000_ethdev.h
> > > > > index 94edff08e..3e74cd8fe 100644
> > > > > --- a/drivers/net/e1000/e1000_ethdev.h
> > > > > +++ b/drivers/net/e1000/e1000_ethdev.h
> > > > > @@ -89,6 +89,12 @@
> > > > >  	ETH_RSS_IPV6_UDP_EX)
> > > > >
> > > > >  /*
> > > > > + * The overhead from MTU to max frame size.
> > > > > + * Considering VLAN so a tag needs to be counted.
> > > > > + */
> > > > > +#define E1000_ETH_OVERHEAD (ETHER_HDR_LEN + ETHER_CRC_LEN +
> > > > > +VLAN_TAG_SIZE)
> > > >
> > > > As an overhead, following drivers set:
> > > > i40e: HDR + CRC + 2 * VLAN
> > > > ixgbe: HDR + CRC
> > > > e1000: HDR + CRC + VLAN
> > > >
> > > > I wonder if this difference is HW limitation, or driver limitation
> > > > or just implementation inconsistency.
> > >
> > > I think this is implementation inconsistency
> > >
> > > The NIC only accept Max Frame Size.
> > >
> > > The problem here is seems all of three setup are not perfect.
> > >
> > > HDR + CRC + 2 * VLAN - it may allow non vlan or single vlan packet that exceed
> > mtu.
> >
> > Hmm, wonder how?
> 
> I'm talking about the case:
> 
> Assume mtu = 1500,  we will set max frame size to 1500 + 14 + 4 + 2*4 = 1526
> Let's assume a non vlan packet with 1522 size, so its l2 payload will be 1504 that exceed the mtu,  but it will still be accepted, does it break
> the configure?

Of course it would, but as I can read the mail, we discussing overhead
to subtract from max_rx_pkt_len to report max allowable mtu.
From that perspective bigger overhead is more conservative and makes sure
our tx packet will never be bigger than max_rx_pkt_len.
Konstantin

> 
> >
> > > HDR + CRC - it may reject vlan or double vlan packet that follow mtu.
> > > HDR + CRC + VLAN ,  it may reject double vlan packet that follow mtu
> > >
> > > I agree it's better to keep consistent on all drivers, but before
> > > this, we may need to decide which one we should take :)
> > >
> > > Regards
> > > Qi
> > >
> > > >
> > > > Better to confirm it that it is not implementation inconsistency.
> > > >
> > > > Wenzhuo, Konstantin, Beilei, Qi,
> > > >
> > > > Can you please comment?
> > > >
> > > > Thanks,
> > > > ferruh

Qi Zhang March 27, 2019, 1:13 a.m. UTC | #6

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, March 26, 2019 10:18 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Stokes, Ian <ian.stokes@intel.com>; dev@dpdk.org
> Cc: stephen@networkplumber.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing,
> Beilei <beilei.xing@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 6/7] net/e1000: set min and max MTU for igb
> devices
> 
> 
> > >
> > > Hi Qi,
> > >
> > > > >
> > > > > On 3/22/2019 1:01 PM, Ian Stokes wrote:
> > > > > > This commit sets the min and max supported MTU values for igb
> > > > > > devices via the eth_igb_info_get() function. Min MTU supported
> > > > > > is set to ETHER_MIN_MTU and max mtu is calculated as the max
> > > > > > packet length supported minus the transport overhead. To aid
> > > > > > in these calculations a new MACRO 'E1000_ETH_OVERHEAD' has
> > > > > > been introduced to consolidate overhead calculation and avoid
> duplication.
> > > > > >
> > > > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com>
> > > > > > ---
> > > > > >  drivers/net/e1000/e1000_ethdev.h | 6 ++++++
> > > > > >  drivers/net/e1000/igb_ethdev.c   | 7 +++++--
> > > > > >  2 files changed, 11 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/e1000/e1000_ethdev.h
> > > > > > b/drivers/net/e1000/e1000_ethdev.h
> > > > > > index 94edff08e..3e74cd8fe 100644
> > > > > > --- a/drivers/net/e1000/e1000_ethdev.h
> > > > > > +++ b/drivers/net/e1000/e1000_ethdev.h
> > > > > > @@ -89,6 +89,12 @@
> > > > > >  	ETH_RSS_IPV6_UDP_EX)
> > > > > >
> > > > > >  /*
> > > > > > + * The overhead from MTU to max frame size.
> > > > > > + * Considering VLAN so a tag needs to be counted.
> > > > > > + */
> > > > > > +#define E1000_ETH_OVERHEAD (ETHER_HDR_LEN + ETHER_CRC_LEN +
> > > > > > +VLAN_TAG_SIZE)
> > > > >
> > > > > As an overhead, following drivers set:
> > > > > i40e: HDR + CRC + 2 * VLAN
> > > > > ixgbe: HDR + CRC
> > > > > e1000: HDR + CRC + VLAN
> > > > >
> > > > > I wonder if this difference is HW limitation, or driver
> > > > > limitation or just implementation inconsistency.
> > > >
> > > > I think this is implementation inconsistency
> > > >
> > > > The NIC only accept Max Frame Size.
> > > >
> > > > The problem here is seems all of three setup are not perfect.
> > > >
> > > > HDR + CRC + 2 * VLAN - it may allow non vlan or single vlan packet
> > > > that exceed
> > > mtu.
> > >
> > > Hmm, wonder how?
> >
> > I'm talking about the case:
> >
> > Assume mtu = 1500,  we will set max frame size to 1500 + 14 + 4 + 2*4
> > = 1526 Let's assume a non vlan packet with 1522 size, so its l2
> > payload will be 1504 that exceed the mtu,  but it will still be accepted, does it
> break the configure?
> 
> Of course it would, but as I can read the mail, we discussing overhead to subtract
> from max_rx_pkt_len to report max allowable mtu.
> From that perspective bigger overhead is more conservative and makes sure our
> tx packet will never be bigger than max_rx_pkt_len.
> Konstantin

I'm OK to choose HDR + CRC + 2 * VLAN as MTU overhead to keep all driver consistent.

Qi

> 
> >
> > >
> > > > HDR + CRC - it may reject vlan or double vlan packet that follow mtu.
> > > > HDR + CRC + VLAN ,  it may reject double vlan packet that follow
> > > > mtu
> > > >
> > > > I agree it's better to keep consistent on all drivers, but before
> > > > this, we may need to decide which one we should take :)
> > > >
> > > > Regards
> > > > Qi
> > > >
> > > > >
> > > > > Better to confirm it that it is not implementation inconsistency.
> > > > >
> > > > > Wenzhuo, Konstantin, Beilei, Qi,
> > > > >
> > > > > Can you please comment?
> > > > >
> > > > > Thanks,
> > > > > ferruh

Qi Zhang March 27, 2019, 7:02 a.m. UTC | #7

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Wednesday, March 27, 2019 9:14 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Stokes, Ian <ian.stokes@intel.com>; dev@dpdk.org
> Cc: stephen@networkplumber.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing,
> Beilei <beilei.xing@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 6/7] net/e1000: set min and max MTU for igb
> devices
> 
> 
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Tuesday, March 26, 2019 10:18 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Stokes, Ian <ian.stokes@intel.com>;
> > dev@dpdk.org
> > Cc: stephen@networkplumber.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > Xing, Beilei <beilei.xing@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 6/7] net/e1000: set min and max MTU
> > for igb devices
> >
> >
> > > >
> > > > Hi Qi,
> > > >
> > > > > >
> > > > > > On 3/22/2019 1:01 PM, Ian Stokes wrote:
> > > > > > > This commit sets the min and max supported MTU values for
> > > > > > > igb devices via the eth_igb_info_get() function. Min MTU
> > > > > > > supported is set to ETHER_MIN_MTU and max mtu is calculated
> > > > > > > as the max packet length supported minus the transport
> > > > > > > overhead. To aid in these calculations a new MACRO
> > > > > > > 'E1000_ETH_OVERHEAD' has been introduced to consolidate
> > > > > > > overhead calculation and avoid
> > duplication.
> > > > > > >
> > > > > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com>
> > > > > > > ---
> > > > > > >  drivers/net/e1000/e1000_ethdev.h | 6 ++++++
> > > > > > >  drivers/net/e1000/igb_ethdev.c   | 7 +++++--
> > > > > > >  2 files changed, 11 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/e1000/e1000_ethdev.h
> > > > > > > b/drivers/net/e1000/e1000_ethdev.h
> > > > > > > index 94edff08e..3e74cd8fe 100644
> > > > > > > --- a/drivers/net/e1000/e1000_ethdev.h
> > > > > > > +++ b/drivers/net/e1000/e1000_ethdev.h
> > > > > > > @@ -89,6 +89,12 @@
> > > > > > >  	ETH_RSS_IPV6_UDP_EX)
> > > > > > >
> > > > > > >  /*
> > > > > > > + * The overhead from MTU to max frame size.
> > > > > > > + * Considering VLAN so a tag needs to be counted.
> > > > > > > + */
> > > > > > > +#define E1000_ETH_OVERHEAD (ETHER_HDR_LEN + ETHER_CRC_LEN
> +
> > > > > > > +VLAN_TAG_SIZE)
> > > > > >
> > > > > > As an overhead, following drivers set:
> > > > > > i40e: HDR + CRC + 2 * VLAN
> > > > > > ixgbe: HDR + CRC

> > > > > > e1000: HDR + CRC + VLAN
> > > > > >
> > > > > > I wonder if this difference is HW limitation, or driver
> > > > > > limitation or just implementation inconsistency.
> > > > >
> > > > > I think this is implementation inconsistency
> > > > >
> > > > > The NIC only accept Max Frame Size.

Sorry , I need to correct above statement, by checking the ixgbe datasheet, actually max frame size is not include VLAN
Packet with VLAN can be as large as MFS +4 and packet with double vlan can be MFS + 8. It's different with i40e.

So current implementation for mtu overhead subtraction for i40e and ixgbe looks good for me.

> > > > >
> > > > > The problem here is seems all of three setup are not perfect.
> > > > >
> > > > > HDR + CRC + 2 * VLAN - it may allow non vlan or single vlan
> > > > > packet that exceed
> > > > mtu.
> > > >
> > > > Hmm, wonder how?
> > >
> > > I'm talking about the case:
> > >
> > > Assume mtu = 1500,  we will set max frame size to 1500 + 14 + 4 +
> > > 2*4 = 1526 Let's assume a non vlan packet with 1522 size, so its l2
> > > payload will be 1504 that exceed the mtu,  but it will still be
> > > accepted, does it
> > break the configure?
> >
> > Of course it would, but as I can read the mail, we discussing overhead
> > to subtract from max_rx_pkt_len to report max allowable mtu.
> > From that perspective bigger overhead is more conservative and makes
> > sure our tx packet will never be bigger than max_rx_pkt_len.
> > Konstantin
> 
> I'm OK to choose HDR + CRC + 2 * VLAN as MTU overhead to keep all driver
> consistent.

> 
> Qi
> 
> >
> > >
> > > >
> > > > > HDR + CRC - it may reject vlan or double vlan packet that follow mtu.
> > > > > HDR + CRC + VLAN ,  it may reject double vlan packet that follow
> > > > > mtu
> > > > >
> > > > > I agree it's better to keep consistent on all drivers, but
> > > > > before this, we may need to decide which one we should take :)
> > > > >
> > > > > Regards
> > > > > Qi
> > > > >
> > > > > >
> > > > > > Better to confirm it that it is not implementation inconsistency.
> > > > > >
> > > > > > Wenzhuo, Konstantin, Beilei, Qi,
> > > > > >
> > > > > > Can you please comment?
> > > > > >
> > > > > > Thanks,
> > > > > > ferruh

diff mbox series

Patch

diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
index 94edff08e..3e74cd8fe 100644
--- a/drivers/net/e1000/e1000_ethdev.h
+++ b/drivers/net/e1000/e1000_ethdev.h
@@ -89,6 +89,12 @@ 
 	ETH_RSS_IPV6_UDP_EX)
 
 /*
+ * The overhead from MTU to max frame size.
+ * Considering VLAN so a tag needs to be counted.
+ */
+#define E1000_ETH_OVERHEAD (ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TAG_SIZE)
+
+/*
  * Maximum number of Ring Descriptors.
  *
  * Since RDLEN/TDLEN should be multiple of 128 bytes, the number of ring
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 87c9aedf2..b897e8ad4 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -2283,6 +2283,10 @@  eth_igb_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
 			ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
 			ETH_LINK_SPEED_1G;
+
+	dev_info->max_mtu = dev_info->max_rx_pktlen - E1000_ETH_OVERHEAD;
+	dev_info->min_mtu = ETHER_MIN_MTU;
+
 }
 
 static const uint32_t *
@@ -4466,8 +4470,7 @@  eth_igb_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
 	uint32_t rctl;
 	struct e1000_hw *hw;
 	struct rte_eth_dev_info dev_info;
-	uint32_t frame_size = mtu + (ETHER_HDR_LEN + ETHER_CRC_LEN +
-				     VLAN_TAG_SIZE);
+	uint32_t frame_size = mtu + E1000_ETH_OVERHEAD;
 
 	hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);