Message ID | 1471598174-16938-1-git-send-email-zefir.kurtisi@neratec.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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
>-----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
> >> -#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
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
> 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 > >> -#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: >> 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
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
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(-)