Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?
Thomas Graf wrote:
@@ -150,9 +176,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);
For registering operations, it is sufficient to just acquire the
family lock, the family itself can't disappear while holding it.
I agree.
@@ -303,38 +332,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);
I don't like having two locks for something as trivial as this.
Basically the only reason the global lock is required here is to
protect from family removal which can be avoided otherwise by
using RCU list operations.
Therefore, I'd propose the following lock semantics:
Use own global mutex to protect writing to the family list, make
reading side lockless using rcu for use when looking up family
upon mesage processing. Use a family lock to protect writing to
operations list and serialize messae processing with unregister
operations.
I was not aware of RCU lists, but after looking at them, I consider your
proposal to be better. I guess, you would rather write the patch
yourself, so I will wait.
Thanks for help,
Richard
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GENETLINK] some thoughts on the usage
Thomas Graf wrote: * Richard MUSIL [EMAIL PROTECTED] 2007-08-10 10:45 I have noticed that although ops for each family are the same (each device is functionally same) I cannot use same genl_ops struct for registration, because it uses internal member to link in list. Therefore it is necessary to allocate new genl_ops for each device and pass it to registration. But I cannot officially use this list to track those genl_ops (so I can properly destroy them later), because there is no interface. So I need to redo the management of the structures on my own. The intended usage of the interface in your example would be to register only one genetlink family, say tpm, register one set of operations and then have an attribute in every message which specifies which TPM device to use. This helps keeping the total number of genetlink families down. I got your point. The fact that there are several families of the same device type seems however quite convenient. For example, I create/register virtual device /dev/tpm0 and register family with the same name for that device, the same for /dev/tpm1 etc. Then I got straightforward association in between devices and families and get for free the whole management what happen if I try to talk to device which is not registered/present etc. I could multiplex it over one channel, but I will need to make the communication protocol more complex and make me handle all exceptions myself. Since in my case there will be probably not more than one device present, and the device itself is quite exotic I would probably not rewrite it to the multiplexing scheme, but I understand what you mean and will take it into account next time I face decision how to use genetlink. However I am still wondering, whether the allocation of structures (genl_family, genl_ops) should not be done by genetlink layer instead of the user (I mean allocating copy of the struct passed by user). This is for example, what TPM layer (tpm.c) does. This would come at slight cost at memory usage and performance, but will protect the caller from inspecting internal behavior and taking care of that. And internal links would nicely help in keeping of track of allocated structures. I could write a patch for this. The second inconvenience is that for each family I register, I also register basically same ops (basically means, the definitions, and doit, dumpit handlers are same, though the structures are at different addresses for reasons described above). When the handler receives the message it needs to associate the message with the actual device it is handling. This could be done through family lookup (using nlmsghdr::nlmsg_type), but I wondered if it would make sense to extend genl_family for user custom data pointer and then pass this custom data (or genl_family reference) to each handler (for example inside genl_info). It is already parsed by genetlink layer, so it should not slow things down. That's not a bad idea, although I think we should try and keep the generic netlink part as simple as possible. There is a family specific header, referred to as user header in genl_info which is basically what you're looking for with the custom header. I believe making the generic netlink family aware of anything beyond family id and operations id only complicates things. Ok, this was just an idea ;-) - probably important only for high performance genetlink users. Richard - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?
Hello Thomas,
I wonder, if you had time to take a look at the patch I posted back then.
Richard
Thomas Graf wrote:
Please provide a new overall patch which is not based on your
initial patch so I can review your idea properly.
Here it goes (merging two previous patches). I have diffed
against v2.6.22, which I am using currently as my base:
include/net/genetlink.h |1 +
net/netlink/genetlink.c | 106 +++
2 files changed, 80 insertions(+), 27 deletions(-)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index b6eaca1..681ad13 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -25,6 +25,7 @@ struct genl_family
struct nlattr **attrbuf;/* private */
struct list_headops_list; /* private */
struct list_headfamily_list;/* private */
+ struct mutexlock; /* private */
};
/**
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index b9ab62f..0104267 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -38,6 +38,32 @@ static void genl_unlock(void)
genl_sock-sk_data_ready(genl_sock, 0);
}
+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)
@@ -150,9 +176,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;
@@ -180,16 +206,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;
}
@@ -216,8 +242,9 @@ int genl_register_family(struct genl_family *family)
goto errout;
INIT_LIST_HEAD(family-ops_list);
+ mutex_init(family-lock);
- genl_lock();
+ genl_fam_lock(family);
if (genl_family_find_byname(family-name)) {
err = -EEXIST;
@@ -251,14 +278,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;
}
@@ -275,7 +302,7 @@ int genl_unregister_family(struct genl_family *family)
{
struct genl_family *rc;
- genl_lock();
+ genl_fam_lock(family);
list_for_each_entry(rc, genl_family_chain(family-id), family_list) {
if (family-id != rc-id || strcmp(rc-name, family-name))
@@ -283,14 +310,16 @@ 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;
}
@@ -303,38 +332,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,
[GENETLINK] some thoughts on the usage
Hello all, I am currently writing virtual TPM device driver. This is supposed to behave the same way as normal TPM but instead sending commands to hardware device, it will pass them back to user space. Probably similar in concept to tun/tap but with the difference it has nothing to do with networking. I am using genetlink for communication with user space backend. Virtual device manager can create certain number of devices (e.g. up to 8) and it works like this: 1) Create platform device (i.e. /dev/tpm#) 2) Register genetlink family for this device with name /dev/tpm# 3) Register ops for this family. I have noticed that although ops for each family are the same (each device is functionally same) I cannot use same genl_ops struct for registration, because it uses internal member to link in list. Therefore it is necessary to allocate new genl_ops for each device and pass it to registration. But I cannot officially use this list to track those genl_ops (so I can properly destroy them later), because there is no interface. So I need to redo the management of the structures on my own. Simple function genl_get_family_ops probably would do, but I do not know, if what I am trying to do is the intended way of using genetlink, so I am asking first. (Can write patch for it later.) The second inconvenience is that for each family I register, I also register basically same ops (basically means, the definitions, and doit, dumpit handlers are same, though the structures are at different addresses for reasons described above). When the handler receives the message it needs to associate the message with the actual device it is handling. This could be done through family lookup (using nlmsghdr::nlmsg_type), but I wondered if it would make sense to extend genl_family for user custom data pointer and then pass this custom data (or genl_family reference) to each handler (for example inside genl_info). It is already parsed by genetlink layer, so it should not slow things down. What would you say? Richard - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?
Thomas Graf wrote:
Please provide a new overall patch which is not based on your
initial patch so I can review your idea properly.
Here it goes (merging two previous patches). I have diffed
against v2.6.22, which I am using currently as my base:
include/net/genetlink.h |1 +
net/netlink/genetlink.c | 106 +++
2 files changed, 80 insertions(+), 27 deletions(-)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index b6eaca1..681ad13 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -25,6 +25,7 @@ struct genl_family
struct nlattr **attrbuf;/* private */
struct list_headops_list; /* private */
struct list_headfamily_list;/* private */
+ struct mutexlock; /* private */
};
/**
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index b9ab62f..0104267 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -38,6 +38,32 @@ static void genl_unlock(void)
genl_sock-sk_data_ready(genl_sock, 0);
}
+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)
@@ -150,9 +176,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;
@@ -180,16 +206,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;
}
@@ -216,8 +242,9 @@ int genl_register_family(struct genl_family *family)
goto errout;
INIT_LIST_HEAD(family-ops_list);
+ mutex_init(family-lock);
- genl_lock();
+ genl_fam_lock(family);
if (genl_family_find_byname(family-name)) {
err = -EEXIST;
@@ -251,14 +278,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;
}
@@ -275,7 +302,7 @@ int genl_unregister_family(struct genl_family *family)
{
struct genl_family *rc;
- genl_lock();
+ genl_fam_lock(family);
list_for_each_entry(rc, genl_family_chain(family-id), family_list) {
if (family-id != rc-id || strcmp(rc-name, family-name))
@@ -283,14 +310,16 @@ 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;
}
@@ -303,38 +332,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 */
+
Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?
Thomas Graf wrote:
Actually there is no reason to not use separate locks for the
message serialization and the protection of the list of registered
families. There is only one lock simply for the reason that I've
never thought of anybody could think of registering a new genetlink
family while processing a message.
Thomas,
I have been giving it a second thought and came up with something more
complex. The idea is to have locking granularity at the level of
individual families.
Message processing will lock only the family it currently processes
*and* global messaging lock, while family management routines will take
global family lock, and particular lock for family.
This should allow running registration/dereg. from message handler as
long as the family involved is not the same family in which context the
message is being processed.
On the other hand, using lock per family, ensures that message handlers
are not (accidentally) called with invalid references. This should not
be so much problem for dumpit handler, but doit uses for example family
attrs. So I added another patch which implements above described
functionality below.
Alternatively you could also postpone the registration of the new
genetlink family to a workqueue.
To be honest, I cannot judge whether the additional complexity I propose
outweighs the gains. In my book, it definitely does, since I like the
easiness of doit, dumpit handlers and implementation using those is
pretty straightforward.
In the long term I believe that refining the locking could also help
in situations where there is heavy traffic over genetlink, then
all family manipulations will not be blocked (which is currently the case).
Let me know, if you accept it as a patch, or should I eventually go for
plan B ;).
--
Richard
From 63b3ee722402533aed6e137347e41ab1a1fa1127 Mon Sep 17 00:00:00 2001
From: Richard Musil [EMAIL PROTECTED]
Date: Mon, 23 Jul 2007 15:12:09 +0200
Subject: [PATCH] Added private mutex for each genetlink family (struct
genl_family).
This mutex is used to synchronize access to particular family between message
processing handlers and management routines for families
(registering/unregistering families/ops).
This should ensure that another family can be registered or unregistered from
inside genetlink message handler. Trying to register or unregister family
from its own handler will still cause deadlock.
---
include/net/genetlink.h |1 +
net/netlink/genetlink.c | 98 +--
2 files changed, 70 insertions(+), 29 deletions(-)
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index b6eaca1..681ad13 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -25,6 +25,7 @@ struct genl_family
struct nlattr **attrbuf;/* private */
struct list_headops_list; /* private */
struct list_headfamily_list;/* private */
+ struct mutexlock; /* private */
};
/**
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 5ee18eb..0104267 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -40,16 +40,30 @@ static void genl_unlock(void)
static DEFINE_MUTEX(genl_fam_mutex); /* serialization for family list
management */
-static inline void genl_fam_lock(void)
+static inline void genl_fam_lock(struct genl_family *family)
{
mutex_lock(genl_fam_mutex);
+ if (family)
+ mutex_lock(family-lock);
}
-static inline genl_fam_unlock(void)
+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)
@@ -162,9 +176,9 @@ int genl_register_ops(struct genl_family *family, struct
genl_ops *ops)
if (ops-policy)
ops-flags |= GENL_CMD_CAP_HASPOL;
- genl_fam_lock();
+ genl_fam_lock(family);
list_add_tail(ops-ops_list, family-ops_list);
- genl_fam_unlock();
+ genl_fam_unlock(family);
genl_ctrl_event(CTRL_CMD_NEWOPS, ops);
err = 0;
@@ -192,16 +206,16 @@ int genl_unregister_ops(struct genl_family *family,
struct genl_ops *ops)
{
struct genl_ops *rc;
- genl_fam_lock();
+ genl_fam_lock(family);
list_for_each_entry(rc, family-ops_list, ops_list) {
if (rc == ops) {
list_del(ops-ops_list);
- genl_fam_unlock();
+ genl_fam_unlock(family);
genl_ctrl_event(CTRL_CMD_DELOPS, ops);
return 0
[GENETLINK]: Question: global lock (genl_mutex) possible refinement?
I am currently trying to write a module which communicates with user space using NETLINK_GENERIC. This module (dev_mgr) manages virtual devices which are also supposed to use genetlink for communication with user space. I want to do something like that: dev_mgr - receives message from user space to create new device dev_mgr inside 'doit' handler: 1. creates device 2. registers new genetlink family for the device 3. returns family name and id to user This should work similarly for device removal. After few reboots I found out that 2. blocks on genl_mutex, since this mutex is already acquired when genl_register_family is called (by genl_rcv). I do not see why registering new family (when processing message for another family) should be a problem. In fact from genl_lock and genl_trylock occurrence it seems that genl_mutex is mostly used for syncing access to family list and also for message processing. Since I am not (yet) familiar enough with (ge)netlink internals I am asking: Would it make sense to split the mutex into two, one for family list and one for messaging, so it would be possible to change families when processing the message? Simple split could introduce possible danger of user removing family inside processing of the message for this particular family, but would this really be a danger? -- Richard - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?
Patrick McHardy wrote: Richard MUSIL wrote: I am currently trying to write a module which communicates with user space using NETLINK_GENERIC. This module (dev_mgr) manages virtual devices which are also supposed to use genetlink for communication with user space. I want to do something like that: dev_mgr - receives message from user space to create new device dev_mgrinside 'doit' handler: 1. creates device 2. registers new genetlink family for the device 3. returns family name and id to user This should work similarly for device removal. After few reboots I found out that 2. blocks on genl_mutex, since this mutex is already acquired when genl_register_family is called (by genl_rcv). I do not see why registering new family (when processing message for another family) should be a problem. In fact from genl_lock and genl_trylock occurrence it seems that genl_mutex is mostly used for syncing access to family list and also for message processing. Since I am not (yet) familiar enough with (ge)netlink internals I am asking: Would it make sense to split the mutex into two, one for family list and one for messaging, so it would be possible to change families when processing the message? Simple split could introduce possible danger of user removing family inside processing of the message for this particular family, but would this really be a danger? The usual way to do this for auto-loading of modules that register things that take a mutex that is already held during netlink queue processing, like qdiscs, classifiers, .. is: - look for qdisc/classifier/..., if not found: - drop mutex (using the __ unlock variant to avoid reentering queue processing) - perform module loading (which takes the mutex and registers itself) - grab mutex again - look for qdisc/classifier/... again - if not found return -ENOENT - if found drop reference, return -EAGAIN The caller is changed to handle -EAGAIN by replaying the entire request. Your problem sounds very similar, look at net/sched/sch_api.c for an example. The aforementioned mutex is local to genetlink module, so I cannot temporarily drop it, call the stuff and grab it again (which was mine original thought too). In fact the only way to go around (without changing the genetlink) seems to schedule the family registration to some other context outside message processing. But this would be clearly much more complex than doing it directly in message handler and also a bit against ease of use which genetlink is supposed to offer. My question was if its really necessary to sync both message processing and genetlink family management on one primitive. I believe it is not, but I would rather be happy if someone who maintains it confirm this theory. Meanwhile I am going to do quick mod to genetlink and if it goes well, post the patch, which seems to be quite simple. -- Richard - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?
Patrick McHardy wrote:
[ Please quote and break your lines appropriately ]
Oops, sorry for that, definitely was not intentional :(.
Export the lock/unlock/.. functions. You'll also need a new version
similar to __rtnl_unlock.
Patrick, you might feel, I am not reading your lines, but in fact I do.
The problem is that I do not feel competent to follow/propose such
changes. So what I propose here (in included patch) is the least change
scenario, which I can think of and on which I feel safe.
If there are some other changes required, as you suggested for example
exporting lock from genetlink module, I hope authors of genetlink will
comment on that. Currently, I do not see any reason for that, but this
could be due to my limited knowledge.
--
Richard
From a02ef65329fa33591247f9f3a39f2917afe1ce89 Mon Sep 17 00:00:00 2001
From: Richard Musil [EMAIL PROTECTED]
Date: Fri, 20 Jul 2007 17:44:20 +0200
Subject: [PATCH] Added separate mutex for syncing the access to family
list (registration, unregistration, ops, etc.)
This change allows family/ops registration/unregistration in 'doit',
'dumpit' message handlers both which hold message sync. mutex. Original
version used only one global mutex for both managing the families and
processing the messages and led to lock when message processing handler
tried to change families.
---
net/netlink/genetlink.c | 38 +-
1 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index b9ab62f..5ee18eb 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -38,6 +38,18 @@ static void genl_unlock(void)
genl_sock-sk_data_ready(genl_sock, 0);
}
+static DEFINE_MUTEX(genl_fam_mutex); /* serialization for family list
management */
+
+static inline void genl_fam_lock(void)
+{
+ mutex_lock(genl_fam_mutex);
+}
+
+static inline genl_fam_unlock(void)
+{
+ mutex_unlock(genl_fam_mutex);
+}
+
#define GENL_FAM_TAB_SIZE 16
#define GENL_FAM_TAB_MASK (GENL_FAM_TAB_SIZE - 1)
@@ -150,9 +162,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();
list_add_tail(ops-ops_list, family-ops_list);
- genl_unlock();
+ genl_fam_unlock();
genl_ctrl_event(CTRL_CMD_NEWOPS, ops);
err = 0;
@@ -180,16 +192,16 @@ int genl_unregister_ops(struct genl_family *family,
struct genl_ops *ops)
{
struct genl_ops *rc;
- genl_lock();
+ genl_fam_lock();
list_for_each_entry(rc, family-ops_list, ops_list) {
if (rc == ops) {
list_del(ops-ops_list);
- genl_unlock();
+ genl_fam_unlock();
genl_ctrl_event(CTRL_CMD_DELOPS, ops);
return 0;
}
}
- genl_unlock();
+ genl_fam_unlock();
return -ENOENT;
}
@@ -217,7 +229,7 @@ int genl_register_family(struct genl_family *family)
INIT_LIST_HEAD(family-ops_list);
- genl_lock();
+ genl_fam_lock();
if (genl_family_find_byname(family-name)) {
err = -EEXIST;
@@ -251,14 +263,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();
genl_ctrl_event(CTRL_CMD_NEWFAMILY, family);
return 0;
errout_locked:
- genl_unlock();
+ genl_fam_unlock();
errout:
return err;
}
@@ -275,7 +287,7 @@ int genl_unregister_family(struct genl_family *family)
{
struct genl_family *rc;
- genl_lock();
+ genl_fam_lock();
list_for_each_entry(rc, genl_family_chain(family-id), family_list) {
if (family-id != rc-id || strcmp(rc-name, family-name))
@@ -283,14 +295,14 @@ int genl_unregister_family(struct genl_family *family)
list_del(rc-family_list);
INIT_LIST_HEAD(family-ops_list);
- genl_unlock();
+ genl_fam_unlock();
kfree(family-attrbuf);
genl_ctrl_event(CTRL_CMD_DELFAMILY, family);
return 0;
}
- genl_unlock();
+ genl_fam_unlock();
return -ENOENT;
}
@@ -425,7 +437,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();
for (i = 0; i GENL_FAM_TAB_SIZE; i++) {
if (i chains_to_skip)
@@ -445,7 +457,7 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct
netlink_callback *cb)
errout:
if (chains_to_skip != 0)
- genl_unlock
