Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

Thomas Graf wrote:

quoted

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

quoted

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