From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B6C3AC77B73 for ; Wed, 31 May 2023 16:57:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 02E3A8E0002; Wed, 31 May 2023 12:57:47 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F20588E0001; Wed, 31 May 2023 12:57:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DE7E58E0002; Wed, 31 May 2023 12:57:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id CF09B8E0001 for ; Wed, 31 May 2023 12:57:46 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 88CF71401D5 for ; Wed, 31 May 2023 16:57:46 +0000 (UTC) X-FDA: 80851156932.26.A1D8B5F Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) by imf08.hostedemail.com (Postfix) with ESMTP id D8D39160011 for ; Wed, 31 May 2023 16:57:42 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=infradead.org header.s=bombadil.20210309 header.b=tc2HCj6C; spf=none (imf08.hostedemail.com: domain of mcgrof@infradead.org has no SPF policy when checking 198.137.202.133) smtp.mailfrom=mcgrof@infradead.org; dmarc=fail reason="No valid SPF, DKIM not aligned (relaxed)" header.from=kernel.org (policy=none) ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1685552264; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=sQf6qkA4MEbn8EichAXNjYGf8MDrQBBNv2l2z1zQPA0=; b=p1YvFQtZWT74rNW6+BjutkHSiZYzGRacJDs6NRk50ppZl+j/QAV03yQgYlLG9yp47XwweS 73wbVV5B6AjLFxf8ZEucSfyLireZtaJyPe05bHVSoaQmz/4CQAoXPmvVMj818qFTd/PDzm ji/LXbhLhH0mAFPfClOYy7xdgb/BB6s= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1685552264; a=rsa-sha256; cv=none; b=zF8kVDGbaONSyWWYi4ld8SPBQqwjWpLIjV54koicAXzrlvU8Pr6ySNkeDcDVhrK312T2PJ u2PnYAeg0AGLb0foN8dkZhNEqIP4tz2DhefxMq6+NDmnZObS6Pn9a3L8G1I0RyWAiwbHUc LM1jrbrHx1aTAypmif8YtX39B3qXJNg= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=infradead.org header.s=bombadil.20210309 header.b=tc2HCj6C; spf=none (imf08.hostedemail.com: domain of mcgrof@infradead.org has no SPF policy when checking 198.137.202.133) smtp.mailfrom=mcgrof@infradead.org; dmarc=fail reason="No valid SPF, DKIM not aligned (relaxed)" header.from=kernel.org (policy=none) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Sender:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description; bh=sQf6qkA4MEbn8EichAXNjYGf8MDrQBBNv2l2z1zQPA0=; b=tc2HCj6Cn1LdEc84oXW9SaHEtv 3siyUr4dMxMBLVDobevnbEEiCz21oQMPxV9VZh6Q9LNtEMcFBYS0/98cfaEVHv8thxxgjG1cmFn4t AcjBb3Ci7Pgy5/CA0pGmkxdp0M6zIjPEWocIqCFxop9tJ1i9uOZAB9ZjTQk5fpLmBNM5NoLqG66ee qQ8q8/a6Rfm3a/iqnYo9R71yfBAtth+4fOyhrcuBFOBpMTxkZC20xc2lkU7XBk0In0NvoBlgaJeRQ 8G8wiV1+qXn6rNrksuo1Zi5gsUGzMKxkcYxW2iYrtCLBf78sPNkwEC7a8wXP0dUfjh9zrHddk0v1y UYFFygGg==; Received: from mcgrof by bombadil.infradead.org with local (Exim 4.96 #2 (Red Hat Linux)) id 1q4P8O-000XDH-2D; Wed, 31 May 2023 16:57:16 +0000 Date: Wed, 31 May 2023 09:57:16 -0700 From: Luis Chamberlain To: David Hildenbrand Cc: Linus Torvalds , Johan Hovold , Lucas De Marchi , Petr Pavlu , gregkh@linuxfoundation.org, rafael@kernel.org, song@kernel.org, lucas.de.marchi@gmail.com, christophe.leroy@csgroup.eu, peterz@infradead.org, rppt@kernel.org, dave@stgolabs.net, willy@infradead.org, vbabka@suse.cz, mhocko@suse.com, dave.hansen@linux.intel.com, colin.i.king@gmail.com, jim.cromie@gmail.com, catalin.marinas@arm.com, jbaron@akamai.com, rick.p.edgecombe@intel.com, yujie.liu@intel.com, tglx@linutronix.de, hch@lst.de, patches@lists.linux.dev, linux-modules@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, pmladek@suse.com, prarit@redhat.com, lennart@poettering.net Subject: Re: [PATCH 2/2] module: add support to avoid duplicates early on load Message-ID: References: <499e30cc-d015-8353-1364-50d17da58f47@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <499e30cc-d015-8353-1364-50d17da58f47@redhat.com> X-Rspamd-Queue-Id: D8D39160011 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: 3s7gw6nuxwf5nahcfq8oy5g43uyr3qi1 X-HE-Tag: 1685552262-832000 X-HE-Meta: U2FsdGVkX18LNtlUuBrAi1D53ftdyEwytP85moAGfF4iivRJO5aP4uZafrh0N6ZKdcqGSoP3T+b3vpEVze4QphlqyaBPDysbtvQo1jKcL5kuayG6XCnPO25vmSneYMocpa7YtQdlJ4aKx8zLDLmRtOnaw2QJrYBI43Bo66jAjn2ZIdDtGkPkBitxmT5n0zzqc/I8tA1MWrG+i0lhs1Egc/12/H+/y3ZfAXO52usMdeg2HK4eBuNYV1KYAqwisyRGK3iiMRWahvA0Vt7wcQJdHinpy9Rar3+WoFMY2zag19Z1jI3jTMQieSZCwHO3XLHZ1SByYomb31LGp3ZHUZ1UBobKzMYCk24EDrPwn9euWkrtSSs9eBZSkjQjnr3A7Zl9ugjEeAm40qm4a9+zK4vrS3yiLQeqb2FhES2oK7qN7IOti+U08yqZ1MONjotHHTdwu5JY5lGfW0cbAYzUxjmRLhjQKRB/RjsOH4I4ng36RnFn78xnFVwV0RWEXlCFI8LzuW6UyuSyrA3eBuer2KjlzeLF3hS21Vvqh6iFWNBUIixtnV+Qi53TaYiw9ELwcCDXMblq9uHm/ua9myHOuJWXtdOVl7P6mfmnQ5LXP4AJFWOTE3P+mPMaUkfeMzZsYs6bPRaFL6C5nu3T+Y1XgBD1Sq4efzJrwbk0lDyZEiew8T9pxSYscxLLqjhkIrI/VTj8IrsMHv60b0Cet5nIdkCfvKU2JRJ6lShz87bFSuu9VpWoGP2DbcJTqItylAnrtMA5x13xWCqLbEihhse1rac2mErPYX89o3cCuZUKeqHP2hufWRku1me/EMqGBcn1TSGl8X85oeMbce0tVbx6m4k6C0Zv9SS2m3PsnGjL12tE7K9iK6vE/JC+U+o3ZoDom/U0pfjFwHX2ny9/27gHg5GQU3VGbHC16h5/RBD42vLQjYcUvMEuFzPk+TLWsa/pVEHeDatwLofXL2T89cuCOm2 BLQktLM5 dSafUhyTHkySUuTVQecgf8TvLR/1MMEQtoxd1ydYLW2KBs4TjkIBPwVFgQ6ayzeAtdU3jhasDYgQWb9esmwoGeI29M+dHPyQKIsPWaXnCkIAbjJ7QzdL8a22mU+YjbpZG/QgIjEmtIBVnznTKQprmbdYN/HffV48u3zyfayo+G/rseQgsc6qCS8EJXFMs7jdeWtLpV2e/dtfhWZV9dshNx1rxIgr+B9K9rEHqRNjzUNkd3Zddy/V4eSEl7m9MqrOGJ4cbpnfhrveo97gQ/XomszZAV8ddgWl81yYcTxHvlik2UH/6BePQMS1vWaoNcJqB7CoVyyOPkPJecQh6scjT7fB7a8InZ1V2N2dtQTfTm061khLExVy7k8IVE8JE5ZMjTaVZlAXWj7wjlVImr8vCWMkCfYeNbHYFUCjBOPE7Lo3uAR8QyPwkZrGbtoKJkpyhthKZZ1D7rOWBLUE= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, May 31, 2023 at 09:51:41AM +0200, David Hildenbrand wrote: > On 30.05.23 18:22, Luis Chamberlain wrote: > > On Mon, May 29, 2023 at 09:55:15PM -0400, Linus Torvalds wrote: > > > On Mon, May 29, 2023 at 11:18 AM Johan Hovold wrote: > > > > > > > > I took a closer look at some of the modules that failed to load and > > > > noticed a pattern in that they have dependencies that are needed by more > > > > than one device. > > > > > > Ok, this is a "maybe something like this" RFC series of two patches - > > > one trivial one to re-organize things a bit so that we can then do the > > > real one which uses a filter based on the inode pointer to return an > > > "idempotent return value" for module loads that share the same inode. > > > > > > It's entirely untested, and since I'm on the road I'm going to not > > > really be able to test it. It compiles for me, and the code looks > > > fairly straightforward, but it's probably buggy. > > > > > > It's very loosely based on Luis' attempt, but it > > > (a) is internal to module loading > > > (b) uses a reliable cookie > > > (c) doesn't leave the cookie around randomly for later > > > (d) has seen absolutely no testing > > > > > > Put another way: if somebody wants to play with this, please treat it > > > as a starting point, not the final thing. You might need to debug > > > things, and fix silly mistakes. > > > > > > The idea is to just have a simple hash list of currently executing > > > module loads, protected by a trivial spinlock. Every module loader > > > adds itself to the right hash list, and if they were the *first* one > > > (ie no other pending module loads for that inode), will actually do > > > the module load. > > > > > > Everybody who *isn't* the first one will just wait for completion and > > > return the same error code that the first one returned. > > > > That's also a hell much more snazzier MODULE_DEBUG_AUTOLOAD_DUPS if we > > ever wanted to do something similar there if we wanted to also > > join request_module() calls, instead of it hiding under debug. > > > > > This is technically bogus. The first one might fail due to arguments. > > > > For boot it's fine, as I can't think of boot wanting to support trying > > to load a module with different arguments but who knows. But I can't > > see it sensible to issue concurrent multiple requests for modules > > with different arguments without waiting in userspace for the first > > to fail. > > > > Even post-boot, doing that sounds rather insane, but it would certainly > > be a compromise and should probably be clearly documented. I think just > > a comment acknolwedging that corner case seems sensible. > > > > Because we won't be able to get the arguments until we process the > > module, so it would be too late for this optimization on kread. So it is > > why I had also stuck to the original feature being in kread, as then it > > provides a uniq kread call and the caller is aware of it. But indeed I > > had not considered the effects of arguments. > > > > Lucas, any thoughts from modules kmod userspace perspective into > > supporting anyone likely issuing concurrent modules requests with > > differing arguments? > > > > > So the cookie shouldn't be just the inode, it should be the inode and > > > a hash of the arguments or something like that. > > > > Personally I think it's a fine optimization without the arguments. > > > > > But it is what it is, > > > and apart from possible show-stopper bugs this is no worse than the > > > failed "exclusive write deny" attempt. IOW - maybe worth trying? > > > > The only thing I can think of is allowing threads other than the > > first one to complete before the one that actually loaded the > > module. I thought about this race for module auto-loading, see > > the comment in kmod_dup_request_announce(), so that just > > further delays the completion to other thread with a stupid > > queue_work(). That seems more important for module auto-loading > > duplicates than for boot finit_module() duplicates. But not sure > > if odering matters in the end due to a preemtible kernel and maybe > > that concern is hysteria. > > > > > And if *that* didn't sell people on this patch series, I don't know > > > what will. I should be in marketing! Two drink minimums, here I come! > > > > Sold: > > > > on 255 vcpus 0 duplicates found with this setup: > > > > root@kmod ~ # cat /sys/kernel/debug/modules/stats > > Mods ever loaded 66 > > Mods failed on kread 0 > > Mods failed on decompress 0 > > Mods failed on becoming 0 > > Mods failed on load 0 > > Total module size 11268096 > > Total mod text size 4149248 > > Failed kread bytes 0 > > Failed decompress bytes 0 > > Failed becoming bytes 0 > > Failed kmod bytes 0 > > Virtual mem wasted bytes 0 > > Average mod size 170729 > > Average mod text size 62868 > > > > So: > > > > Tested-by: Luis Chamberlain > > > > In terms of bootup timing: > > > > Before: > > Startup finished in 41.653s (kernel) + 44.305s (userspace) = 1min 25.958s > > graphical.target reached after 44.178s in userspace. > > After: > > Startup finished in 23.995s (kernel) + 40.350s (userspace) = 1min 4.345s > > graphical.target reached after 40.226s in userspace. > > I'll try grabbing the system where we saw the KASAN-related issues [1] and > give it a churn with and without the two patches. Might take a bit (~1 day), > unfortunately. > > [1] https://lkml.kernel.org/r/20221013180518.217405-1-david@redhat.com Great, don't forget: diff --git a/kernel/module/main.c b/kernel/module/main.c index 82b0dcc1fe77..222015093eeb 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -3066,7 +3066,7 @@ struct idempotent { #define IDEM_HASH_BITS 8 static struct hlist_head idem_hash[1 << IDEM_HASH_BITS]; -static struct spinlock idem_lock; +static DEFINE_SPINLOCK(idem_lock); static bool idempotent(struct idempotent *u, const void *cookie) {