[PATCH 6/9] irda: use sock slab cache

[PATCH 6/9] irda: use sock slab cache

[Arnaldo Carvalho de Melo]

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

Hi David, Jean,

	Please read the log in the patch.

Regards,

- Arnaldo


[-- Attachment #2: irda_sock.patch --]
[-- Type: text/plain, Size: 5066 bytes --]

===================================================================


ChangeSet@1.2342, 2005-01-19 23:04:52-02:00, acme@toy.ghostprotocols.net
  [IRDA] use a private slab cache for socks
  
  Required to get rid of sk_protinfo and to introduce struct connection_sock,
  also for consistency with other protocol families implementations.
  
  Signed-off-by: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
  Signed-off-by: David S. Miller <davem@davemloft.net>


 include/net/irda/af_irda.h |    9 ++++++-
 net/irda/af_irda.c         |   52 +++++++++++++++++++++++----------------------
 2 files changed, 34 insertions(+), 27 deletions(-)


diff -Nru a/include/net/irda/af_irda.h b/include/net/irda/af_irda.h
--- a/include/net/irda/af_irda.h	2005-01-19 23:43:00 -02:00
+++ b/include/net/irda/af_irda.h	2005-01-19 23:43:00 -02:00
@@ -33,9 +33,12 @@
 #include <net/irda/irlmp.h>		/* struct lsap_cb */
 #include <net/irda/irttp.h>		/* struct tsap_cb */
 #include <net/irda/discovery.h>		/* struct discovery_t */
+#include <net/sock.h>
 
 /* IrDA Socket */
 struct irda_sock {
+	/* struct sock has to be the first member of irda_sock */
+	struct sock sk;
 	__u32 saddr;          /* my local address */
 	__u32 daddr;          /* peer address */
 
@@ -69,7 +72,6 @@
 
 	int errno;            /* status of the IAS query */
 
-	struct sock *sk;
 	wait_queue_head_t query_wait;	/* Wait for the answer to a query */
 	struct timer_list watchdog;	/* Timeout for discovery */
 
@@ -77,6 +79,9 @@
 	LOCAL_FLOW rx_flow;
 };
 
-#define irda_sk(__sk) ((struct irda_sock *)(__sk)->sk_protinfo)
+static inline struct irda_sock *irda_sk(struct sock *sk)
+{
+	return (struct irda_sock *)sk;
+}
 
 #endif /* AF_IRDA_H */
diff -Nru a/net/irda/af_irda.c b/net/irda/af_irda.c
--- a/net/irda/af_irda.c	2005-01-19 23:43:00 -02:00
+++ b/net/irda/af_irda.c	2005-01-19 23:43:00 -02:00
@@ -60,6 +60,8 @@
 
 #include <net/irda/af_irda.h>
 
+static kmem_cache_t *irda_sk_slab;
+
 static int irda_create(struct socket *sock, int protocol);
 
 static struct proto_ops irda_stream_ops;
@@ -87,10 +89,10 @@
 
 	IRDA_DEBUG(3, "%s()\n", __FUNCTION__);
 
-	self = (struct irda_sock *) instance;
+	self = instance;
 	ASSERT(self != NULL, return -1;);
 
-	sk = self->sk;
+	sk = instance;
 	ASSERT(sk != NULL, return -1;);
 
 	err = sock_queue_rcv_skb(sk, skb);
@@ -117,7 +119,7 @@
 	struct irda_sock *self;
 	struct sock *sk;
 
-	self = (struct irda_sock *) instance;
+	self = instance;
 
 	IRDA_DEBUG(2, "%s(%p)\n", __FUNCTION__, self);
 
@@ -125,7 +127,7 @@
 	if(skb)
 		dev_kfree_skb(skb);
 
-	sk = self->sk;
+	sk = instance;
 	if (sk == NULL) {
 		IRDA_DEBUG(0, "%s(%p) : BUG : sk is NULL\n",
 			   __FUNCTION__, self);
@@ -183,11 +185,11 @@
 	struct irda_sock *self;
 	struct sock *sk;
 
-	self = (struct irda_sock *) instance;
+	self = instance;
 
 	IRDA_DEBUG(2, "%s(%p)\n", __FUNCTION__, self);
 
-	sk = self->sk;
+	sk = instance;
 	if (sk == NULL) {
 		dev_kfree_skb(skb);
 		return;
@@ -245,11 +247,11 @@
 	struct irda_sock *self;
 	struct sock *sk;
 
-	self = (struct irda_sock *) instance;
+	self = instance;
 
 	IRDA_DEBUG(2, "%s(%p)\n", __FUNCTION__, self);
 
-	sk = self->sk;
+	sk = instance;
 	if (sk == NULL) {
 		dev_kfree_skb(skb);
 		return;
@@ -332,10 +334,10 @@
 
 	IRDA_DEBUG(2, "%s()\n", __FUNCTION__);
 
-	self = (struct irda_sock *) instance;
+	self = instance;
 	ASSERT(self != NULL, return;);
 
-	sk = self->sk;
+	sk = instance;
 	ASSERT(sk != NULL, return;);
 
 	switch (flow) {
@@ -1082,17 +1084,12 @@
 	}
 
 	/* Allocate networking socket */
-	if ((sk = sk_alloc(PF_IRDA, GFP_ATOMIC, 1, NULL)) == NULL)
+	sk = sk_alloc(PF_IRDA, GFP_ATOMIC,
+		      sizeof(struct irda_sock), irda_sk_slab);
+	if (sk == NULL)
 		return -ENOMEM;
 
-	/* Allocate IrDA socket */
-	self = sk->sk_protinfo = kmalloc(sizeof(struct irda_sock), GFP_ATOMIC);
-	if (self == NULL) {
-		sk_free(sk);
-		return -ENOMEM;
-	}
-	memset(self, 0, sizeof(struct irda_sock));
-
+	self = irda_sk(sk);
 	IRDA_DEBUG(2, "%s() : self is %p\n", __FUNCTION__, self);
 
 	init_waitqueue_head(&self->query_wait);
@@ -1102,8 +1099,6 @@
 	sk_set_owner(sk, THIS_MODULE);
 	sk->sk_family = PF_IRDA;
 	sk->sk_protocol = protocol;
-	/* Link networking socket and IrDA socket structs together */
-	self->sk = sk;
 
 	switch (sock->type) {
 	case SOCK_STREAM:
@@ -1187,7 +1182,6 @@
 		self->lsap = NULL;
 	}
 #endif /* CONFIG_IRDA_ULTRA */
-	kfree(self);
 }
 
 /*
@@ -1208,8 +1202,6 @@
 
 	/* Destroy IrDA socket */
 	irda_destroy_socket(irda_sk(sk));
-	/* Prevent sock_def_destruct() to create havoc */
-	sk->sk_protinfo = NULL;
 
 	sock_orphan(sk);
 	sock->sk   = NULL;
@@ -2557,6 +2549,13 @@
  */
 int __init irsock_init(void)
 {
+	irda_sk_slab = kmem_cache_create("irda_sock",
+					 sizeof(struct irda_sock), 0,
+					 SLAB_HWCACHE_ALIGN, NULL, NULL);
+
+	if (irda_sk_slab == NULL)
+		return -ENOMEM;
+
 	sock_register(&irda_family_ops);
 
 	return 0;
@@ -2572,5 +2571,8 @@
 {
 	sock_unregister(PF_IRDA);
 
-        return;
+	if (irda_sk_slab != NULL) {
+		kmem_cache_destroy(irda_sk_slab);
+		irda_sk_slab = NULL;
+	}
 }

Re: [PATCH 6/9] irda: use sock slab cache

[Jean Tourrilhes]

On Thu, Jan 20, 2005 at 12:04:31AM -0200, Arnaldo Carvalho de Melo wrote:
> Hi David, Jean,
> 
> 	Please read the log in the patch.
> 
> Regards,
> 
> - Arnaldo
> 

> ===================================================================
> 
> 
> ChangeSet@1.2342, 2005-01-19 23:04:52-02:00, acme@toy.ghostprotocols.net
>   [IRDA] use a private slab cache for socks
>   
>   Required to get rid of sk_protinfo and to introduce struct connection_sock,
>   also for consistency with other protocol families implementations.

	I don't have anything against this patch, as it looks clean
and straightforward. Stephen might have something to say. I'm way
behind, so it may take a while before I test that.
	I'm just curious about the overhead of adding a specific slab
for IrDA sockets. Most users never create any (using IrCOMM), or
maximum one (using Obex), so it's not like it will get a lot of use
(except here, of course).

	Thanks, and have fun...

	Jean

Re: [PATCH 6/9] irda: use sock slab cache

[Arnaldo Carvalho de Melo]

Jean Tourrilhes escreveu:
> 	I'm just curious about the overhead of adding a specific slab
> for IrDA sockets. Most users never create any (using IrCOMM), or
> maximum one (using Obex), so it's not like it will get a lot of use
> (except here, of course).

Well, lets start with something that may sound funny: when this series
of patches is finished the overhead will _decrease_ for most people.

Why? Today we have in most machines five slab caches of this nature:
udp_sock, raw_sock, tcp_sock, unix_sock (PF_LOCAL) and the generic,
sock, that only is used by the protocols that are using kmalloc(pritave_sock) +
sk_protinfo.

When all protos use private slab caches, the generic slab "sock" is no longer
needed.

So most users will have only the udp, raw, tcp and unix slabs, if they require
another family, say AF_IRDA, they are back to the previous situation, with
5 sock slabs, only when users need two or more extra slabs we get some
overhead wrt the previous situation, but this is compensated by likely
performance gains associated with less cacheline trashing, as noticed when
I first introduced sock slabs, with all the protos converted at that time, i.e.
the 4 most common mentioned above.

And not having two ways to allocate the private area for protos reduces
the networking infrastructure complexity and makes all the networking families
look more similar in its implementation, which facilitates indentifying common
code patterns that can be made generic.

All of this is part of an effort I've been working on for a long time, with the
ultimate goals of reducing the costs associated with maintaining the support
for legacy protocols, taking advantage of the work done on the mainstream
protocols and easing the implementation of new network protocols, such as
DCCP, which I'm working on.

Regards,

- Arnaldo

Re: [PATCH 6/9] irda: use sock slab cache

[Christoph Hellwig]

On Thu, Jan 20, 2005 at 01:47:10AM -0200, Arnaldo Carvalho de Melo wrote:
> >	I'm just curious about the overhead of adding a specific slab
> >for IrDA sockets. Most users never create any (using IrCOMM), or
> >maximum one (using Obex), so it's not like it will get a lot of use
> >(except here, of course).
> 
> Well, lets start with something that may sound funny: when this series
> of patches is finished the overhead will _decrease_ for most people.
> 
> Why? Today we have in most machines five slab caches of this nature:
> udp_sock, raw_sock, tcp_sock, unix_sock (PF_LOCAL) and the generic,
> sock, that only is used by the protocols that are using 
> kmalloc(pritave_sock) +
> sk_protinfo.

But as Jean sais this type of socket is used very little, as are a few
other probably (raw, pfkey?), so maybe those should just use kmalloc +
kfree instead of their own slab?

Re: [PATCH 6/9] irda: use sock slab cache

[Arnaldo Carvalho de Melo]

Christoph Hellwig escreveu:
> On Thu, Jan 20, 2005 at 01:47:10AM -0200, Arnaldo Carvalho de Melo wrote:
> 
>>>	I'm just curious about the overhead of adding a specific slab
>>>for IrDA sockets. Most users never create any (using IrCOMM), or
>>>maximum one (using Obex), so it's not like it will get a lot of use
>>>(except here, of course).
>>
>>Well, lets start with something that may sound funny: when this series
>>of patches is finished the overhead will _decrease_ for most people.
>>
>>Why? Today we have in most machines five slab caches of this nature:
>>udp_sock, raw_sock, tcp_sock, unix_sock (PF_LOCAL) and the generic,
>>sock, that only is used by the protocols that are using 
>>kmalloc(pritave_sock) +
>>sk_protinfo.
> 
> 
> But as Jean sais this type of socket is used very little, as are a few
> other probably (raw, pfkey?), so maybe those should just use kmalloc +
> kfree instead of their own slab?

OK, rethinking the scheme:

Now: rename the zero_it sk_alloc parameter to obj_size, kmalloc the whole
aggregate sock in sk_alloc (e.g. struct irda_sock in the patch that originated
this thread) and set sk->sk_slab to NULL, not using sk_cachep (the generic
"sock" slab), at sk_free time we use this logic:

	if (sk->sk_slab == NULL)
		kfree(sk);
	else
		kmem_cache_free(sk->sk_slab, sk);

This works well for the transitional case, i.e. for the protocols that
still use the "sock" generic slab cache, for the ones that already
use private slab caches (tcp, udp, etc) and for the ones that will
be using this new scheme, with just kmalloc/kfree aggregate, derived
from struct sock, i.e. we can even leave the parameter as "zero_it"
as is, as it will be removed in the future, final scheme, described
in the next paragraph.

Future: rename sk->sk_prot->slab_obj_size to sk->sk_prot->obj_size,
leave sk->sk_prot->slab as NULL, this will make sk_alloc use kmalloc to
allocate sk->sk_prot->obj_size bytes, and sk_free, will just use kfree
when sk->sk_prot->slab is NULL.

What do you guys think? Sane? :-)

David, please don't apply this series of patches, I'll rework it
with this new scheme if nobody pokes any hole in it and resent.

Best Regards,

- Arnaldo

Re: [PATCH 6/9] irda: use sock slab cache

[Arnaldo Carvalho de Melo]

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

Arnaldo Carvalho de Melo escreveu:
> Christoph Hellwig escreveu:
> 
>> On Thu, Jan 20, 2005 at 01:47:10AM -0200, Arnaldo Carvalho de Melo wrote:
>>
>>>>     I'm just curious about the overhead of adding a specific slab
>>>> for IrDA sockets. Most users never create any (using IrCOMM), or
>>>> maximum one (using Obex), so it's not like it will get a lot of use
>>>> (except here, of course).
>>>
>>>
>>> Well, lets start with something that may sound funny: when this series
>>> of patches is finished the overhead will _decrease_ for most people.
>>>
>>> Why? Today we have in most machines five slab caches of this nature:
>>> udp_sock, raw_sock, tcp_sock, unix_sock (PF_LOCAL) and the generic,
>>> sock, that only is used by the protocols that are using 
>>> kmalloc(pritave_sock) +
>>> sk_protinfo.
>>
>>
>>
>> But as Jean sais this type of socket is used very little, as are a few
>> other probably (raw, pfkey?), so maybe those should just use kmalloc +
>> kfree instead of their own slab?
> 
> 
> OK, rethinking the scheme:
> 
> Now: rename the zero_it sk_alloc parameter to obj_size, kmalloc the whole
> aggregate sock in sk_alloc (e.g. struct irda_sock in the patch that 
> originated
> this thread) and set sk->sk_slab to NULL, not using sk_cachep (the generic
> "sock" slab), at sk_free time we use this logic:
> 
>     if (sk->sk_slab == NULL)
>         kfree(sk);
>     else
>         kmem_cache_free(sk->sk_slab, sk);
> 
> This works well for the transitional case, i.e. for the protocols that
> still use the "sock" generic slab cache, for the ones that already
> use private slab caches (tcp, udp, etc) and for the ones that will
> be using this new scheme, with just kmalloc/kfree aggregate, derived
> from struct sock, i.e. we can even leave the parameter as "zero_it"
> as is, as it will be removed in the future, final scheme, described
> in the next paragraph.
> 
> Future: rename sk->sk_prot->slab_obj_size to sk->sk_prot->obj_size,
> leave sk->sk_prot->slab as NULL, this will make sk_alloc use kmalloc to
> allocate sk->sk_prot->obj_size bytes, and sk_free, will just use kfree
> when sk->sk_prot->slab is NULL.
> 
> What do you guys think? Sane? :-)
> 
> David, please don't apply this series of patches, I'll rework it
> with this new scheme if nobody pokes any hole in it and resent.

Take a look at this patch, it shows how I think the transitional
stage should be, the protocols will just use (IRDA for the example):

sk = sk_alloc(PF_IRDA, GFP_KERNEL, sizeof(struct irda_sock), NULL);

The ones still using the generic "sock" slab are using this, that
works with the patch attached:

sk = sk_alloc(PF_AX25, GFP_KERNEL, 1, NULL);
sk->sk_protinfo = kmalloc(sizeof(struct ax25_cb), GFP_KERNEL);

And the ones already using private slab caches:

sk = sk_alloc(PF_UNIX, GFP_KERNEL, sizeof(struct unix_sock), unix_sk_cachep);

All of this should work and when the transition to stop using sk->sk_protinfo
is finished the generic "sock" cache can be safely removed and the logic
in sk_alloc/sk_free will just not use it.

Further on, when all families are converted to using sk->sk_prot sk_alloc will
have this prototype:

struct sock *sk_alloc(struct proto *prot, int priority, int zero_it)

it will get the family from prot->family, the slab from prot->slab and
the size of the object, if zero_it, that will be just a boolean, like it
was before sock slab caches were introduced, is true, we use prot->obj_size
to do the zeroing using memset.

- Arnaldo


[-- Attachment #2: sk_slab.patch --]
[-- Type: text/plain, Size: 1157 bytes --]

===== net/core/sock.c 1.57 vs edited =====
--- 1.57/net/core/sock.c	2005-01-10 18:23:56 -02:00
+++ edited/net/core/sock.c	2005-01-20 13:04:13 -02:00
@@ -621,9 +621,17 @@
 {
 	struct sock *sk = NULL;
 
-	if (!slab)
-		slab = sk_cachep;
-	sk = kmem_cache_alloc(slab, priority);
+	/* FIXME:
+	 * Transitional, will be removed when all the families stop
+	 * using sk->sk_protinfo
+	 */
+	if (zero_it > 1)
+		sk = kmalloc(zero_it, priority);
+	else {
+		if (!slab)
+			slab = sk_cachep;
+		sk = kmem_cache_alloc(slab, priority);
+	}
 	if (sk) {
 		if (zero_it) {
 			memset(sk, 0,
@@ -631,10 +639,15 @@
 			sk->sk_family = family;
 			sock_lock_init(sk);
 		}
-		sk->sk_slab = slab;
+
+		if (zero_it != 1)
+			sk->sk_slab = slab;
 		
 		if (security_sk_alloc(sk, family, priority)) {
-			kmem_cache_free(slab, sk);
+			if (sk->sk_slab)
+				kmem_cache_free(slab, sk);
+			else
+				kfree(sk);
 			sk = NULL;
 		}
 	}
@@ -662,7 +675,10 @@
 		       __FUNCTION__, atomic_read(&sk->sk_omem_alloc));
 
 	security_sk_free(sk);
-	kmem_cache_free(sk->sk_slab, sk);
+	if (sk->sk_slab)
+		kmem_cache_free(sk->sk_slab, sk);
+	else
+		kfree(sk);
 	module_put(owner);
 }
 

Re: [PATCH 6/9] irda: use sock slab cache

[Jean Tourrilhes]

On Thu, Jan 20, 2005 at 01:20:49PM -0200, Arnaldo Carvalho de Melo wrote:
> Arnaldo Carvalho de Melo escreveu:
> >Christoph Hellwig escreveu:
> >
> >>On Thu, Jan 20, 2005 at 01:47:10AM -0200, Arnaldo Carvalho de Melo wrote:
> >>
> >>>>    I'm just curious about the overhead of adding a specific slab
> >>>>for IrDA sockets. Most users never create any (using IrCOMM), or
> >>>>maximum one (using Obex), so it's not like it will get a lot of use
> >>>>(except here, of course).
> >>>
> >>>
> >>>Well, lets start with something that may sound funny: when this series
> >>>of patches is finished the overhead will _decrease_ for most people.
> >>>
> >>>Why? Today we have in most machines five slab caches of this nature:
> >>>udp_sock, raw_sock, tcp_sock, unix_sock (PF_LOCAL) and the generic,
> >>>sock, that only is used by the protocols that are using 
> >>>kmalloc(pritave_sock) +
> >>>sk_protinfo.
> >>
> >>
> >>
> >>But as Jean sais this type of socket is used very little, as are a few
> >>other probably (raw, pfkey?), so maybe those should just use kmalloc +
> >>kfree instead of their own slab?

	Just to clarify, but I think you already got this nuance. I
was not talking about the frequency of people using IrDA. I just say
that when people use IrDA, they are likely to use few sockets (as
opposed to a web browser that open zillions of TCP sockets).
	There may be other socket types that falls under this
category. One example is raw socket. It's used relatively frequently,
but usually the number of raw sockets on the system is limited (how
many instance of tcpdump or ethereal will you run in parallel ?).
	On the other hand, some sockets may be seldom used, but when
used many of them are open, and therefore would justify their own slab
(on demand). Maybe ATM sockets would be a good example.

> Take a look at this patch, it shows how I think the transitional
> stage should be, the protocols will just use (IRDA for the example):

	Can't we just transition IrDA sockets to "future", rather than
"transitional". This way we are done, and no longer need to worry
about it. I mean, you already did the patch, so what's the reason of
holding back ?

	Thanks...

	Jean

Re: [PATCH 6/9] irda: use sock slab cache

[Arnaldo Carvalho de Melo]

Jean Tourrilhes escreveu:
> On Thu, Jan 20, 2005 at 01:20:49PM -0200, Arnaldo Carvalho de Melo wrote:
  >>Take a look at this patch, it shows how I think the transitional
>>stage should be, the protocols will just use (IRDA for the example):
> 
> 
> 	Can't we just transition IrDA sockets to "future", rather than
> "transitional". This way we are done, and no longer need to worry
> about it. I mean, you already did the patch, so what's the reason of
> holding back ?

No, no reason, this latest scheme is good to balance the needs of
performance protocols critical protocols (tcp, etc) and the ones
where the overhead of a private slabcache is too much, the transitional
solution is just to keep the "old protocols", i.e. the ones not
converted yet, working for the time being. IRDA, from what we
discussed, will be converted today to its final form, that is,
without an slab cache, aggregate kmalloc at sk_alloc time, not using
anymore the generic "sock" slab.

- Arnaldo