Re: [RFC] module: add per-module params lock
On Mon, Jun 8, 2015 at 5:13 PM, Rusty Russell wrote: > Dan Streetman writes: >> On Thu, Jun 4, 2015 at 8:42 PM, Rusty Russell wrote: >>> Dan Streetman writes: I sent this as part of a patch series a few days ago, which I was asked to break up, so I'm sending only this patch as a RFC now, until I work out the details of the zswap patch that needs this. I'd like to get comments on this early, since it changes the way module param locking is done. >>> >>> OK, it's not insane, but I'm not entirely convinced. >>> >>> 1) The difference between blocking sysfs for read vs write is mainly >>>documentation. In theory, it allows a rwsem, though in practice it's >>>not been a bottleneck to now. >> >> The sysfs block/unblock calls could remain the same, but the downside >> there is a module can't block more than 1 of its params. It seemed to >> me like the per-param r/w block functions were adding unnecessary >> restrictions, since we're not using a rwsem for each individual param. >> If the param lock is changed to a rwsem in the future, it won't be >> hard to also change all callers. Or, we could change it to a resem >> now. But as you said, kernel param locking isn't really a contended >> lock. >> >>> >>> 2) Implicit is bad: implying the module rather than the parameter is >>>weird >> >> Well implying it enforces only blocking your own module params; should >> module A be blocking and accessing module B's params? Isn't requiring >> each module to pass THIS_MODULE unnecessary, at least in the vast >> majority of cases? There is still __kernel_param_lock(module) that >> can be used if it really is necessary anywhere to block some other >> module's params. Or, I can change it to require passing THIS_MODULE >> if you think that's a better api. > > It's weird to do anything other than lock the actual parameter you're > talking about. Nobody actually seems to want multi param locks (and if > they try it they'll quickly discover it deadlocks). > >>> , and skips the BUG_ON() check which was there before. >> >> those made absolutely no sense to me; why in the world is it necessary >> to BUG() simply because the param permissions don't match r or w? At >> *most* that deserves a WARN_ON() but more likely a pr_warn() is >> appropriate. And even then, who cares? Does it actually cause any >> harm? No. Especially since the underlying lock isn't a rwsem. But >> even if it *was* a rwsem, it still wouldn't hurt anything. > > Because you're locking something that can't change. I really want that > not to happen: I'd make it BUILD_BUG_ON() if I could. Ah, ok. I dug in there and I see the perm field isn't actually changeable from sysfs, it's only the initial setting; so chmod will only change the sysfs file perm, not the kernel_param.perm field that the block_sysfs macros check. If that's the case and perm *really* should never change, why not just make it const and use BUILD_BUG_ON()? I'll send a patch... > > You're confusing the API (which explicitly DOESN'T talk about locking, > just blocking sysfs access), and the implementation. For the majority > of users, the API is more important. > > If you really dislike the API (and it's such a minor one), perhaps it's > better to expose the lock explicitly: I got to this point only because I was getting deadlocked when loading a module from a param callback, so the API was really secondary to me; I don't hate the API; and BUILD_BUG_ON seems a lot better. If the global mutex -> per module mutex seems ok, I can send reduce the patch to only do that without changing the API. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] module: add per-module params lock
Dan Streetman writes:
> On Thu, Jun 4, 2015 at 8:42 PM, Rusty Russell wrote:
>> Dan Streetman writes:
>>> I sent this as part of a patch series a few days ago, which I was asked to
>>> break up, so I'm sending only this patch as a RFC now, until I work out
>>> the details of the zswap patch that needs this. I'd like to get comments
>>> on this early, since it changes the way module param locking is done.
>>
>> OK, it's not insane, but I'm not entirely convinced.
>>
>> 1) The difference between blocking sysfs for read vs write is mainly
>>documentation. In theory, it allows a rwsem, though in practice it's
>>not been a bottleneck to now.
>
> The sysfs block/unblock calls could remain the same, but the downside
> there is a module can't block more than 1 of its params. It seemed to
> me like the per-param r/w block functions were adding unnecessary
> restrictions, since we're not using a rwsem for each individual param.
> If the param lock is changed to a rwsem in the future, it won't be
> hard to also change all callers. Or, we could change it to a resem
> now. But as you said, kernel param locking isn't really a contended
> lock.
>
>>
>> 2) Implicit is bad: implying the module rather than the parameter is
>>weird
>
> Well implying it enforces only blocking your own module params; should
> module A be blocking and accessing module B's params? Isn't requiring
> each module to pass THIS_MODULE unnecessary, at least in the vast
> majority of cases? There is still __kernel_param_lock(module) that
> can be used if it really is necessary anywhere to block some other
> module's params. Or, I can change it to require passing THIS_MODULE
> if you think that's a better api.
It's weird to do anything other than lock the actual parameter you're
talking about. Nobody actually seems to want multi param locks (and if
they try it they'll quickly discover it deadlocks).
>> , and skips the BUG_ON() check which was there before.
>
> those made absolutely no sense to me; why in the world is it necessary
> to BUG() simply because the param permissions don't match r or w? At
> *most* that deserves a WARN_ON() but more likely a pr_warn() is
> appropriate. And even then, who cares? Does it actually cause any
> harm? No. Especially since the underlying lock isn't a rwsem. But
> even if it *was* a rwsem, it still wouldn't hurt anything.
Because you're locking something that can't change. I really want that
not to happen: I'd make it BUILD_BUG_ON() if I could.
You're confusing the API (which explicitly DOESN'T talk about locking,
just blocking sysfs access), and the implementation. For the majority
of users, the API is more important.
If you really dislike the API (and it's such a minor one), perhaps it's
better to expose the lock explicitly:
/* Stops sysfs updates to the module's parameters */
static inline struct mutex *module_param_mutex(struct module *mod)
{
return mod ? mod->param_lock : param_lock;
}
And make the callers do the obvious thing with it?
Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] module: add per-module params lock
On Thu, Jun 4, 2015 at 8:42 PM, Rusty Russell wrote: > Dan Streetman writes: >> I sent this as part of a patch series a few days ago, which I was asked to >> break up, so I'm sending only this patch as a RFC now, until I work out >> the details of the zswap patch that needs this. I'd like to get comments >> on this early, since it changes the way module param locking is done. > > OK, it's not insane, but I'm not entirely convinced. > > 1) The difference between blocking sysfs for read vs write is mainly >documentation. In theory, it allows a rwsem, though in practice it's >not been a bottleneck to now. The sysfs block/unblock calls could remain the same, but the downside there is a module can't block more than 1 of its params. It seemed to me like the per-param r/w block functions were adding unnecessary restrictions, since we're not using a rwsem for each individual param. If the param lock is changed to a rwsem in the future, it won't be hard to also change all callers. Or, we could change it to a resem now. But as you said, kernel param locking isn't really a contended lock. > > 2) Implicit is bad: implying the module rather than the parameter is >weird Well implying it enforces only blocking your own module params; should module A be blocking and accessing module B's params? Isn't requiring each module to pass THIS_MODULE unnecessary, at least in the vast majority of cases? There is still __kernel_param_lock(module) that can be used if it really is necessary anywhere to block some other module's params. Or, I can change it to require passing THIS_MODULE if you think that's a better api. > , and skips the BUG_ON() check which was there before. those made absolutely no sense to me; why in the world is it necessary to BUG() simply because the param permissions don't match r or w? At *most* that deserves a WARN_ON() but more likely a pr_warn() is appropriate. And even then, who cares? Does it actually cause any harm? No. Especially since the underlying lock isn't a rwsem. But even if it *was* a rwsem, it still wouldn't hurt anything. Maybe the param's permissions change at runtime by the module for whatever reason. Should the module really check the current permissions before deciding to block the param or not? No, it's simpler and harmless to just block it unconditionally, and avoids a race with the permissions being changed again and the param accessed. And what if the user changes the permissions from sysfs? Certainly we don't want to BUG(), and even printing a WARN() or pr_warn() isn't really appropriate. In any case, IMHO, those permission checks aren't needed and shouldn't be done. > > And finally, why are you loading a module from a param callback? That's > a first! Yeah :) So zswap uses a crypto compressor. The crypto api allows selecting different compressor functions via crypto_alloc_comp(), and it will internally load any crypto module that's requested if it's missing (it also loads the module via crypto_has_comp()/crypto_has_alg()). So, when someone requests to change zswap's compressor function, zswap tries to create the crypto compressor transform, and the crypto api then does request_module() to load the new compressor. The configuration and module loading is done in the param callback so that zswap can fail the param setting if there is no such implementation available. > > Thanks, > Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] module: add per-module params lock
Dan Streetman writes: > I sent this as part of a patch series a few days ago, which I was asked to > break up, so I'm sending only this patch as a RFC now, until I work out > the details of the zswap patch that needs this. I'd like to get comments > on this early, since it changes the way module param locking is done. OK, it's not insane, but I'm not entirely convinced. 1) The difference between blocking sysfs for read vs write is mainly documentation. In theory, it allows a rwsem, though in practice it's not been a bottleneck to now. 2) Implicit is bad: implying the module rather than the parameter is weird, and skips the BUG_ON() check which was there before. And finally, why are you loading a module from a param callback? That's a first! Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC] module: add per-module params lock
I sent this as part of a patch series a few days ago, which I was asked to
break up, so I'm sending only this patch as a RFC now, until I work out
the details of the zswap patch that needs this. I'd like to get comments
on this early, since it changes the way module param locking is done.
...
Add a "param_lock" mutex to each module, and add a pointer to the owning
module to struct kernel_param. Then change the param sysfs modification
code to only use the global param_lock for built-ins, and use the
per-module param_lock for all modules. Also simplify the
kernel_[un]block_sysfs_[read|write]() functions, to simply
kernel_[un]block_sysfs().
The kernel param code currently uses a single global mutex to protect
modification of any and all kernel params. While this generally works,
there is one specific problem with it; a module callback function
cannot safely load another module, i.e. with request_module() or even
with indirect calls such as crypto_has_alg(). If the module to be
loaded has any of its params configured (e.g. with a /etc/modprobe.d/*
config file), then the attempt will result in a deadlock between the
first module param callback waiting for modprobe, and modprobe trying to
lock the global kernel param mutex to set the new module's param.
This fixes that by using per-module mutexes, so that each individual module
is protected against concurrent changes in its own kernel params, but is
not blocked by changes to other module params. All built-in modules
continue to use the global mutex, since they will always be loaded at
runtime and references (e.g. request_module(), crypto_has_alg()) to them
will never cause load-time param changing.
This also simplifies the interface used by modules to block sysfs access
to their params; while there are currently functions to block and unblock
sysfs param access which are split up by read and write and expect a single
kernel param to be passed, their actual operation is identical and applies
to all params, not just the one passed to them; they simply lock and unlock
the global param mutex. They are thus replaced with simplified functions
that clearly indicate that all params for a module are being blocked, both
for read and write access.
Signed-off-by: Dan Streetman
---
arch/um/drivers/hostaudio_kern.c | 20 +++
drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 6 +--
drivers/net/wireless/libertas_tf/if_usb.c| 6 +--
drivers/usb/atm/ueagle-atm.c | 4 +-
drivers/video/fbdev/vt8623fb.c | 4 +-
include/linux/module.h | 1 +
include/linux/moduleparam.h | 67 +---
kernel/module.c | 1 +
kernel/params.c | 45 ++--
net/mac80211/rate.c | 4 +-
10 files changed, 77 insertions(+), 81 deletions(-)
diff --git a/arch/um/drivers/hostaudio_kern.c b/arch/um/drivers/hostaudio_kern.c
index 9b90fdc..1bcb798 100644
--- a/arch/um/drivers/hostaudio_kern.c
+++ b/arch/um/drivers/hostaudio_kern.c
@@ -185,9 +185,9 @@ static int hostaudio_open(struct inode *inode, struct file
*file)
int ret;
#ifdef DEBUG
- kparam_block_sysfs_write(dsp);
+ kparam_block_sysfs();
printk(KERN_DEBUG "hostaudio: open called (host: %s)\n", dsp);
- kparam_unblock_sysfs_write(dsp);
+ kparam_unblock_sysfs();
#endif
state = kmalloc(sizeof(struct hostaudio_state), GFP_KERNEL);
@@ -199,11 +199,11 @@ static int hostaudio_open(struct inode *inode, struct
file *file)
if (file->f_mode & FMODE_WRITE)
w = 1;
- kparam_block_sysfs_write(dsp);
+ kparam_block_sysfs();
mutex_lock(&hostaudio_mutex);
ret = os_open_file(dsp, of_set_rw(OPENFLAGS(), r, w), 0);
mutex_unlock(&hostaudio_mutex);
- kparam_unblock_sysfs_write(dsp);
+ kparam_unblock_sysfs();
if (ret < 0) {
kfree(state);
@@ -260,17 +260,17 @@ static int hostmixer_open_mixdev(struct inode *inode,
struct file *file)
if (file->f_mode & FMODE_WRITE)
w = 1;
- kparam_block_sysfs_write(mixer);
+ kparam_block_sysfs();
mutex_lock(&hostaudio_mutex);
ret = os_open_file(mixer, of_set_rw(OPENFLAGS(), r, w), 0);
mutex_unlock(&hostaudio_mutex);
- kparam_unblock_sysfs_write(mixer);
+ kparam_unblock_sysfs();
if (ret < 0) {
- kparam_block_sysfs_write(dsp);
+ kparam_block_sysfs();
printk(KERN_ERR "hostaudio_open_mixdev failed to open '%s', "
"err = %d\n", dsp, -ret);
- kparam_unblock_sysfs_write(dsp);
+ kparam_unblock_sysfs();
kfree(state);
return ret;
}
@@ -326,10 +326,10 @@ MODULE_LICENSE("GPL");
static int __init hostaudio_init_module(void)
{
- __kernel
