Re: [RFC] module: add per-module params lock

[Dan Streetman]

On Mon, Jun 8, 2015 at 5:13 PM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
> Dan Streetman <ddstreet@xxxxxxxx> writes:
>> On Thu, Jun 4, 2015 at 8:42 PM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
>>> Dan Streetman <ddstreet@xxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/