[PATCH] gianfar: prevent fragmentation in DSA environments

The eTSEC register MRBLR defines the maximum space in
the RX buffers and is set to 1536 by gianfar. This
reasonably covers the common use case where the MTU
is kept at default 1500.

Alas, if the eTSEC is attached to a DSA enabled switch,
the DSA header extension causes every maximum sized
frame to be fragmented by the hardware (1536 + 4).

This patch increases the maximum RX buffer size by
RBUF_ALIGNMENT (64) octets. Since the driver uses a
half-page memory schema, this change does not
increase allocated memory but allows the hardware to
use 1600 bytes of the totally available 2048.

Signed-off-by: Zefir Kurtisi <redacted>
---
 drivers/net/ethernet/freescale/gianfar.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 373fd09..02b794b 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h

@@ -100,7 +100,8 @@ extern const char gfar_driver_version[];
 #define DEFAULT_RX_LFC_THR  16
 #define DEFAULT_LFC_PTVVAL  4
 
-#define GFAR_RXB_SIZE 1536
+/* prevent fragmenation by HW in DSA environments */
+#define GFAR_RXB_SIZE (1536 + RXBUF_ALIGNMENT)
 #define GFAR_SKBFRAG_SIZE (RXBUF_ALIGNMENT + GFAR_RXB_SIZE \
 			  + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
 #define GFAR_RXB_TRUESIZE 2048

-- 
2.7.4

On Fri, Aug 19, 2016 at 11:16:14AM +0200, Zefir Kurtisi wrote:

Hi Zefir

Using RXBUF_ALIGNMENT suggests this has something to do with
alignment, not extra headers.

How about

/* Prevent fragmenation by HW when using extra headers like DSA */
#define GFAR_RXB_SIZE (1536 + 8)

	Andrew

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch]
Sent: Friday, August 19, 2016 5:59 PM
To: Zefir Kurtisi <redacted>
Cc: netdev@vger.kernel.org; claudiu.manoil@freescale.com
Subject: Re: [PATCH] gianfar: prevent fragmentation in DSA environments

On Fri, Aug 19, 2016 at 11:16:14AM +0200, Zefir Kurtisi wrote:

quoted

The eTSEC register MRBLR defines the maximum space in
the RX buffers and is set to 1536 by gianfar. This
reasonably covers the common use case where the MTU
is kept at default 1500.

Alas, if the eTSEC is attached to a DSA enabled switch,
the DSA header extension causes every maximum sized
frame to be fragmented by the hardware (1536 + 4).

This patch increases the maximum RX buffer size by
RBUF_ALIGNMENT (64) octets. Since the driver uses a
half-page memory schema, this change does not
increase allocated memory but allows the hardware to
use 1600 bytes of the totally available 2048.

Signed-off-by: Zefir Kurtisi <redacted>
---
 drivers/net/ethernet/freescale/gianfar.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.h

b/drivers/net/ethernet/freescale/gianfar.h

quoted

index 373fd09..02b794b 100644

--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h

@@ -100,7 +100,8 @@ extern const char gfar_driver_version[];
 #define DEFAULT_RX_LFC_THR  16
 #define DEFAULT_LFC_PTVVAL  4

-#define GFAR_RXB_SIZE 1536
+/* prevent fragmenation by HW in DSA environments */
+#define GFAR_RXB_SIZE (1536 + RXBUF_ALIGNMENT)

Hi Zefir

Using RXBUF_ALIGNMENT suggests this has something to do with
alignment, not extra headers.

How about

/* Prevent fragmenation by HW when using extra headers like DSA */
#define GFAR_RXB_SIZE (1536 + 8)

MRBL (Maximum receive buffer length) must be multiple of 64.
Please consult de hardware documentation at least before prosing
changes to the driver.

Claudiu

On 08/19/2016 04:59 PM, Andrew Lunn wrote:

On Fri, Aug 19, 2016 at 11:16:14AM +0200, Zefir Kurtisi wrote:

quoted

The eTSEC register MRBLR defines the maximum space in
the RX buffers and is set to 1536 by gianfar. This
reasonably covers the common use case where the MTU
is kept at default 1500.

Alas, if the eTSEC is attached to a DSA enabled switch,
the DSA header extension causes every maximum sized
frame to be fragmented by the hardware (1536 + 4).

This patch increases the maximum RX buffer size by
RBUF_ALIGNMENT (64) octets. Since the driver uses a
half-page memory schema, this change does not
increase allocated memory but allows the hardware to
use 1600 bytes of the totally available 2048.

Signed-off-by: Zefir Kurtisi <redacted>
---
 drivers/net/ethernet/freescale/gianfar.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 373fd09..02b794b 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h

@@ -100,7 +100,8 @@ extern const char gfar_driver_version[];
 #define DEFAULT_RX_LFC_THR  16
 #define DEFAULT_LFC_PTVVAL  4
 
-#define GFAR_RXB_SIZE 1536
+/* prevent fragmenation by HW in DSA environments */
+#define GFAR_RXB_SIZE (1536 + RXBUF_ALIGNMENT)

Hi Zefir

Using RXBUF_ALIGNMENT suggests this has something to do with
alignment, not extra headers.

How about

/* Prevent fragmenation by HW when using extra headers like DSA */
#define GFAR_RXB_SIZE (1536 + 8)

	Andrew

Hi Andrew,

the MRBLR has to be written with multiples of 64 (lower 6 bits reserved as 0),
therefore (1536 + 64) is the next valid value to chose.

As noted in the commit message, this is an artificial limitation, since the RX
buffers are essentially 2048 bytes (GFAR_RXB_TRUESIZE) long. I don't fully
understand why GFAR_RXB_SIZE has to be lower than that, preventing
the HW using all available memory - the freescale guys most probably do.


Cheers,
Zefir

quoted

quoted

-#define GFAR_RXB_SIZE 1536
+/* prevent fragmenation by HW in DSA environments */
+#define GFAR_RXB_SIZE (1536 + RXBUF_ALIGNMENT)

Hi Zefir

Using RXBUF_ALIGNMENT suggests this has something to do with
alignment, not extra headers.

How about

/* Prevent fragmenation by HW when using extra headers like DSA */
#define GFAR_RXB_SIZE (1536 + 8)

MRBL (Maximum receive buffer length) must be multiple of 64.
Please consult de hardware documentation at least before prosing
changes to the driver.

 
Hi Claudiu

Thanks for pointing this out. This makes RXBUF_ALIGNMENT even worse
for understabability, since you say multiples of 64 is important, not
alignment.

How about

/* Prevent fragmentation by HW when using extra headers like DSA,
while respecting the multiple of 64 requirement. */

#define GFAR_RXB_SIZE roundup(1536 + 8, 64)

	Andrew

I don't fully understand why GFAR_RXB_SIZE has to be lower than
that, preventing the HW using all available memory - the freescale
guys most probably do.

That will be because the cache invalidates in the DMA API are
expensive. There is no point invalidating the whole 2K if you never
make use of the top part. Just invalidate what is actually used.  I
optimised the Intel i210 driver recently along these lines, and my
receive performance went up 6%

     Andrew

Hi Andrew,


From: Andrew Lunn <andrew@lunn.ch>
Sent: Friday, August 19, 2016 7:39 PM
To: Claudiu Manoil
Cc: Zefir Kurtisi; netdev@vger.kernel.org; claudiu.manoil@freescale.com
Subject: Re: [PATCH] gianfar: prevent fragmentation in DSA environments
    

quoted

quoted

-#define GFAR_RXB_SIZE 1536
+/* prevent fragmenation by HW in DSA environments */
+#define GFAR_RXB_SIZE (1536 + RXBUF_ALIGNMENT)

Hi Zefir

Using RXBUF_ALIGNMENT suggests this has something to do with
alignment, not extra headers.

How about

/* Prevent fragmenation by HW when using extra headers like DSA */
#define GFAR_RXB_SIZE (1536 + 8)

MRBL (Maximum receive buffer length) must be multiple of 64.
Please consult de hardware documentation at least before prosing
changes to the driver.

 
Hi Claudiu

Thanks for pointing this out. This makes RXBUF_ALIGNMENT even worse
for understabability, since you say multiples of 64 is important, not
alignment.

How about

/* Prevent fragmentation by HW when using extra headers like DSA,
while respecting the multiple of 64 requirement. */

#define GFAR_RXB_SIZE roundup(1536 + 8, 64)

[Claudiu]
Nice improvement.
But what's so special about 8?  I thought only 4 bytes were missing :)
At least 1536 is the default size of the MRBLR register, as specified
in the h/w ref manual.  Is there some recommended standard size
to accommodate most (if not all) headers, to refer to?


    

Nice improvement.

Thanks

But what's so special about 8?

When talking to Marvell Switch chips, there are two different headers
which can be added. The DSA header is 4 bytes, and the EDSA header is
8 bytes. However, if there is already a VLAN header, when using EDSA,
it will replace the VLAN header, so only need 4 additional bytes.
There are some very old Marvell switches which add a 4 byte
trailer. If you are talking to a Broadcom switch chip, it also has a 4
byte header.

I thought only 4 bytes were missing :)

So the requirement is probably not currently for the newer Marvell
switch chips? But we should be looking forward and expect at some
point somebody wants to use the newer chips. I've got a Freescale
Vybrid board using the modern Marvell chips, but the FEC driver does
not have a such a hard limit as this driver.

However, it does not seem as simple as that. A standard Ethernet frame
should have a maximum size of 1522 when including a VLAN header. Yet
the driver appears to be using 1536, which is this rounded up to
multiples of 64. So there is already 14 spare bytes in there. So there
must be something else going on here.

At least 1536 is the default size of the MRBLR register, as specified
in the h/w ref manual.  Is there some recommended standard size
to accommodate most (if not all) headers, to refer to?

Not that i know of. These switch headers are proprietary.

    Andrew

On 08/19/2016 11:45 PM, Andrew Lunn wrote:

quoted

Nice improvement.

Thanks

quoted

But what's so special about 8?

When talking to Marvell Switch chips, there are two different headers
which can be added. The DSA header is 4 bytes, and the EDSA header is
8 bytes. However, if there is already a VLAN header, when using EDSA,
it will replace the VLAN header, so only need 4 additional bytes.
There are some very old Marvell switches which add a 4 byte
trailer. If you are talking to a Broadcom switch chip, it also has a 4
byte header.

quoted

I thought only 4 bytes were missing :)

So the requirement is probably not currently for the newer Marvell
switch chips? But we should be looking forward and expect at some
point somebody wants to use the newer chips. I've got a Freescale
Vybrid board using the modern Marvell chips, but the FEC driver does
not have a such a hard limit as this driver.

However, it does not seem as simple as that. A standard Ethernet frame
should have a maximum size of 1522 when including a VLAN header. Yet
the driver appears to be using 1536, which is this rounded up to
multiples of 64. So there is already 14 spare bytes in there. So there
must be something else going on here.

quoted

At least 1536 is the default size of the MRBLR register, as specified
in the h/w ref manual.  Is there some recommended standard size
to accommodate most (if not all) headers, to refer to?

Not that i know of. These switch headers are proprietary.

    Andrew

Hi,

it is a combination of
* (E)DSA header (+4/8 bytes)
* GMAC_FCB_LEN (8)
* FSL_GIANFAR_DEV_HAS_TIMER (causing priv->padding=8)
which sums up to a maximum frame size of 1538 and activates scatter-gather.

A v2 patch with better info is on its way.


Thanks,
Zefir