From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753525AbbFOAbN (ORCPT ); Sun, 14 Jun 2015 20:31:13 -0400 Received: from ozlabs.org ([103.22.144.67]:40973 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752751AbbFOAbD (ORCPT ); Sun, 14 Jun 2015 20:31:03 -0400 From: Rusty Russell To: Dan Streetman Cc: Jani Nikula , Greg Kroah-Hartman , "David S. Miller" , Christoph Hellwig , Andrew Morton , linux-kernel , Dan Streetman Subject: Re: [PATCH] module: add per-module param_lock In-Reply-To: <1434155424-4192-1-git-send-email-ddstreet@ieee.org> References: <1434155424-4192-1-git-send-email-ddstreet@ieee.org> User-Agent: Notmuch/0.17 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Mon, 15 Jun 2015 05:45:04 +0930 Message-ID: <874mmaf3nb.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Dan Streetman writes: > You suggested giving direct access to the mutex, but this patch just > provides lock/unlock functions to the appropriate mutex, which seemed > cleaner and clearer to me, but if you prefer the direct mutex access > I can update the patch for that. Happy with your judgement on this, and I think we've done enough bike-shedding that existant code wins. > Also, this combines the making the kparam_lock into a per-module mutex, > with also removing the kparam_block_sysfs_*() macros; I can split that up > if you would prefer two patches. Hmm, I've split the "make u16 perm const" into its own patch instead, since that's a nice independent change. > +/* Use the module's mutex, or if built-in use the built-in mutex */ > +#define KPARAM_MUTEX(mod) ((mod) ? &(mod)->param_lock : ¶m_lock) > +#define KPARAM_IS_LOCKED(mod) mutex_is_locked(KPARAM_MUTEX(mod)) In future I prefer inlines to macros. In fact, since this is just in params.c it's not too bad to just open-code them in the two places they're each used. But that's nit-picking. Applied, thanks! Rusty. > + > /* This just allows us to keep track of which parameters are kmalloced. */ > struct kmalloced_param { > struct list_head list; > char val[]; > }; > static LIST_HEAD(kmalloced_params); > +static DEFINE_SPINLOCK(kmalloced_params_lock); > > static void *kmalloc_parameter(unsigned int size) > { > @@ -43,7 +48,10 @@ static void *kmalloc_parameter(unsigned int size) > if (!p) > return NULL; > > + spin_lock(&kmalloced_params_lock); > list_add(&p->list, &kmalloced_params); > + spin_unlock(&kmalloced_params_lock); > + > return p->val; > } > > @@ -52,6 +60,7 @@ static void maybe_kfree_parameter(void *param) > { > struct kmalloced_param *p; > > + spin_lock(&kmalloced_params_lock); > list_for_each_entry(p, &kmalloced_params, list) { > if (p->val == param) { > list_del(&p->list); > @@ -59,6 +68,7 @@ static void maybe_kfree_parameter(void *param) > break; > } > } > + spin_unlock(&kmalloced_params_lock); > } > > static char dash2underscore(char c) > @@ -118,10 +128,10 @@ static int parse_one(char *param, > return -EINVAL; > pr_debug("handling %s with %p\n", param, > params[i].ops->set); > - mutex_lock(¶m_lock); > + kernel_param_lock(params[i].mod); > param_check_unsafe(¶ms[i]); > err = params[i].ops->set(val, ¶ms[i]); > - mutex_unlock(¶m_lock); > + kernel_param_unlock(params[i].mod); > return err; > } > } > @@ -364,12 +374,11 @@ EXPORT_SYMBOL(param_ops_invbool); > > int param_set_bint(const char *val, const struct kernel_param *kp) > { > - struct kernel_param boolkp; > + /* Match bool exactly, by re-using it. */ > + struct kernel_param boolkp = *kp; > bool v; > int ret; > > - /* Match bool exactly, by re-using it. */ > - boolkp = *kp; > boolkp.arg = &v; > > ret = param_set_bool(val, &boolkp); > @@ -387,7 +396,8 @@ struct kernel_param_ops param_ops_bint = { > EXPORT_SYMBOL(param_ops_bint); > > /* We break the rule and mangle the string. */ > -static int param_array(const char *name, > +static int param_array(struct module *mod, > + const char *name, > const char *val, > unsigned int min, unsigned int max, > void *elem, int elemsize, > @@ -418,7 +428,7 @@ static int param_array(const char *name, > /* nul-terminate and parse */ > save = val[len]; > ((char *)val)[len] = '\0'; > - BUG_ON(!mutex_is_locked(¶m_lock)); > + BUG_ON(!KPARAM_IS_LOCKED(mod)); > ret = set(val, &kp); > > if (ret != 0) > @@ -440,7 +450,7 @@ static int param_array_set(const char *val, const struct kernel_param *kp) > const struct kparam_array *arr = kp->arr; > unsigned int temp_num; > > - return param_array(kp->name, val, 1, arr->max, arr->elem, > + return param_array(kp->mod, kp->name, val, 1, arr->max, arr->elem, > arr->elemsize, arr->ops->set, kp->level, > arr->num ?: &temp_num); > } > @@ -449,14 +459,13 @@ static int param_array_get(char *buffer, const struct kernel_param *kp) > { > int i, off, ret; > const struct kparam_array *arr = kp->arr; > - struct kernel_param p; > + struct kernel_param p = *kp; > > - p = *kp; > for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) { > if (i) > buffer[off++] = ','; > p.arg = arr->elem + arr->elemsize * i; > - BUG_ON(!mutex_is_locked(¶m_lock)); > + BUG_ON(!KPARAM_IS_LOCKED(p.mod)); > ret = arr->ops->get(buffer + off, &p); > if (ret < 0) > return ret; > @@ -539,9 +548,9 @@ static ssize_t param_attr_show(struct module_attribute *mattr, > if (!attribute->param->ops->get) > return -EPERM; > > - mutex_lock(¶m_lock); > + kernel_param_lock(mk->mod); > count = attribute->param->ops->get(buf, attribute->param); > - mutex_unlock(¶m_lock); > + kernel_param_unlock(mk->mod); > if (count > 0) { > strcat(buf, "\n"); > ++count; > @@ -551,7 +560,7 @@ static ssize_t param_attr_show(struct module_attribute *mattr, > > /* sysfs always hands a nul-terminated string in buf. We rely on that. */ > static ssize_t param_attr_store(struct module_attribute *mattr, > - struct module_kobject *km, > + struct module_kobject *mk, > const char *buf, size_t len) > { > int err; > @@ -560,10 +569,10 @@ static ssize_t param_attr_store(struct module_attribute *mattr, > if (!attribute->param->ops->set) > return -EPERM; > > - mutex_lock(¶m_lock); > + kernel_param_lock(mk->mod); > param_check_unsafe(attribute->param); > err = attribute->param->ops->set(buf, attribute->param); > - mutex_unlock(¶m_lock); > + kernel_param_unlock(mk->mod); > if (!err) > return len; > return err; > @@ -576,18 +585,19 @@ static ssize_t param_attr_store(struct module_attribute *mattr, > #define __modinit __init > #endif > > -#ifdef CONFIG_SYSFS > -void __kernel_param_lock(void) > +void kernel_param_lock(struct module *mod) > { > - mutex_lock(¶m_lock); > + mutex_lock(KPARAM_MUTEX(mod)); > } > -EXPORT_SYMBOL(__kernel_param_lock); > > -void __kernel_param_unlock(void) > +void kernel_param_unlock(struct module *mod) > { > - mutex_unlock(¶m_lock); > + mutex_unlock(KPARAM_MUTEX(mod)); > } > -EXPORT_SYMBOL(__kernel_param_unlock); > + > +#ifdef CONFIG_SYSFS > +EXPORT_SYMBOL(kernel_param_lock); > +EXPORT_SYMBOL(kernel_param_unlock); > > /* > * add_sysfs_param - add a parameter to sysfs > diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c > index d53355b..8544e2e 100644 > --- a/net/mac80211/rate.c > +++ b/net/mac80211/rate.c > @@ -103,7 +103,7 @@ ieee80211_rate_control_ops_get(const char *name) > const struct rate_control_ops *ops; > const char *alg_name; > > - kparam_block_sysfs_write(ieee80211_default_rc_algo); > + kernel_param_lock(THIS_MODULE); > if (!name) > alg_name = ieee80211_default_rc_algo; > else > @@ -117,7 +117,7 @@ ieee80211_rate_control_ops_get(const char *name) > /* try built-in one if specific alg requested but not found */ > if (!ops && strlen(CONFIG_MAC80211_RC_DEFAULT)) > ops = ieee80211_try_rate_control_ops_get(CONFIG_MAC80211_RC_DEFAULT); > - kparam_unblock_sysfs_write(ieee80211_default_rc_algo); > + kernel_param_unlock(THIS_MODULE); > > return ops; > } > -- > 2.1.0