* Re: [PATCH] modules: elide param_lock if !CONFIG_SYSFS
@ 2015-06-25 21:18 ` Rusty Russell
0 siblings, 0 replies; 9+ messages in thread
From: Rusty Russell @ 2015-06-25 21:18 UTC (permalink / raw)
To: Dan Streetman; +Cc: Stephen Rothwell, linux-next, linux-kernel, Dan Streetman
Dan Streetman <ddstreet@ieee.org> writes:
> Only include the built-in and per-module param_lock, and corresponding
> lock/unlock functions, if sysfs is enabled. If there is no sysfs there
> is no need for locking kernel params.
>
> This fixes a build break when CONFIG_SYSFS is not enabled, introduced
> by commit b51d23e.
This doesn't even come close to applying to my tree?
I've fixed it like so, and tested it compiles both with and without
SYSFS.
Subject: param: fix module param locks when !CONFIG_SYSFS.
As Dan Streetman points out, the entire point of locking for is to
stop sysfs accesses, so they're elided entirely in the !SYSFS case.
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
diff --git a/include/linux/module.h b/include/linux/module.h
index abaecf1a63df..237bce382e32 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -240,7 +240,9 @@ struct module {
unsigned int num_syms;
/* Kernel parameters. */
+#ifdef CONFIG_SYSFS
struct mutex param_lock;
+#endif
struct kernel_param *kp;
unsigned int num_kp;
diff --git a/kernel/module.c b/kernel/module.c
index 1dcf09f214fa..c19b2c50b317 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1820,6 +1820,10 @@ static void mod_sysfs_fini(struct module *mod)
mod_kobject_put(mod);
}
+static void init_param_lock(struct module *mod)
+{
+ mutex_init(&mod->param_lock);
+}
#else /* !CONFIG_SYSFS */
static int mod_sysfs_setup(struct module *mod,
@@ -1842,6 +1846,9 @@ static void del_usage_links(struct module *mod)
{
}
+static void init_param_lock(struct module *mod)
+{
+}
#endif /* CONFIG_SYSFS */
static void mod_sysfs_teardown(struct module *mod)
@@ -3442,7 +3449,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
if (err)
goto unlink_mod;
- mutex_init(&mod->param_lock);
+ init_param_lock(mod);
/* Now we've got everything in the final locations, we can
* find optional sections. */
diff --git a/kernel/params.c b/kernel/params.c
index 8890d0b8dffc..faa461c16f12 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -25,12 +25,22 @@
#include <linux/slab.h>
#include <linux/ctype.h>
+#ifdef CONFIG_SYSFS
/* Protects all built-in parameters, modules use their own param_lock */
static DEFINE_MUTEX(param_lock);
/* 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))
+
+static inline void check_kparam_locked(struct module *mod)
+{
+ BUG_ON(!mutex_is_locked(KPARAM_MUTEX(mod)));
+}
+#else
+static inline void check_kparam_locked(struct module *mod)
+{
+}
+#endif /* !CONFIG_SYSFS */
/* This just allows us to keep track of which parameters are kmalloced. */
struct kmalloced_param {
@@ -459,7 +469,7 @@ static int param_array(struct module *mod,
/* nul-terminate and parse */
save = val[len];
((char *)val)[len] = '\0';
- BUG_ON(!KPARAM_IS_LOCKED(mod));
+ check_kparam_locked(mod);
ret = set(val, &kp);
if (ret != 0)
@@ -496,7 +506,7 @@ static int param_array_get(char *buffer, const struct kernel_param *kp)
if (i)
buffer[off++] = ',';
p.arg = arr->elem + arr->elemsize * i;
- BUG_ON(!KPARAM_IS_LOCKED(p.mod));
+ check_kparam_locked(p.mod);
ret = arr->ops->get(buffer + off, &p);
if (ret < 0)
return ret;
@@ -616,6 +626,7 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
#define __modinit __init
#endif
+#ifdef CONFIG_SYSFS
void kernel_param_lock(struct module *mod)
{
mutex_lock(KPARAM_MUTEX(mod));
@@ -626,7 +637,6 @@ void kernel_param_unlock(struct module *mod)
mutex_unlock(KPARAM_MUTEX(mod));
}
-#ifdef CONFIG_SYSFS
EXPORT_SYMBOL(kernel_param_lock);
EXPORT_SYMBOL(kernel_param_unlock);
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] modules: elide param_lock if !CONFIG_SYSFS
2015-06-25 21:18 ` Rusty Russell
(?)
@ 2015-06-25 21:34 ` Dan Streetman
2015-06-27 0:21 ` Stephen Rothwell
-1 siblings, 1 reply; 9+ messages in thread
From: Dan Streetman @ 2015-06-25 21:34 UTC (permalink / raw)
To: Rusty Russell; +Cc: Stephen Rothwell, linux-next, linux-kernel
On Thu, Jun 25, 2015 at 5:18 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Dan Streetman <ddstreet@ieee.org> writes:
>> Only include the built-in and per-module param_lock, and corresponding
>> lock/unlock functions, if sysfs is enabled. If there is no sysfs there
>> is no need for locking kernel params.
>>
>> This fixes a build break when CONFIG_SYSFS is not enabled, introduced
>> by commit b51d23e.
>
> This doesn't even come close to applying to my tree?
sorry, I had the !CONFIG_MODULES patch in my tree also, so this was on
top of that one:
https://lkml.org/lkml/2015/6/24/550
I can resend that one, on top of this one, or you can fix it up.
Sorry for not getting it right the first time ;-)
>
> I've fixed it like so, and tested it compiles both with and without
> SYSFS.
>
> Subject: param: fix module param locks when !CONFIG_SYSFS.
>
> As Dan Streetman points out, the entire point of locking for is to
> stop sysfs accesses, so they're elided entirely in the !SYSFS case.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index abaecf1a63df..237bce382e32 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -240,7 +240,9 @@ struct module {
> unsigned int num_syms;
>
> /* Kernel parameters. */
> +#ifdef CONFIG_SYSFS
> struct mutex param_lock;
> +#endif
> struct kernel_param *kp;
> unsigned int num_kp;
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 1dcf09f214fa..c19b2c50b317 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1820,6 +1820,10 @@ static void mod_sysfs_fini(struct module *mod)
> mod_kobject_put(mod);
> }
>
> +static void init_param_lock(struct module *mod)
> +{
> + mutex_init(&mod->param_lock);
> +}
> #else /* !CONFIG_SYSFS */
>
> static int mod_sysfs_setup(struct module *mod,
> @@ -1842,6 +1846,9 @@ static void del_usage_links(struct module *mod)
> {
> }
>
> +static void init_param_lock(struct module *mod)
> +{
> +}
> #endif /* CONFIG_SYSFS */
>
> static void mod_sysfs_teardown(struct module *mod)
> @@ -3442,7 +3449,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> if (err)
> goto unlink_mod;
>
> - mutex_init(&mod->param_lock);
> + init_param_lock(mod);
oops, i missed fixing this up, when I tested with allnoconfig this
wasn't built due to CONFIG_MODULES=n. I should have built only with
syfs=n :(
>
> /* Now we've got everything in the final locations, we can
> * find optional sections. */
> diff --git a/kernel/params.c b/kernel/params.c
> index 8890d0b8dffc..faa461c16f12 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -25,12 +25,22 @@
> #include <linux/slab.h>
> #include <linux/ctype.h>
>
> +#ifdef CONFIG_SYSFS
> /* Protects all built-in parameters, modules use their own param_lock */
> static DEFINE_MUTEX(param_lock);
>
> /* 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))
> +
> +static inline void check_kparam_locked(struct module *mod)
> +{
> + BUG_ON(!mutex_is_locked(KPARAM_MUTEX(mod)));
> +}
> +#else
> +static inline void check_kparam_locked(struct module *mod)
> +{
> +}
> +#endif /* !CONFIG_SYSFS */
>
> /* This just allows us to keep track of which parameters are kmalloced. */
> struct kmalloced_param {
> @@ -459,7 +469,7 @@ static int param_array(struct module *mod,
> /* nul-terminate and parse */
> save = val[len];
> ((char *)val)[len] = '\0';
> - BUG_ON(!KPARAM_IS_LOCKED(mod));
> + check_kparam_locked(mod);
> ret = set(val, &kp);
>
> if (ret != 0)
> @@ -496,7 +506,7 @@ static int param_array_get(char *buffer, const struct kernel_param *kp)
> if (i)
> buffer[off++] = ',';
> p.arg = arr->elem + arr->elemsize * i;
> - BUG_ON(!KPARAM_IS_LOCKED(p.mod));
> + check_kparam_locked(p.mod);
> ret = arr->ops->get(buffer + off, &p);
> if (ret < 0)
> return ret;
> @@ -616,6 +626,7 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
> #define __modinit __init
> #endif
>
> +#ifdef CONFIG_SYSFS
> void kernel_param_lock(struct module *mod)
> {
> mutex_lock(KPARAM_MUTEX(mod));
> @@ -626,7 +637,6 @@ void kernel_param_unlock(struct module *mod)
> mutex_unlock(KPARAM_MUTEX(mod));
> }
>
> -#ifdef CONFIG_SYSFS
> EXPORT_SYMBOL(kernel_param_lock);
> EXPORT_SYMBOL(kernel_param_unlock);
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] modules: elide param_lock if !CONFIG_SYSFS
2015-06-25 21:34 ` Dan Streetman
@ 2015-06-27 0:21 ` Stephen Rothwell
2015-06-28 5:21 ` Rusty Russell
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Rothwell @ 2015-06-27 0:21 UTC (permalink / raw)
To: Rusty Russell; +Cc: Dan Streetman, linux-next, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1776 bytes --]
Hi Rusty,
On Thu, 25 Jun 2015 17:34:31 -0400 Dan Streetman <ddstreet@ieee.org> wrote:
>
> On Thu, Jun 25, 2015 at 5:18 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > Dan Streetman <ddstreet@ieee.org> writes:
> >> Only include the built-in and per-module param_lock, and corresponding
> >> lock/unlock functions, if sysfs is enabled. If there is no sysfs there
> >> is no need for locking kernel params.
> >>
> >> This fixes a build break when CONFIG_SYSFS is not enabled, introduced
> >> by commit b51d23e.
> >
> > This doesn't even come close to applying to my tree?
>
> sorry, I had the !CONFIG_MODULES patch in my tree also, so this was on
> top of that one:
> https://lkml.org/lkml/2015/6/24/550
>
> I can resend that one, on top of this one, or you can fix it up.
>
> Sorry for not getting it right the first time ;-)
This is what I ended up applying to yesterday's linux-next:
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Fri, 26 Jun 2015 13:19:19 +1000
Subject: [PATCH] modules: only use mod->param_lock if CONFIG_MODULES
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
kernel/params.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/params.c b/kernel/params.c
index faa461c16f12..adc0bbc06cc5 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -30,7 +30,11 @@
static DEFINE_MUTEX(param_lock);
/* Use the module's mutex, or if built-in use the built-in mutex */
+#ifdef CONFIG_MODULES
#define KPARAM_MUTEX(mod) ((mod) ? &(mod)->param_lock : ¶m_lock)
+#else
+#define KPARAM_MUTEX(mod) (¶m_lock)
+#endif
static inline void check_kparam_locked(struct module *mod)
{
--
2.1.4
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] modules: elide param_lock if !CONFIG_SYSFS
2015-06-27 0:21 ` Stephen Rothwell
@ 2015-06-28 5:21 ` Rusty Russell
0 siblings, 0 replies; 9+ messages in thread
From: Rusty Russell @ 2015-06-28 5:21 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: Dan Streetman, linux-next, linux-kernel
Stephen Rothwell <sfr@canb.auug.org.au> writes:
> Hi Rusty,
>
> On Thu, 25 Jun 2015 17:34:31 -0400 Dan Streetman <ddstreet@ieee.org> wrote:
>>
>> On Thu, Jun 25, 2015 at 5:18 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> > Dan Streetman <ddstreet@ieee.org> writes:
>> >> Only include the built-in and per-module param_lock, and corresponding
>> >> lock/unlock functions, if sysfs is enabled. If there is no sysfs there
>> >> is no need for locking kernel params.
>> >>
>> >> This fixes a build break when CONFIG_SYSFS is not enabled, introduced
>> >> by commit b51d23e.
>> >
>> > This doesn't even come close to applying to my tree?
>>
>> sorry, I had the !CONFIG_MODULES patch in my tree also, so this was on
>> top of that one:
>> https://lkml.org/lkml/2015/6/24/550
>>
>> I can resend that one, on top of this one, or you can fix it up.
>>
>> Sorry for not getting it right the first time ;-)
>
> This is what I ended up applying to yesterday's linux-next:
Thanks, added correct description to previous patch and applied this
on top.
Sorry for the hassle,
Rusty.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] modules: elide param_lock if !CONFIG_SYSFS
2015-06-25 21:18 ` Rusty Russell
(?)
(?)
@ 2015-06-25 22:39 ` Stephen Rothwell
-1 siblings, 0 replies; 9+ messages in thread
From: Stephen Rothwell @ 2015-06-25 22:39 UTC (permalink / raw)
To: Rusty Russell; +Cc: Dan Streetman, linux-next, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1113 bytes --]
Hi Rusty,
On Fri, 26 Jun 2015 06:48:37 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> Dan Streetman <ddstreet@ieee.org> writes:
> > Only include the built-in and per-module param_lock, and corresponding
> > lock/unlock functions, if sysfs is enabled. If there is no sysfs there
> > is no need for locking kernel params.
> >
> > This fixes a build break when CONFIG_SYSFS is not enabled, introduced
> > by commit b51d23e.
>
> This doesn't even come close to applying to my tree?
That's because (as Dan noted) there was another fix patch in the tree
he was fixing ...
> I've fixed it like so, and tested it compiles both with and without
> SYSFS.
>
> Subject: param: fix module param locks when !CONFIG_SYSFS.
>
> As Dan Streetman points out, the entire point of locking for is to
> stop sysfs accesses, so they're elided entirely in the !SYSFS case.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Except the commit you added to the modules tree has no signed off by or
commentary :-(
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread