GENETLINK: Global lock refined to family granularity

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: Richard Musil <[email protected]>
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(-)

index decdda5..8d4f8b7 100644
@@ -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 */
 };
 
 /**

1.5.4.4

From: Richard MUSIL <[email protected]>
Date: Thu, 27 Mar 2008 13:05:33 +0100

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

Please, be patient.

* Richard MUSIL <[email protected]> 2008-03-27 13:05

Sorry but I didn't receive any recent emails.

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.

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

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.

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.

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

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.

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

Richard

* Richard MUSIL <[email protected]> 2008-03-31 13:21

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.

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 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.

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.

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>

Ok, I will see, what I can do.

Richard