[PATCH] GENETLINK: Global lock refined to family granularity

[PATCH] GENETLINK: Global lock refined to family granularity

[Richard MUSIL]

This patch is a revive of the patch I posted half a year ago. In that time
Thomas Graf (who was acting as maintainer of genetlink module) accepted
the idea, but suggested to do it using RCU lists. Since I did not have
and experience with RCU lists, we agreed, he will implement it himself.

Since that time, he neither did it, nor replied to my recent emails and
I guess he is no longer active.

I am asking, whomever maintains genetlink right now, for including this
patch. (Currently, this is patch to linux-2.6.25-rc7.)

The idea behind (i.e. why I needed it) is device driver, which uses
genetlink to talk to userspace and which creates virtual devices and
registers families for them in the runtime. So these devices can also
talk to userspace using genetlink.

Current implementation prevents that, because processing any single
genetlink message blocks complete genetlink infrastructure (i.e.
registrations, and deregistrations).

Thanks,
Richard

>From 51b904776bf2ad869f4ddf13c3145ef1ccb6780e Mon Sep 17 00:00:00 2001
From: Richard Musil <richard.musil@st.com>
Date: Wed, 26 Mar 2008 15:59:01 +0100
Subject: [PATCH] GENETLINK: Adding more granularity to locking mechanism.

The old way used one lock for any operation on genetlink layer, which
prevented (for example) registration or removal of genetlink family when
processing genetlink mesasge.

This patch adds locking on family level, allowing only particular family
(which is currently processing the message) to be locked, so registration
or removal of other families can proceed freely.
---
 include/net/genetlink.h |    1 +
 net/netlink/genetlink.c |  106 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 79 insertions(+), 28 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index decdda5..8d4f8b7 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -43,6 +43,7 @@ struct genl_family
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
 	struct list_head	mcast_groups;	/* private */
+ 	struct mutex		lock;		/* private */
 };
 
 /**
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index d16929c..5e95885 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -32,6 +32,32 @@ static inline void genl_unlock(void)
 	mutex_unlock(&genl_mutex);
 }
 
+static DEFINE_MUTEX(genl_fam_mutex);	/* serialization for family list management */
+
+static inline void genl_fam_lock(struct genl_family *family)
+{
+	mutex_lock(&genl_fam_mutex);
+	if (family)
+		mutex_lock(&family->lock);
+}
+
+static inline void genl_fam_unlock(struct genl_family *family)
+{
+	if (family)
+		mutex_unlock(&family->lock);
+	mutex_unlock(&genl_fam_mutex);
+}
+
+static inline void genl_onefam_lock(struct genl_family *family)
+{
+	mutex_lock(&family->lock);
+}
+
+static inline void genl_onefam_unlock(struct genl_family *family)
+{
+	mutex_unlock(&family->lock);
+}
+
 #define GENL_FAM_TAB_SIZE	16
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
@@ -268,9 +294,9 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
 	if (ops->policy)
 		ops->flags |= GENL_CMD_CAP_HASPOL;
 
-	genl_lock();
+	genl_fam_lock(family);
 	list_add_tail(&ops->ops_list, &family->ops_list);
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	genl_ctrl_event(CTRL_CMD_NEWOPS, ops);
 	err = 0;
@@ -298,16 +324,16 @@ int genl_unregister_ops(struct genl_family *family, struct genl_ops *ops)
 {
 	struct genl_ops *rc;
 
-	genl_lock();
+	genl_fam_lock(family);
 	list_for_each_entry(rc, &family->ops_list, ops_list) {
 		if (rc == ops) {
 			list_del(&ops->ops_list);
-			genl_unlock();
+			genl_fam_unlock(family);
 			genl_ctrl_event(CTRL_CMD_DELOPS, ops);
 			return 0;
 		}
 	}
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	return -ENOENT;
 }
@@ -335,8 +361,8 @@ int genl_register_family(struct genl_family *family)
 
 	INIT_LIST_HEAD(&family->ops_list);
 	INIT_LIST_HEAD(&family->mcast_groups);
-
-	genl_lock();
+	mutex_init(&family->lock);
+	genl_fam_lock(family);
 
 	if (genl_family_find_byname(family->name)) {
 		err = -EEXIST;
@@ -370,14 +396,14 @@ int genl_register_family(struct genl_family *family)
 		family->attrbuf = NULL;
 
 	list_add_tail(&family->family_list, genl_family_chain(family->id));
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	genl_ctrl_event(CTRL_CMD_NEWFAMILY, family);
 
 	return 0;
 
 errout_locked:
-	genl_unlock();
+	genl_fam_unlock(family);
 errout:
 	return err;
 }
@@ -394,7 +420,7 @@ int genl_unregister_family(struct genl_family *family)
 {
 	struct genl_family *rc;
 
-	genl_lock();
+	genl_fam_lock(family);
 
 	genl_unregister_mc_groups(family);
 
@@ -404,14 +430,15 @@ int genl_unregister_family(struct genl_family *family)
 
 		list_del(&rc->family_list);
 		INIT_LIST_HEAD(&family->ops_list);
-		genl_unlock();
+		genl_fam_unlock(family);
+		mutex_destroy(&family->lock);
 
 		kfree(family->attrbuf);
 		genl_ctrl_event(CTRL_CMD_DELFAMILY, family);
 		return 0;
 	}
 
-	genl_unlock();
+	genl_fam_unlock(family);
 
 	return -ENOENT;
 }
@@ -424,38 +451,57 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct genlmsghdr *hdr = nlmsg_data(nlh);
 	int hdrlen, err;
 
+	genl_fam_lock(NULL);
 	family = genl_family_find_byid(nlh->nlmsg_type);
-	if (family == NULL)
+	if (family == NULL) {
+		genl_fam_unlock(NULL);
 		return -ENOENT;
+	}
+
+	/* get particular family lock, but release global family lock
+	 * so registering operations for other families are possible */
+	genl_onefam_lock(family);
+	genl_fam_unlock(NULL);
 
 	hdrlen = GENL_HDRLEN + family->hdrsize;
-	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
-		return -EINVAL;
+	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) {
+		err = -EINVAL;
+		goto unlock_out;
+	}
 
 	ops = genl_get_cmd(hdr->cmd, family);
-	if (ops == NULL)
-		return -EOPNOTSUPP;
+	if (ops == NULL) {
+		err = -EOPNOTSUPP;
+		goto unlock_out;
+	}
 
 	if ((ops->flags & GENL_ADMIN_PERM) &&
-	    security_netlink_recv(skb, CAP_NET_ADMIN))
-		return -EPERM;
+	    security_netlink_recv(skb, CAP_NET_ADMIN)) {
+		err = -EPERM;
+		goto unlock_out;
+	}
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
-		if (ops->dumpit == NULL)
-			return -EOPNOTSUPP;
+		if (ops->dumpit == NULL) {
+			err = -EOPNOTSUPP;
+			goto unlock_out;
+		}
 
-		return netlink_dump_start(genl_sock, skb, nlh,
+		err = netlink_dump_start(genl_sock, skb, nlh,
 					  ops->dumpit, ops->done);
+		goto unlock_out;
 	}
 
-	if (ops->doit == NULL)
-		return -EOPNOTSUPP;
+	if (ops->doit == NULL) {
+		err = -EOPNOTSUPP;
+		goto unlock_out;
+	}
 
 	if (family->attrbuf) {
 		err = nlmsg_parse(nlh, hdrlen, family->attrbuf, family->maxattr,
 				  ops->policy);
 		if (err < 0)
-			return err;
+			goto unlock_out;
 	}
 
 	info.snd_seq = nlh->nlmsg_seq;
@@ -465,7 +511,11 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
 	info.attrs = family->attrbuf;
 
-	return ops->doit(skb, &info);
+	err = ops->doit(skb, &info);
+
+unlock_out:
+	genl_onefam_unlock(family);
+	return err;
 }
 
 static void genl_rcv(struct sk_buff *skb)
@@ -602,7 +652,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 	int fams_to_skip = cb->args[1];
 
 	if (chains_to_skip != 0)
-		genl_lock();
+		genl_fam_lock(NULL);
 
 	for (i = 0; i < GENL_FAM_TAB_SIZE; i++) {
 		if (i < chains_to_skip)
@@ -622,7 +672,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
 
 errout:
 	if (chains_to_skip != 0)
-		genl_unlock();
+		genl_fam_unlock(NULL);
 
 	cb->args[0] = i;
 	cb->args[1] = n;
-- 
1.5.4.4

Re: [PATCH] GENETLINK: Global lock refined to family granularity

[David Miller]

From: Richard MUSIL <richard.musil@st.com>
Date: Thu, 27 Mar 2008 13:05:33 +0100

> This patch is a revive of the patch I posted half a year ago. In
> that time Thomas Graf (who was acting as maintainer of genetlink
> module) accepted the idea, but suggested to do it using RCU
> lists. Since I did not have and experience with RCU lists, we
> agreed, he will implement it himself.
> 
> Since that time, he neither did it, nor replied to my recent emails
> and I guess he is no longer active.

He is active, just very busy this past few months.

Please, be patient.

Re: [PATCH] GENETLINK: Global lock refined to family granularity

[Thomas Graf]

* Richard MUSIL <richard.musil@st.com> 2008-03-27 13:05
> This patch is a revive of the patch I posted half a year ago. In that time
> Thomas Graf (who was acting as maintainer of genetlink module) accepted
> the idea, but suggested to do it using RCU lists. Since I did not have
> and experience with RCU lists, we agreed, he will implement it himself.
> 
> Since that time, he neither did it, nor replied to my recent emails and
> I guess he is no longer active.

Sorry but I didn't receive any recent emails.

> I am asking, whomever maintains genetlink right now, for including this
> patch. (Currently, this is patch to linux-2.6.25-rc7.)
> 
> The idea behind (i.e. why I needed it) is device driver, which uses
> genetlink to talk to userspace and which creates virtual devices and
> registers families for them in the runtime. So these devices can also
> talk to userspace using genetlink.
> 
> Current implementation prevents that, because processing any single
> genetlink message blocks complete genetlink infrastructure (i.e.
> registrations, and deregistrations).

On a first sight your patch seemed very legitimate although the amount
of locks taken for every single message received was very irritating.
Also your patch explicitely allows registration operations but when the
family being currently received on is manipulated, it will simply deadlock.
So this patch is about allowing registration of operations for other
families at the cost of two additional locks for every message received
combined with complex locking logic. All a bit much for something that
can be easly solved using a workqueue.

BTW, your patch does not account for multicast group registration.

Re: [PATCH] GENETLINK: Global lock refined to family granularity

[Richard MUSIL]

On 31.3.2008 12:33, Thomas Graf wrote:
> * Richard MUSIL <richard.musil@st.com> 2008-03-27 13:05
>> This patch is a revive of the patch I posted half a year ago. In that time
>> Thomas Graf (who was acting as maintainer of genetlink module) accepted
>> the idea, but suggested to do it using RCU lists. Since I did not have
>> and experience with RCU lists, we agreed, he will implement it himself.
>>
>> Since that time, he neither did it, nor replied to my recent emails and
>> I guess he is no longer active.
> 
> Sorry but I didn't receive any recent emails.

Sent two of them, one on 25.9.2007 and one on 25.1.2008, no reply, must
got lost.

>> I am asking, whomever maintains genetlink right now, for including this
>> patch. (Currently, this is patch to linux-2.6.25-rc7.)
>>
>> The idea behind (i.e. why I needed it) is device driver, which uses
>> genetlink to talk to userspace and which creates virtual devices and
>> registers families for them in the runtime. So these devices can also
>> talk to userspace using genetlink.
>>
>> Current implementation prevents that, because processing any single
>> genetlink message blocks complete genetlink infrastructure (i.e.
>> registrations, and deregistrations).
> 
> On a first sight your patch seemed very legitimate although the amount
> of locks taken for every single message received was very irritating.

It takes two locks, instead of one. I do not see, how it could be less
(in terms of synchronization primitives - but not necessarily mutexes)
if I want that level of granularity. Might be more efficient with RCU
lists (but I do not know), but complexity remains the same.

> Also your patch explicitely allows registration operations but when the
> family being currently received on is manipulated, it will simply deadlock.

In that case, nothing can be done. Of course, trying to unregister
family form its own handler could lead to either crash or deadlock. The
point was, it would not deadlock when manipulating other family - which
happens with current implementation.
Also, it would allow processing of different messages in parallel as
long as they are belonging to different families. Right now, the
processing is blocked as long as any single message is processed.

> So this patch is about allowing registration of operations for other
> families at the cost of two additional locks for every message received

I am not sure about two additional locks, so far I see only one - the
family lock.

> combined with complex locking logic. All a bit much for something that
> can be easly solved using a workqueue.

I am not convinced about that. Does it mean you suggest that any
genetlink message handler should by default offload processing to
workqueue, so it won't block any other message processing from any other
family?

I can imagine running two completely different families with two
completely different needs which do not know about each other at all.
Right now, the one can easily block the other.

So the workqueue does not seem that obvious to me.

> BTW, your patch does not account for multicast group registration.

You right it does not. Should I look into it, or was your answer
definitive no?

Richard

Re: [PATCH] GENETLINK: Global lock refined to family granularity

[Thomas Graf]

* Richard MUSIL <richard.musil@st.com> 2008-03-31 13:21
> It takes two locks, instead of one. I do not see, how it could be less
> (in terms of synchronization primitives - but not necessarily mutexes)
> if I want that level of granularity. Might be more efficient with RCU
> lists (but I do not know), but complexity remains the same.

The synchronization mutex in genl_recv() remains and you add an
additional global mutex genl_fam_lock and a per family lock. That's
2 vs 6 lock operations. It is possible to use RCU to manage the
family, ops and groups list and make the processing path lockless.
All you have to do is synchronize the family unregister function to
message processing as dumping can't disable preempt. Given this, your
patch would be a lot more acceptable to me as the biggest cost would
be reduced and the cost vs gain ratio would be a lot better.

> In that case, nothing can be done. Of course, trying to unregister
> family form its own handler could lead to either crash or deadlock. The
> point was, it would not deadlock when manipulating other family - which
> happens with current implementation.
> Also, it would allow processing of different messages in parallel as
> long as they are belonging to different families. Right now, the
> processing is blocked as long as any single message is processed.

The old behaviour is well defined and obvious. If you allow manipulation
during message processing then allow it for all objects not just some.

I'm all for parallel processing. It has been requested for some time now
by various people but so far nobody got around implementing it. It would
be a very legitimate reason to complicate locking and would even be
worth some additional locking costs. I'm all for this.

> I am not convinced about that. Does it mean you suggest that any
> genetlink message handler should by default offload processing to
> workqueue, so it won't block any other message processing from any other
> family?

I meant deferring the registration of operations to after message
processing using workqueues. Much like it is done for many other things
which can't be done in current locking context.

> You right it does not. Should I look into it, or was your answer
> definitive no?

I'm not against the idea, I just feel that the gains in your patch in
its current form do not justify the additional locking and complextity
costs. Combined with parallel processing and RCU lists it would be a
no brainer and probably merged instantly.

Re: [PATCH] GENETLINK: Global lock refined to family granularity

[Richard MUSIL]

On 31.3.2008 14:26, Thomas Graf wrote:
> * Richard MUSIL <richard.musil@st.com> 2008-03-31 13:21
>> It takes two locks, instead of one. I do not see, how it could be less
>> (in terms of synchronization primitives - but not necessarily mutexes)
>> if I want that level of granularity. Might be more efficient with RCU
>> lists (but I do not know), but complexity remains the same.
> 
> The synchronization mutex in genl_recv() remains and you add an
> additional global mutex genl_fam_lock and a per family lock. That's
> 2 vs 6 lock operations. It is possible to use RCU to manage the

Shame on me, I should know better (what my patch is doing). You are
right, the global lock in genl_recv remains. I guess my further
arguments are now irrelevant.
Concerning the RCU lists, I am also for better solution, the problem is,
I have never used them before, so the probability I will do it right, is
far small - but I may give it try.

<skipped the most since it is no longer relevant>

> I'm not against the idea, I just feel that the gains in your patch in
> its current form do not justify the additional locking and complextity
> costs. Combined with parallel processing and RCU lists it would be a
> no brainer and probably merged instantly.

Ok, I will see, what I can do.

Richard