[PATCH] gianfar: prevent fragmentation in DSA environments

[PATCH] gianfar: prevent fragmentation in DSA environments

[Zefir Kurtisi]

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 <zefir.kurtisi@neratec.com>
---
 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

Re: [PATCH] gianfar: prevent fragmentation in DSA environments

[Andrew Lunn]

On Fri, Aug 19, 2016 at 11:16:14AM +0200, Zefir Kurtisi wrote:
> 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 <zefir.kurtisi@neratec.com>
> ---
>  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

RE: [PATCH] gianfar: prevent fragmentation in DSA environments

[Claudiu Manoil]

>-----Original Message-----
>From: Andrew Lunn [mailto:andrew@lunn.ch]
>Sent: Friday, August 19, 2016 5:59 PM
>To: Zefir Kurtisi <zefir.kurtisi@neratec.com>
>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:
>> 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 <zefir.kurtisi@neratec.com>
>> ---
>>  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)
>

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

Claudiu

Re: [PATCH] gianfar: prevent fragmentation in DSA environments

[Andrew Lunn]

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

Re: [PATCH] gianfar: prevent fragmentation in DSA environments

[Claudiu Manoil]

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
    
> >> -#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?


    

Re: [PATCH] gianfar: prevent fragmentation in DSA environments

[Andrew Lunn]

> 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

Re: [PATCH] gianfar: prevent fragmentation in DSA environments

[Zefir Kurtisi]

On 08/19/2016 11:45 PM, Andrew Lunn wrote:
>> 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
> 
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

Re: [PATCH] gianfar: prevent fragmentation in DSA environments

[Zefir Kurtisi]

On 08/19/2016 04:59 PM, Andrew Lunn wrote:
> On Fri, Aug 19, 2016 at 11:16:14AM +0200, Zefir Kurtisi wrote:
>> 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 <zefir.kurtisi@neratec.com>
>> ---
>>  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

Re: [PATCH] gianfar: prevent fragmentation in DSA environments

[Andrew Lunn]

> 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