Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932645AbbFJBCF (ORCPT ); Tue, 9 Jun 2015 21:02:05 -0400 Received: from mail-qk0-f173.google.com ([209.85.220.173]:34159 "EHLO mail-qk0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752723AbbFJBBz (ORCPT ); Tue, 9 Jun 2015 21:01:55 -0400 MIME-Version: 1.0 In-Reply-To: <87twuhewek.fsf@rustcorp.com.au> References: <1433422379-24418-1-git-send-email-ddstreet@ieee.org> <87mw0ft283.fsf@rustcorp.com.au> <87twuhewek.fsf@rustcorp.com.au> From: Dan Streetman Date: Tue, 9 Jun 2015 21:01:34 -0400 X-Google-Sender-Auth: 4r9Db9srQSyvPX7_yGvQ4U7-xaI 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: 3942 Lines: 78 On Mon, Jun 8, 2015 at 5:13 PM, Rusty Russell wrote: > Dan Streetman writes: >> 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. > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/