Messages in this thread | | | From | Dan Streetman <> | Date | Tue, 9 Jun 2015 21:01:34 -0400 | Subject | Re: [RFC] module: add per-module params lock |
| |
On Mon, Jun 8, 2015 at 5:13 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: > Dan Streetman <ddstreet@ieee.org> writes: >> On Thu, Jun 4, 2015 at 8:42 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>> Dan Streetman <ddstreet@ieee.org> 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.
| |