[PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[Thomas Graf]

NLMSG_GOODSIZE specifies a good default size for the skb tailroom
used in netlink messages when the size is unknown at the time of
the allocation.

The current value doesn't make much sense anymore because
skb_shared_info isn't taken into account which means that
depending on the architecture NLMSG_GOOSIZE can exceed PAGE_SIZE
resulting in a waste of almost a complete page.

Using SKB_MAXORDER solves this potential leak at the cost of
slightly smaller but safer sizes for some architectures.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

--- linux-2.6.11-rc2-bk4.orig/include/linux/netlink.h	2005-01-26 18:19:27.000000000 +0100
+++ linux-2.6.11-rc2-bk4/include/linux/netlink.h	2005-01-28 23:06:02.000000000 +0100
@@ -133,10 +133,9 @@
 
 /*
  *	skb should fit one page. This choice is good for headerless malloc.
- *
- *      FIXME: What is the best size for SLAB???? --ANK
  */
-#define NLMSG_GOODSIZE (PAGE_SIZE - ((sizeof(struct sk_buff)+0xF)&~0xF))
+#define NLMSG_GOODORDER 0
+#define NLMSG_GOODSIZE (SKB_MAX_ORDER(0, NLMSG_GOODORDER))
 
 
 struct netlink_callback

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[Herbert Xu]

Thomas Graf <tgraf@suug.ch> wrote:
> 
> The current value doesn't make much sense anymore because
> skb_shared_info isn't taken into account which means that
> depending on the architecture NLMSG_GOOSIZE can exceed PAGE_SIZE
> resulting in a waste of almost a complete page.

You're quite right.

> Using SKB_MAXORDER solves this potential leak at the cost of
> slightly smaller but safer sizes for some architectures.

At first glance it's not clear which of sk_buff or skb_shared_info
is bigger.  So it might even end up being bigger :)

> -#define NLMSG_GOODSIZE (PAGE_SIZE - ((sizeof(struct sk_buff)+0xF)&~0xF))
> +#define NLMSG_GOODORDER 0
> +#define NLMSG_GOODSIZE (SKB_MAX_ORDER(0, NLMSG_GOODORDER))

Are we ever going use NLMSG_GOODORDER for anything? If not why don't
we go straight to NLMSG_GOODSIZE?

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: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[Thomas Graf]

* Herbert Xu <E1CufRB-0000zf-00@gondolin.me.apana.org.au> 2005-01-29 10:22
> Thomas Graf <tgraf@suug.ch> wrote:
> > 
> > The current value doesn't make much sense anymore because
> > skb_shared_info isn't taken into account which means that
> > depending on the architecture NLMSG_GOOSIZE can exceed PAGE_SIZE
> > resulting in a waste of almost a complete page.
> 
> You're quite right.
> 
> > Using SKB_MAXORDER solves this potential leak at the cost of
> > slightly smaller but safer sizes for some architectures.
> 
> At first glance it's not clear which of sk_buff or skb_shared_info
> is bigger.  So it might even end up being bigger :)

skb_shared_info is 160 bytes and sk_buff is 196 bytes on my box (x86)
but that's not the point. We need to take SMP_CACHE_BYTES alignment into
account which tends to be quite big. The original NLMSG_GOODSIZE
results in 3908 on my box and gets pumped up to 4128 by skb_alloc
(ALIGN(...,SMP_CACHE_BYTES) + sizeof(struct skb_shared_info)) having
SMP_CACHE_BYTES at 128. 

> > -#define NLMSG_GOODSIZE (PAGE_SIZE - ((sizeof(struct sk_buff)+0xF)&~0xF))
> > +#define NLMSG_GOODORDER 0
> > +#define NLMSG_GOODSIZE (SKB_MAX_ORDER(0, NLMSG_GOODORDER))
> 
> Are we ever going use NLMSG_GOODORDER for anything? If not why don't
> we go straight to NLMSG_GOODSIZE?

My thought behind it is that it clearly documents that we use order-0
allocation and we can easly bump it up if we need more space but it's
basically just cosmetic.

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[David S. Miller]

On Sat, 29 Jan 2005 00:40:22 +0100
Thomas Graf <tgraf@suug.ch> wrote:

> > > Using SKB_MAXORDER solves this potential leak at the cost of
> > > slightly smaller but safer sizes for some architectures.
> > 
> > At first glance it's not clear which of sk_buff or skb_shared_info
> > is bigger.  So it might even end up being bigger :)
> 
> skb_shared_info is 160 bytes and sk_buff is 196 bytes on my box (x86)
> but that's not the point. We need to take SMP_CACHE_BYTES alignment into
> account which tends to be quite big. The original NLMSG_GOODSIZE
> results in 3908 on my box and gets pumped up to 4128 by skb_alloc
> (ALIGN(...,SMP_CACHE_BYTES) + sizeof(struct skb_shared_info)) having
> SMP_CACHE_BYTES at 128. 

Furthermore, it's not sk_buff that's ever the issue.  The SKB data
area size is where the skb_shared_info gets tacked onto, sk_buff's
size is never added to this calculation.

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[Alexey Kuznetsov]

Hello!

> > -#define NLMSG_GOODSIZE (PAGE_SIZE - ((sizeof(struct sk_buff)+0xF)&~0xF))


... which reminded me about an old stale patchlet:


Comment: improper size calculation for collapsed skb size.
         Noticed by Denis V. Lunev <den@asplinux.ru>

===== net/ipv4/tcp_input.c 1.89 vs edited =====
--- 1.89/net/ipv4/tcp_input.c	Tue Jan 18 01:09:33 2005
+++ edited/net/ipv4/tcp_input.c	Thu Jan 27 19:41:07 2005
@@ -3760,8 +3760,8 @@
 	while (before(start, end)) {
 		struct sk_buff *nskb;
 		int header = skb_headroom(skb);
-		int copy = (PAGE_SIZE - sizeof(struct sk_buff) -
-			    sizeof(struct skb_shared_info) - header - 31)&~15;
+		int copy = (PAGE_SIZE - sizeof(struct skb_shared_info)
+			    - header - 31)&~15;
 
 		/* Too big header? This can happen with IPv6. */
 		if (copy < 0)

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[Thomas Graf]

* Alexey Kuznetsov <20050128234828.GA24868@yakov.inr.ac.ru> 2005-01-29 02:48
> > > -#define NLMSG_GOODSIZE (PAGE_SIZE - ((sizeof(struct sk_buff)+0xF)&~0xF))
> 
> 
> ... which reminded me about an old stale patchlet:
> 
> 
> Comment: improper size calculation for collapsed skb size.
>          Noticed by Denis V. Lunev <den@asplinux.ru>
> 
> ===== net/ipv4/tcp_input.c 1.89 vs edited =====
> --- 1.89/net/ipv4/tcp_input.c	Tue Jan 18 01:09:33 2005
> +++ edited/net/ipv4/tcp_input.c	Thu Jan 27 19:41:07 2005
> @@ -3760,8 +3760,8 @@
>  	while (before(start, end)) {
>  		struct sk_buff *nskb;
>  		int header = skb_headroom(skb);
> -		int copy = (PAGE_SIZE - sizeof(struct sk_buff) -
> -			    sizeof(struct skb_shared_info) - header - 31)&~15;
> +		int copy = (PAGE_SIZE - sizeof(struct skb_shared_info)
> +			    - header - 31)&~15;

Indeed, it can still exceed PAGE_SIZE in alloc_skb because of the SMP_CACHE_BYTES
alignment though. Can someone enlighten me about the magic 31 in there? I don't get
it from the context. The patch below should do a little bit better

--- linux-2.6.11-rc2-bk4.orig/net/ipv4/tcp_input.c	2005-01-26 18:19:42.000000000 +0100
+++ linux-2.6.11-rc2-bk4/net/ipv4/tcp_input.c	2005-01-29 01:12:30.000000000 +0100
@@ -3760,8 +3760,7 @@
 	while (before(start, end)) {
 		struct sk_buff *nskb;
 		int header = skb_headroom(skb);
-		int copy = (PAGE_SIZE - sizeof(struct sk_buff) -
-			    sizeof(struct skb_shared_info) - header - 31)&~15;
+		int copy = SKB_MAX_ORDER(header + 31, 0);
 
 		/* Too big header? This can happen with IPv6. */
 		if (copy < 0)

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[Thomas Graf]

* Thomas Graf <20050129002128.GX31837@postel.suug.ch> 2005-01-29 01:21
> --- linux-2.6.11-rc2-bk4.orig/net/ipv4/tcp_input.c	2005-01-26 18:19:42.000000000 +0100
> +++ linux-2.6.11-rc2-bk4/net/ipv4/tcp_input.c	2005-01-29 01:12:30.000000000 +0100
> @@ -3760,8 +3760,7 @@
>  	while (before(start, end)) {
>  		struct sk_buff *nskb;
>  		int header = skb_headroom(skb);
> -		int copy = (PAGE_SIZE - sizeof(struct sk_buff) -
> -			    sizeof(struct skb_shared_info) - header - 31)&~15;
> +		int copy = SKB_MAX_ORDER(header + 31, 0);
>  
>  		/* Too big header? This can happen with IPv6. */
>  		if (copy < 0)

Sorry, this is incomplete, we should refetch copy via (skb->end - skb->head) after
allocating it.  I have to think some more about this first. ;-)

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[David S. Miller]

On Sat, 29 Jan 2005 01:27:01 +0100
Thomas Graf <tgraf@suug.ch> wrote:

> * Thomas Graf <20050129002128.GX31837@postel.suug.ch> 2005-01-29 01:21
> > --- linux-2.6.11-rc2-bk4.orig/net/ipv4/tcp_input.c	2005-01-26 18:19:42.000000000 +0100
> > +++ linux-2.6.11-rc2-bk4/net/ipv4/tcp_input.c	2005-01-29 01:12:30.000000000 +0100
> > @@ -3760,8 +3760,7 @@
> >  	while (before(start, end)) {
> >  		struct sk_buff *nskb;
> >  		int header = skb_headroom(skb);
> > -		int copy = (PAGE_SIZE - sizeof(struct sk_buff) -
> > -			    sizeof(struct skb_shared_info) - header - 31)&~15;
> > +		int copy = SKB_MAX_ORDER(header + 31, 0);
> >  
> >  		/* Too big header? This can happen with IPv6. */
> >  		if (copy < 0)
> 
> Sorry, this is incomplete, we should refetch copy via (skb->end - skb->head) after
> allocating it.  I have to think some more about this first. ;-)

I don't understand, if we alloc_skb(copy) we are guarenteed to have
"copy" bytes available in the SKB data area.

This transformation to SKB_MAX_ORDER() (with the "+ 31" removed as
per Alexey's reply), seems perfectly fine to me.

Isn't it?

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[Thomas Graf]

> > >  		struct sk_buff *nskb;
> > >  		int header = skb_headroom(skb);
> > > -		int copy = (PAGE_SIZE - sizeof(struct sk_buff) -
> > > -			    sizeof(struct skb_shared_info) - header - 31)&~15;
> > > +		int copy = SKB_MAX_ORDER(header + 31, 0);
> > >  
> > >  		/* Too big header? This can happen with IPv6. */
> > >  		if (copy < 0)
> > 
> > Sorry, this is incomplete, we should refetch copy via (skb->end - skb->head) after
> > allocating it.  I have to think some more about this first. ;-)
> 
> I don't understand, if we alloc_skb(copy) we are guarenteed to have
> "copy" bytes available in the SKB data area.

Yes but don't we waste space in the headroom of the new skb iff the
headroom of the original skb is no aligned to SKB_DATA_ALIGN?

I'll post a new patch without the anyway though.

[PATCH] NET: Fix calculation for collapsed skb size

[Thomas Graf]

patch also applies to 2.4 with some fuzz.

Noticed by Denis V. Lunev <den@asplinux.ru>

Signed-off-by: Thomas Graf <tgraf@suug.ch>

--- linux-2.6.11-rc3-bk3.orig/net/ipv4/tcp_input.c	2005-02-07 09:47:02.000000000 -0500
+++ linux-2.6.11-rc3-bk3/net/ipv4/tcp_input.c	2005-02-07 09:50:20.000000000 -0500
@@ -3760,8 +3760,7 @@
 	while (before(start, end)) {
 		struct sk_buff *nskb;
 		int header = skb_headroom(skb);
-		int copy = (PAGE_SIZE - sizeof(struct sk_buff) -
-			    sizeof(struct skb_shared_info) - header - 31)&~15;
+		int copy = SKB_MAX_ORDER(header, 0);
 
 		/* Too big header? This can happen with IPv6. */
 		if (copy < 0)

Re: [PATCH] NET: Fix calculation for collapsed skb size

[David S. Miller]

On Mon, 7 Feb 2005 15:27:42 +0100
Thomas Graf <tgraf@suug.ch> wrote:

> patch also applies to 2.4 with some fuzz.
> 
> Noticed by Denis V. Lunev <den@asplinux.ru>
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

Applied, thanks Thomas.

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[Alexey Kuznetsov]

Hello!

> alignment though. Can someone enlighten me about the magic 31 in there?

It was me who wrote both NLMSG_GOODSIZE and this crap. Does the explanation
sound enough? :-)

Apparently it was a brainless attempt of formal merge of (broken)
NLMSG_GOODSIZE and inversed calculation in alloc_skb. Delete this 31.

Alexey

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[Thomas Graf]

* Thomas Graf <20050128230327.GV31837@postel.suug.ch> 2005-01-29 00:03
> --- linux-2.6.11-rc2-bk4.orig/include/linux/netlink.h	2005-01-26 18:19:27.000000000 +0100
> +++ linux-2.6.11-rc2-bk4/include/linux/netlink.h	2005-01-28 23:06:02.000000000 +0100

It is also needed for 2.4 and applies with some fuzz.

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[Thomas Graf]

> NLMSG_GOODSIZE specifies a good default size for the skb tailroom
> used in netlink messages when the size is unknown at the time of
> the allocation.
> 
> The current value doesn't make much sense anymore because
> skb_shared_info isn't taken into account which means that
> depending on the architecture NLMSG_GOOSIZE can exceed PAGE_SIZE
> resulting in a waste of almost a complete page.
> 
> Using SKB_MAXORDER solves this potential leak at the cost of
> slightly smaller but safer sizes for some architectures.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

Dave, did you get this one?

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[David S. Miller]

On Sun, 6 Feb 2005 19:54:17 +0100
Thomas Graf <tgraf@suug.ch> wrote:

> Dave, did you get this one?

Yes I did.  I'm just busy with all the memory barrier stuff
this past few days, sorry.

Re: [PATCH] NETLINK: Use SKB_MAXORDER to calculate NLMSG_GOODSIZE

[David S. Miller]

On Sat, 29 Jan 2005 00:03:27 +0100
Thomas Graf <tgraf@suug.ch> wrote:

> NLMSG_GOODSIZE specifies a good default size for the skb tailroom
> used in netlink messages when the size is unknown at the time of
> the allocation.
> 
> The current value doesn't make much sense anymore because
> skb_shared_info isn't taken into account which means that
> depending on the architecture NLMSG_GOOSIZE can exceed PAGE_SIZE
> resulting in a waste of almost a complete page.
> 
> Using SKB_MAXORDER solves this potential leak at the cost of
> slightly smaller but safer sizes for some architectures.

Applied, to 2.4.x and 2.6.x, thanks Thomas.

This issue comes up again and again.  In the most recent
discussion I remember partaking in, I was trying to get it
so that all the code paths would calculate the size they
actually needed.  Most, if not all, are able to do so.