irda: use sock slab cache

Hi David, Jean,

Please read the log in the patch.

Regards,

- Arnaldo

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

[email protected], 2005-01-19 23:04:52-02:00, [email protected]
[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.

 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
@@ -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 */

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

Jean Tourrilhes escreveu:

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

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?

Christoph Hellwig escreveu:

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

Arnaldo Carvalho de Melo escreveu:

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

===== net/core/sock.c 1.57 vs edited =====

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

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.

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

Jean Tourrilhes escreveu:

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