Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?
From: Thomas Graf <tgraf@suug.ch>
Date: 2007-08-16 15:57:59
* Richard MUSIL [off-list ref] 2007-07-24 13:09
Thomas Graf wrote:quoted
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:
Sorry for taking so long.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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);
This looks good.
quoted hunk ↗ jump to hunk
@@ -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.