MAX_SKB_FRAGS and GRO

MAX_SKB_FRAGS and GRO

[Anton Blanchard]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]


Hi Herbert,

I was looking through the hardware GRO support in various drivers and I think
we have a couple of issues with PAGE_SIZE > 4k. For example, if we have a 64kB
page size then MAX_SKB_FRAGS ends up as 3:

#define MAX_SKB_FRAGS (65536/PAGE_SIZE + 2)

This should be fine for hardware and software GSO, but we encounter issues
with hardware GRO (not sure about software GRO).

In the ixgbe case we use MAX_SKB_FRAGS to program the max number of GRO
descriptors, even though we assemble GRO packets using ->frag_list:

#if (MAX_SKB_FRAGS > 16)
                rscctrl |= IXGBE_RSCCTL_MAXDESC_16;
#elif (MAX_SKB_FRAGS > 8)
                rscctrl |= IXGBE_RSCCTL_MAXDESC_8;
#elif (MAX_SKB_FRAGS > 4)
                rscctrl |= IXGBE_RSCCTL_MAXDESC_4;
#else
                rscctrl |= IXGBE_RSCCTL_MAXDESC_1;
#endif

With MAX_SKB_FRAGS = 3 it looks like we only allow 1 GRO descriptor, and since
the card can only do DMAs of 16kB that will be our maximum GRO packet.

I don't have a hardware GRO capable ixgbe to try this out yet, but I think
we want to do something like the attached (completely untested) patch, dividing
GSO_MAX_SIZE by our per packet maximum.

In the cxgb3 case we build GRO packets via ->frags. (FYI I had a look through
the driver but I wasn't able to see where it caps GRO assembly to
MAX_SKB_FRAGS, probably missing something). cxgb3 may be able to do 64kB
DMAs (again not sure), but you could imagine a card with DMA restrictions,
perhaps as bad as 4kB per descriptor. In this case MAX_SKB_FRAGS is sized way
too small.

Thinking out aloud, would setting a pessimistic value for MAX_SKB_FRAGS
be one way to fix this? ie:

#define MAX_SKB_FRAGS (65536/4096 + 2)

I'm not sure if that would break the tx side of some adapters.

Anton

[-- Attachment #2: ixgbe_hw_gro.patch --]
[-- Type: text/x-diff, Size: 1769 bytes --]

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index b5f64ad..283f6cc 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -644,6 +644,13 @@ static inline void ixgbe_release_rx_desc(struct ixgbe_hw *hw,
 	IXGBE_WRITE_REG(hw, IXGBE_RDT(rx_ring->reg_idx), val);
 }
 
+/* The maximum DMA the card can do is 16kB, so cap it if required */
+#if (PAGE_SIZE / 2) > IXGBE_MAX_RXBUFFER
+#define IXGBE_RXBUFFER_SIZE	IXGBE_MAX_RXBUFFER
+#else
+#define IXGBE_RXBUFFER_SIZE	(PAGE_SIZE / 2)
+#endif
+
 /**
  * ixgbe_alloc_rx_buffers - Replace used receive buffers; packet split
  * @adapter: address of board private structure
@@ -2032,11 +2039,7 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
 		  IXGBE_SRRCTL_BSIZEHDR_MASK;
 
 	if (rx_ring->flags & IXGBE_RING_RX_PS_ENABLED) {
-#if (PAGE_SIZE / 2) > IXGBE_MAX_RXBUFFER
-		srrctl |= IXGBE_MAX_RXBUFFER >> IXGBE_SRRCTL_BSIZEPKT_SHIFT;
-#else
-		srrctl |= (PAGE_SIZE / 2) >> IXGBE_SRRCTL_BSIZEPKT_SHIFT;
-#endif
+		srrctl |= IXGBE_RXBUFFER_SIZE >> IXGBE_SRRCTL_BSIZEPKT_SHIFT;
 		srrctl |= IXGBE_SRRCTL_DESCTYPE_HDR_SPLIT_ALWAYS;
 	} else {
 		srrctl |= ALIGN(rx_ring->rx_buf_len, 1024) >>
@@ -2101,11 +2104,11 @@ static void ixgbe_configure_rscctl(struct ixgbe_adapter *adapter, int index)
 	 * than 65535
 	 */
 	if (rx_ring->flags & IXGBE_RING_RX_PS_ENABLED) {
-#if (MAX_SKB_FRAGS > 16)
+#if (GSO_MAX_SIZE / IXGBE_RXBUFFER_SIZE) >= 16
 		rscctrl |= IXGBE_RSCCTL_MAXDESC_16;
-#elif (MAX_SKB_FRAGS > 8)
+#elif (GSO_MAX_SIZE / IXGBE_RXBUFFER_SIZE) >= 8
 		rscctrl |= IXGBE_RSCCTL_MAXDESC_8;
-#elif (MAX_SKB_FRAGS > 4)
+#elif (GSO_MAX_SIZE / IXGBE_RXBUFFER_SIZE) >= 4
 		rscctrl |= IXGBE_RSCCTL_MAXDESC_4;
 #else
 		rscctrl |= IXGBE_RSCCTL_MAXDESC_1;

Re: MAX_SKB_FRAGS and GRO

[Herbert Xu]

On Mon, Feb 08, 2010 at 09:03:07PM +1100, Anton Blanchard wrote:
> 
> I was looking through the hardware GRO support in various drivers and I think
> we have a couple of issues with PAGE_SIZE > 4k. For example, if we have a 64kB
> page size then MAX_SKB_FRAGS ends up as 3:
> 
> #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 2)
> 
> This should be fine for hardware and software GSO, but we encounter issues
> with hardware GRO (not sure about software GRO).
> 
> In the ixgbe case we use MAX_SKB_FRAGS to program the max number of GRO
> descriptors, even though we assemble GRO packets using ->frag_list:
> 
> #if (MAX_SKB_FRAGS > 16)
>                 rscctrl |= IXGBE_RSCCTL_MAXDESC_16;
> #elif (MAX_SKB_FRAGS > 8)
>                 rscctrl |= IXGBE_RSCCTL_MAXDESC_8;
> #elif (MAX_SKB_FRAGS > 4)
>                 rscctrl |= IXGBE_RSCCTL_MAXDESC_4;
> #else
>                 rscctrl |= IXGBE_RSCCTL_MAXDESC_1;
> #endif

First of all this isn't GRO, but RSC.  With GRO we impose extra
restrictions on what packets can be merged while RSC is more
permissive.

In fact I think the ixgbe code may be broken as it is since it's
not marking RSC packets in any way to prevent them from being
forwarded through another interface.

As to your problem with RSC on a 64K page system, I'm sure one
of the Intel developers would be able to help you out.
 
> Thinking out aloud, would setting a pessimistic value for MAX_SKB_FRAGS
> be one way to fix this? ie:
> 
> #define MAX_SKB_FRAGS (65536/4096 + 2)

While I can't think of any serious issues with this, as this is
an entirely ixgbe-specific problem, the fix should probably stay
there.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: MAX_SKB_FRAGS and GRO

[Duyck, Alexander H]

Herbert Xu wrote:
> On Mon, Feb 08, 2010 at 09:03:07PM +1100, Anton Blanchard wrote:
>> 
>> I was looking through the hardware GRO support in various drivers
>> and I think we have a couple of issues with PAGE_SIZE > 4k. For
>> example, if we have a 64kB page size then MAX_SKB_FRAGS ends up as 3:
>> 
>> #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 2)
>> 
>> This should be fine for hardware and software GSO, but we encounter
>> issues with hardware GRO (not sure about software GRO).
>> 
>> In the ixgbe case we use MAX_SKB_FRAGS to program the max number of
>> GRO descriptors, even though we assemble GRO packets using
>> ->frag_list: 
>> 
>> #if (MAX_SKB_FRAGS > 16)
>>                 rscctrl |= IXGBE_RSCCTL_MAXDESC_16; #elif
>>                 (MAX_SKB_FRAGS > 8) rscctrl |=
>> IXGBE_RSCCTL_MAXDESC_8; #elif (MAX_SKB_FRAGS > 4)
>>                 rscctrl |= IXGBE_RSCCTL_MAXDESC_4;
>> #else
>>                 rscctrl |= IXGBE_RSCCTL_MAXDESC_1;
>> #endif
> 
> First of all this isn't GRO, but RSC.  With GRO we impose extra
> restrictions on what packets can be merged while RSC is more
> permissive.
> 
> In fact I think the ixgbe code may be broken as it is since it's
> not marking RSC packets in any way to prevent them from being
> forwarded through another interface.
> 
> As to your problem with RSC on a 64K page system, I'm sure one
> of the Intel developers would be able to help you out.
> 
>> Thinking out aloud, would setting a pessimistic value for
>> MAX_SKB_FRAGS 
>> be one way to fix this? ie:
>> 
>> #define MAX_SKB_FRAGS (65536/4096 + 2)
> 
> While I can't think of any serious issues with this, as this is
> an entirely ixgbe-specific problem, the fix should probably stay
> there.
> 
> Cheers,

Herbert is 100% correct, this is HW RSC and not GRO.

When we are using packet split mode we don't use the frag_list, we use frags and just append the buffers as pages.  This is why we use MAX_SKB_FRAGS to determine how many descriptors can be used for a single RSC receive.  The reason for doing it this way is because we can then hand it off to a process like GRO to join up to 3 frames together on systems with 64K pages.

In regards to the bridging/forwarding issue I don't think there is a problem there since we use the LRO flag to determine if HW RSC can be enabled and so typically if you put the port into a forwarding/bridging state HW RSC is automatically disabled.  I will recommend that our validation team double check that though.

If you are running in single buffer mode, which is the mode that uses the frag_list, then you can join up to 16 descriptors as I recall.  However we must avoid joining enough descriptors to result in 64K or more bytes per frame so usually we end up getting a max of 32K per RSC context simply due to the fact that everything is based on powers of 2.

Thanks,

Alex

Re: MAX_SKB_FRAGS and GRO

[Brandeburg, Jesse]

On Mon, 8 Feb 2010, Herbert Xu wrote:
> On Mon, Feb 08, 2010 at 09:03:07PM +1100, Anton Blanchard wrote:
> > 
> > I was looking through the hardware GRO support in various drivers and I think
> > we have a couple of issues with PAGE_SIZE > 4k. For example, if we have a 64kB
> > page size then MAX_SKB_FRAGS ends up as 3:
> > 
> > #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 2)
> > 
> > This should be fine for hardware and software GSO, but we encounter issues
> > with hardware GRO (not sure about software GRO).
> > 
> > In the ixgbe case we use MAX_SKB_FRAGS to program the max number of GRO
> > descriptors, even though we assemble GRO packets using ->frag_list:
> > 
> > #if (MAX_SKB_FRAGS > 16)
> >                 rscctrl |= IXGBE_RSCCTL_MAXDESC_16;
> > #elif (MAX_SKB_FRAGS > 8)
> >                 rscctrl |= IXGBE_RSCCTL_MAXDESC_8;
> > #elif (MAX_SKB_FRAGS > 4)
> >                 rscctrl |= IXGBE_RSCCTL_MAXDESC_4;
> > #else
> >                 rscctrl |= IXGBE_RSCCTL_MAXDESC_1;
> > #endif
> 
> First of all this isn't GRO, but RSC.  With GRO we impose extra
> restrictions on what packets can be merged while RSC is more
> permissive.

Herbert is right.  Just for clarity lets not call it "hardware GRO" but 
instead just call it RSC or hardware RSC (receive side coalescing)
 
> In fact I think the ixgbe code may be broken as it is since it's
> not marking RSC packets in any way to prevent them from being
> forwarded through another interface.

Herbert, what are we missing? Is there some interface for us to do what 
you describe?  do we need to set DODGY?
 
> As to your problem with RSC on a 64K page system, I'm sure one
> of the Intel developers would be able to help you out.

I'm here, Anton, what can I do or test specifically for you?
  
> > Thinking out aloud, would setting a pessimistic value for MAX_SKB_FRAGS
> > be one way to fix this? ie:
> > 
> > #define MAX_SKB_FRAGS (65536/4096 + 2)
> 
> While I can't think of any serious issues with this, as this is
> an entirely ixgbe-specific problem, the fix should probably stay
> there.

I've always seen MAX_SKB_FRAGS as being the primary way of making sure 
that an SKB never has more than 64kB of data in it.  If we can stuff > 
64kB of data in an skb then some of the above problem with 64kB pages goes 
away.

Re: MAX_SKB_FRAGS and GRO

[Rick Jones]

>>First of all this isn't GRO, but RSC.  With GRO we impose extra
>>restrictions on what packets can be merged while RSC is more
>>permissive.
> 
> 
> Herbert is right.  Just for clarity lets not call it "hardware GRO" but 
> instead just call it RSC or hardware RSC (receive side coalescing)

I'll paint a stripe on the bikeshed and suggest something even more 
explicit - if it is the hardware, how about HRC (Hardware Receive 
Coalescing) or NRC (Nic Receive Coalescing).

rick jones

Re: MAX_SKB_FRAGS and GRO

[Ben Hutchings]

On Mon, 2010-02-08 at 09:53 -0800, Rick Jones wrote:
> >>First of all this isn't GRO, but RSC.  With GRO we impose extra
> >>restrictions on what packets can be merged while RSC is more
> >>permissive.
> > 
> > 
> > Herbert is right.  Just for clarity lets not call it "hardware GRO" but 
> > instead just call it RSC or hardware RSC (receive side coalescing)
> 
> I'll paint a stripe on the bikeshed and suggest something even more 
> explicit - if it is the hardware, how about HRC (Hardware Receive 
> Coalescing) or NRC (Nic Receive Coalescing).

The term 'Large Receive Offload' (LRO) has been around for a while;
what's wrong with that?  Qualify it with 'hardware' (HLRO) if necessary.

Any name involving 'coalescing' would easily be confused with interrupt
moderation, which ethtool calls coalescing.  Also, using anything close
to a proprietary name would only reward and encourage the marketing
geniuses who invent redundant and confusing terminology.

Ben.

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

Re: MAX_SKB_FRAGS and GRO

[Herbert Xu]

On Mon, Feb 08, 2010 at 09:41:58AM -0800, Brandeburg, Jesse wrote:
>  
> > In fact I think the ixgbe code may be broken as it is since it's
> > not marking RSC packets in any way to prevent them from being
> > forwarded through another interface.
> 
> Herbert, what are we missing? Is there some interface for us to do what 
> you describe?  do we need to set DODGY?

The problem is that AFAIK RSC does not impose enough restrictions
on what is merged.  This means that information is lost during the
merge process.  IOW if the merged packet is forwarded we cannot
regenerate the original fragments exactly.

Therefore whenever bridging/routing is used on that interface RSC
must be turned off.

The easiest way to do this is to do what LRO does, which is to
set gso_type to zero and gso_size non-zero.  Of course you'll
also need to use the LRO toggle to control whether RSC is enabled
or not.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt