lkml.org 
[lkml]   [2015]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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.


\
 
 \ /
  Last update: 2015-06-10 03:41    [W:0.077 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site