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 A41DBC76188 for ; Wed, 5 Apr 2023 19:46:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EE17F6B0071; Wed, 5 Apr 2023 15:46:04 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E90706B0074; Wed, 5 Apr 2023 15:46:04 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D7FA56B0075; Wed, 5 Apr 2023 15:46:04 -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 C88446B0071 for ; Wed, 5 Apr 2023 15:46:04 -0400 (EDT) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id A2DF91A048A for ; Wed, 5 Apr 2023 19:46:04 +0000 (UTC) X-FDA: 80648368248.20.B364FE9 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) by imf16.hostedemail.com (Postfix) with ESMTP id 59F8E180018 for ; Wed, 5 Apr 2023 19:46:02 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=infradead.org header.s=bombadil.20210309 header.b=tYXt24Fo; spf=none (imf16.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=1680723963; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=gRyDGkNXA8X/Ou+wCx5J0rIg1wyfF74j9vjMPvDUruY=; b=LoMNR7Oot9y1LcQz0ZRZvaxHJT48U4VG1BzevAuXjtrn6PmrxZRfiQMThumcutVAACtxVJ SsCLmJhoW/YOtsMCiO06mQyyQlo4z7jpQSBqYl/FnBRhLn+gWLkTLYDab3OG7fvyDLvLnc IMaRV4ciAzyIp+wPtU02mLjgLLH8E0g= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=infradead.org header.s=bombadil.20210309 header.b=tYXt24Fo; spf=none (imf16.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-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680723963; a=rsa-sha256; cv=none; b=2a+4IipAlouI+qn3v9wBy5560hDMlUNhY4pFCas953PGU7jLHx5m4+Gs7xNJVmfXH5jcj9 nxYhNIeEiszSFP8bY9b4LkcJLcKy0qaO558fAww52UVGy5KQ7fZRYBirvhxNsU48LBh8Bf K94YrBg1xje+3Hkp92juo2ME3Lxw0Ic= 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-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=gRyDGkNXA8X/Ou+wCx5J0rIg1wyfF74j9vjMPvDUruY=; b=tYXt24Fo50ltUGZjPjw2lPjPeF nzY6+MQSkeHE/QQynKGgV5VV5lcwL+HdN/7sVQPA/lOJowkAzVPwlrGZdP5RRyOlT9x5Or6Jud0RF rpjK/rHLoylED/BxIlOYor/5xjtmHNdH+Y92VoG8pEM6QJ410A2J6xQX2X5xzmPmm/sGx7GMD57/H fR/YNVwFPAg+dxxkHyps6vNJA6hvv+J2zaH3tcW+KrzQYlvD0Ai74boZq12pBdFycMCoY9rcMLD6N NZbanhKGgMn2inBPxugV6qcnRU6qemPwa/xBWnjEvce+TByP4Cq4QdDBhTfOmx8vzZ0Us5rav4nxk 3NLOOFdg==; Received: from mcgrof by bombadil.infradead.org with local (Exim 4.96 #2 (Red Hat Linux)) id 1pk94s-005Wvv-0J; Wed, 05 Apr 2023 19:45:54 +0000 Date: Wed, 5 Apr 2023 12:45:54 -0700 From: Luis Chamberlain To: David Hildenbrand Cc: patches@lists.linux.dev, linux-modules@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, pmladek@suse.com, petr.pavlu@suse.com, prarit@redhat.com, torvalds@linux-foundation.org, gregkh@linuxfoundation.org, rafael@kernel.org, christophe.leroy@csgroup.eu, tglx@linutronix.de, peterz@infradead.org, song@kernel.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 Subject: Re: [PATCH v2 3/6] module: extract patient module check into helper Message-ID: References: <20230405022702.753323-1-mcgrof@kernel.org> <20230405022702.753323-4-mcgrof@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 59F8E180018 X-Rspam-User: X-Stat-Signature: zb3grp8endmocsgmg44qw61ctejegcpo X-HE-Tag: 1680723962-859151 X-HE-Meta: U2FsdGVkX1/QrGPmuxDmLW9GraJqJDq5KIb9L/o5B0pAzPUS8d9EVmOFCx9AsJlmod+8YoGXPSo3bJ5M4t5aBC3h0GTSIztzKJ13D+3Wt7nM7d3L9z4OUAeIF3JI8MG0l8HfvhoghS+0qBYvHBoH/7CKNcxXcm/+ukWlKYLdNc/aucSjUNjTBNeC9znGLuTFUX2c0mE5MUIZ1vYB0A7knCoKvFtp/abnYZSVSOsiNsw5bE8iG2R871qS6k9SS35mjToWZdQtKZsodBZmnI+nyfXfSfb3ngJly2XqKVJ+KRrDoMiPEv9rvwU7MHzX/Y3vTvs0hnVh/+LOc8Yr4z7pnewWbbQdmkwqC3SOIX9KYKCiK/bzLU6qsUbppnAEaoKw2PSxIZNTnGo32AaozzMNLFW49UO2s+yZC9ZKnkt6M7kBW/OqECQFevki8PhnpUTvaThUk+WSmFKMKp5BVS6AteQUWqm0UbxE3Sfy3Mu9X8maROyjs+PxhSE9NpoXjQ5+JEDGcRO8xWtfH0QujxZteC65HSIrzOBs7/5XaUAKdR11Z7ct0X79T8tLLWkphXmOdaq9SJmCBbOLAdk9WRyaoGuo1eiqb7aRYurS/3W0bx8akDIrPppyxZIJ3rDQuftR6wO//b9EvYim3rOlA0cOASnmNrAaYgX+7Oyqgmx87Ik8iiOcUPrvWGzCAoIgklMG5GDbgDrGqRxJqxo5FAL5mO6Nprr1JNUSglu7INoDF1Ps0ngwYYozBEWqP5UM2yPut/cUrzswbCjF6OWKrmJlombWHomMg1QUlmt+pxGwWxOY9fEuvC07rnO0ZOSG/cuSGpjQqFS8CRthppmlafVEtD7yTx7o2gyOi++9BGM4cBVS4KHicaw3jnb7kZiKsrV9paqD8jZdfLGblp5bpZkGlpjK5I1fzR/7wDuOsOL6KDqEEnOVYByXriTB1gZRxIqdazXf/jcJIDDacKbhrMg GzNjyH/n PHoJYz1tFlK7L+m1JUkku3/5I7wrtjvIbE3Rr8YJBe8ZzgTQVcE9IeJ89QLUmkJN7TdVM63jEt7J3AKE4KKUsurGmLbCRFs7jkWPAHar7xZXTvtMn5Le+wj0dNrr2qjzm8bEHNMeBZl1PgeCvzRN6+E2l2HdNPA+VvFnJjX6R9s2WoO9Vc7Vy9HXmDEHWGEuVlbdkWqlTwFoMeCz+K3K8midUSHY/5r/raWGPTYwrpZLQhB1VylgFE6M2J2ZM3OrfuUWmspaGDt/NDos6kIMzXNeqLtmI9edgzsftGC8vaBUerKo7fEngb9bKwS3ZnstSxOFPdUy1zvSj6Vmlc07jvz3tJitZy4RO92zXdAXoIB4zrZtYeRjXfS+ly4OEs1zxmszyrCoGUFLZW4JZhY0LxIooFOfgjscNtDc7O+jqp72E03zjFdjpfUIugT4ityD0AAI+GFVfIT+zH14= 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, Apr 05, 2023 at 07:11:24PM +0200, David Hildenbrand wrote: > On 05.04.23 04:26, Luis Chamberlain wrote: > > The patient module check inside add_unformed_module() is large > > enough as we need it. It is a bit hard to read too, so just > > move it to a helper and do the inverse checks first to help > > shift the code and make it easier to read. The new helper then > > is module_patient_check_exists(). > > > > Signed-off-by: Luis Chamberlain > > --- > > kernel/module/main.c | 71 +++++++++++++++++++++++++------------------- > > 1 file changed, 40 insertions(+), 31 deletions(-) > > > > diff --git a/kernel/module/main.c b/kernel/module/main.c > > index 98c261928325..8f382580195b 100644 > > --- a/kernel/module/main.c > > +++ b/kernel/module/main.c > > @@ -2638,6 +2638,43 @@ static bool finished_loading(const char *name) > > return ret; > > } > > +/* Must be called with module_mutex held */ > > +static int module_patient_check_exists(const char *name) > > +{ > > + struct module *old; > > + int err = 0; > > + > > + old = find_module_all(name, strlen(name), true); > > + if (old == NULL) > > + return 0; > > + > > + if (old->state == MODULE_STATE_COMING > > + || old->state == MODULE_STATE_UNFORMED) { > > I never understood why people prefer to prefix the || on a newline. But it > seems to be a thing in the module/ world :) Yeah the other way seems better, I'll make it pretty. > > + /* Wait in case it fails to load. */ > > + mutex_unlock(&module_mutex); > > + err = wait_event_interruptible(module_wq, > > + finished_loading(name)); > > + if (err) > > + return err; > > You return with the mutex unlocked. The caller will unlock again ... Fixed now by moving the mutex below up after the wait_event_interruptible(), thanks. > > + > > + /* The module might have gone in the meantime. */ > > + mutex_lock(&module_mutex); > > + old = find_module_all(name, strlen(name), true); > > + } > > + > > + /* > > + * We are here only when the same module was being loaded. Do > > + * not try to load it again right now. It prevents long delays > > + * caused by serialized module load failures. It might happen > > + * when more devices of the same type trigger load of > > + * a particular module. > > + */ > > + if (old && old->state == MODULE_STATE_LIVE) > > + return -EEXIST; > > + else > > + return -EBUSY; > > You can drop the else and return right away. Will make it pretty like that, thanks. Luis