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

[Dan Streetman]

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.

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