netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] sfc: Reduce RX scatter buffer size to multiple of 256
@ 2013-05-13 17:48 Ben Hutchings
  2013-05-13 19:02 ` Geert Uytterhoeven
  0 siblings, 1 reply; 5+ messages in thread
From: Ben Hutchings @ 2013-05-13 17:48 UTC (permalink / raw)
  To: David Miller, Heiko Carstens, Geert Uytterhoeven
  Cc: linux-net-drivers, Linux Kernel Development, linux-s390, netdev

efx_start_datapath() asserts that we can fit 2 RX scatter buffers plus
a software structure, each cache-aligned, into a single page.  Where
L1_CACHE_BYTES == 256 and PAGE_SIZE == 4096, which is the case on
s390, this assertion fails.  Reduce EFX_RX_USR_BUF_SIZE to make this
work.

This should also be good for performance, as it ensures that each RX
scatter buffer covers whole cache lines and slightly reduces the use
of DMA writes that can require a read-modify-write on inter-processor
links.

(We could use 2048 - L1_CACHE_BYTES, but EFX_RX_USR_BUF_SIZE also
affects user-level networking where a larger amount of housekeeping
data may be needed.  Although this version of the driver does not
support user-level networking, I prefer to keep scattering behaviour
consistent with the out-of-tree version.)

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
Heiko or Geert, please confirm that this really does fix the build
failure - I don't have an s390 toolchain.

Ben.

 drivers/net/ethernet/sfc/net_driver.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 9bd433a..6c8e1a8 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -72,8 +72,10 @@
 /* Maximum possible MTU the driver supports */
 #define EFX_MAX_MTU (9 * 1024)
 
-/* Size of an RX scatter buffer.  Small enough to pack 2 into a 4K page. */
-#define EFX_RX_USR_BUF_SIZE 1824
+/* Size of an RX scatter buffer.  Small enough to pack 2 into a 4K page,
+ * and should be a multiple of the cache line size.
+ */
+#define EFX_RX_USR_BUF_SIZE	(2048 - 256)
 
 /* Forward declare Precision Time Protocol (PTP) support structure. */
 struct efx_ptp_data;

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net] sfc: Reduce RX scatter buffer size to multiple of 256
  2013-05-13 17:48 [PATCH net] sfc: Reduce RX scatter buffer size to multiple of 256 Ben Hutchings
@ 2013-05-13 19:02 ` Geert Uytterhoeven
  2013-05-13 19:11   ` Ben Hutchings
  0 siblings, 1 reply; 5+ messages in thread
From: Geert Uytterhoeven @ 2013-05-13 19:02 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: David Miller, Heiko Carstens, Solarflare linux maintainers,
	Linux Kernel Development, linux-s390, netdev

Hi Ben,

On Mon, May 13, 2013 at 7:48 PM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> efx_start_datapath() asserts that we can fit 2 RX scatter buffers plus
> a software structure, each cache-aligned, into a single page.  Where
> L1_CACHE_BYTES == 256 and PAGE_SIZE == 4096, which is the case on
> s390, this assertion fails.  Reduce EFX_RX_USR_BUF_SIZE to make this
> work.
>
> This should also be good for performance, as it ensures that each RX
> scatter buffer covers whole cache lines and slightly reduces the use
> of DMA writes that can require a read-modify-write on inter-processor
> links.
>
> (We could use 2048 - L1_CACHE_BYTES, but EFX_RX_USR_BUF_SIZE also
> affects user-level networking where a larger amount of housekeeping
> data may be needed.  Although this version of the driver does not
> support user-level networking, I prefer to keep scattering behaviour
> consistent with the out-of-tree version.)
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> Heiko or Geert, please confirm that this really does fix the build

Thanks! But unfortunately I still get the same error:

  CC [M]  drivers/net/ethernet/sfc/efx.o
/scratch/geert/linux/linux-m68k/drivers/net/ethernet/sfc/efx.c: In
function 'efx_start_datapath':
/scratch/geert/linux/linux-m68k/drivers/net/ethernet/sfc/efx.c:646:3:
error: call to '__compiletime_assert_648' declared with attribute
error: BUILD_BUG_ON failed: sizeof(struct efx_rx_page_state) +
EFX_PAGE_IP_ALIGN + EFX_RX_USR_BUF_SIZE > PAGE_SIZE / 2
make[4]: *** [drivers/net/ethernet/sfc/efx.o] Error 1

> failure - I don't have an s390 toolchain.

http://kernel.org/pub/tools/crosstool/files/bin/

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] sfc: Reduce RX scatter buffer size to multiple of 256
  2013-05-13 19:02 ` Geert Uytterhoeven
@ 2013-05-13 19:11   ` Ben Hutchings
  2013-05-13 19:16     ` Geert Uytterhoeven
  0 siblings, 1 reply; 5+ messages in thread
From: Ben Hutchings @ 2013-05-13 19:11 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: David Miller, Heiko Carstens, Solarflare linux maintainers,
	Linux Kernel Development, linux-s390, netdev

On Mon, 2013-05-13 at 21:02 +0200, Geert Uytterhoeven wrote:
> Hi Ben,
> 
> On Mon, May 13, 2013 at 7:48 PM, Ben Hutchings
> <bhutchings@solarflare.com> wrote:
> > efx_start_datapath() asserts that we can fit 2 RX scatter buffers plus
> > a software structure, each cache-aligned, into a single page.  Where
> > L1_CACHE_BYTES == 256 and PAGE_SIZE == 4096, which is the case on
> > s390, this assertion fails.  Reduce EFX_RX_USR_BUF_SIZE to make this
> > work.
> >
> > This should also be good for performance, as it ensures that each RX
> > scatter buffer covers whole cache lines and slightly reduces the use
> > of DMA writes that can require a read-modify-write on inter-processor
> > links.
> >
> > (We could use 2048 - L1_CACHE_BYTES, but EFX_RX_USR_BUF_SIZE also
> > affects user-level networking where a larger amount of housekeeping
> > data may be needed.  Although this version of the driver does not
> > support user-level networking, I prefer to keep scattering behaviour
> > consistent with the out-of-tree version.)
> >
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> > ---
> > Heiko or Geert, please confirm that this really does fix the build
> 
> Thanks! But unfortunately I still get the same error:
> 
>   CC [M]  drivers/net/ethernet/sfc/efx.o
> /scratch/geert/linux/linux-m68k/drivers/net/ethernet/sfc/efx.c: In
> function 'efx_start_datapath':
> /scratch/geert/linux/linux-m68k/drivers/net/ethernet/sfc/efx.c:646:3:
> error: call to '__compiletime_assert_648' declared with attribute
> error: BUILD_BUG_ON failed: sizeof(struct efx_rx_page_state) +
> EFX_PAGE_IP_ALIGN + EFX_RX_USR_BUF_SIZE > PAGE_SIZE / 2
> make[4]: *** [drivers/net/ethernet/sfc/efx.o] Error 1

OK, this doesn't work because on s390 EFX_PAGE_IP_ALIGN == 2 (this macro
is equivalent to NET_IP_ALIGN, though that wasn't always true).  So DMA
is going to be misaligned on s390 anyway.

I'll see if I can work out a sensible definition that works for s390
while still being good for x86 and powerpc (which are the two we mostly
care about).

> > failure - I don't have an s390 toolchain.
> 
> http://kernel.org/pub/tools/crosstool/files/bin/

I know, but this still takes time to set up.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] sfc: Reduce RX scatter buffer size to multiple of 256
  2013-05-13 19:11   ` Ben Hutchings
@ 2013-05-13 19:16     ` Geert Uytterhoeven
  2013-05-13 19:36       ` Ben Hutchings
  0 siblings, 1 reply; 5+ messages in thread
From: Geert Uytterhoeven @ 2013-05-13 19:16 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: David Miller, Heiko Carstens, Solarflare linux maintainers,
	Linux Kernel Development, linux-s390, netdev

On Mon, May 13, 2013 at 9:11 PM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
>> error: BUILD_BUG_ON failed: sizeof(struct efx_rx_page_state) +
>> EFX_PAGE_IP_ALIGN + EFX_RX_USR_BUF_SIZE > PAGE_SIZE / 2
>> make[4]: *** [drivers/net/ethernet/sfc/efx.o] Error 1
>
> OK, this doesn't work because on s390 EFX_PAGE_IP_ALIGN == 2 (this macro

Yeah, that's what I just discovered, too.

#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
#define EFX_PAGE_IP_ALIGN 0
#else
#define EFX_PAGE_IP_ALIGN NET_IP_ALIGN
#endif

> is equivalent to NET_IP_ALIGN, though that wasn't always true).  So DMA
> is going to be misaligned on s390 anyway.

Hmm, so it's making the choice between misaligned CPU and misaligned
DMA? Sounds fishy...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] sfc: Reduce RX scatter buffer size to multiple of 256
  2013-05-13 19:16     ` Geert Uytterhoeven
@ 2013-05-13 19:36       ` Ben Hutchings
  0 siblings, 0 replies; 5+ messages in thread
From: Ben Hutchings @ 2013-05-13 19:36 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: David Miller, Heiko Carstens, Solarflare linux maintainers,
	Linux Kernel Development, linux-s390, netdev

On Mon, 2013-05-13 at 21:16 +0200, Geert Uytterhoeven wrote:
> On Mon, May 13, 2013 at 9:11 PM, Ben Hutchings
> <bhutchings@solarflare.com> wrote:
> >> error: BUILD_BUG_ON failed: sizeof(struct efx_rx_page_state) +
> >> EFX_PAGE_IP_ALIGN + EFX_RX_USR_BUF_SIZE > PAGE_SIZE / 2
> >> make[4]: *** [drivers/net/ethernet/sfc/efx.o] Error 1
> >
> > OK, this doesn't work because on s390 EFX_PAGE_IP_ALIGN == 2 (this macro
> 
> Yeah, that's what I just discovered, too.
> 
> #ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> #define EFX_PAGE_IP_ALIGN 0
> #else
> #define EFX_PAGE_IP_ALIGN NET_IP_ALIGN
> #endif
> 
> > is equivalent to NET_IP_ALIGN, though that wasn't always true).  So DMA
> > is going to be misaligned on s390 anyway.
> 
> Hmm, so it's making the choice between misaligned CPU and misaligned
> DMA? Sounds fishy...

When writing headers into an skb, we need to ensure that the IP header
is aligned as required by the CPU.  I'm not sure whether this is true
for page buffers; it will depend on whether GRO pulls the IP header into
an skb header area before looking at it.

For RX DMA, our controller doesn't care about alignment but there is a
performance concern.  So where possible we prefer to align the start of
the DMA buffer with a cache line.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2013-05-13 19:36 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-13 17:48 [PATCH net] sfc: Reduce RX scatter buffer size to multiple of 256 Ben Hutchings
2013-05-13 19:02 ` Geert Uytterhoeven
2013-05-13 19:11   ` Ben Hutchings
2013-05-13 19:16     ` Geert Uytterhoeven
2013-05-13 19:36       ` Ben Hutchings

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).