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

[Rusty Russell]

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.

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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/