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

[Arnaldo Carvalho de Melo <acme@conectiva.com.br>]

[-- 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);
 }