Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754241AbbFEKsX (ORCPT ); Fri, 5 Jun 2015 06:48:23 -0400 Received: from mail-qc0-f171.google.com ([209.85.216.171]:33372 "EHLO mail-qc0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753726AbbFEKsV (ORCPT ); Fri, 5 Jun 2015 06:48:21 -0400 MIME-Version: 1.0 In-Reply-To: <87mw0ft283.fsf@rustcorp.com.au> References: <1433422379-24418-1-git-send-email-ddstreet@ieee.org> <87mw0ft283.fsf@rustcorp.com.au> From: Dan Streetman Date: Fri, 5 Jun 2015 06:47:59 -0400 X-Google-Sender-Auth: T8JO7rssi-Kfe7fXl6ZfzReWEzY Message-ID: Subject: Re: [RFC] module: add per-module params lock To: Rusty Russell Cc: Jani Nikula , Greg Kroah-Hartman , "David S. Miller" , Christoph Hellwig , Andrew Morton , linux-kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3702 Lines: 78 On Thu, Jun 4, 2015 at 8:42 PM, Rusty Russell wrote: > Dan Streetman 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/