Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 30 May 2023 22:30:11 -0700
From:   Lucas De Marchi <lucas.demarchi@intel.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
CC:     Luis Chamberlain <mcgrof@kernel.org>,
        Johan Hovold <johan@kernel.org>,
        Petr Pavlu <petr.pavlu@suse.com>, <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>, <david@redhat.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: <atfuud5a4zcrs5s6rh7t7eauuofhtwczvzd7r5tsg3ramuqrfq@sxbvdnhaaxgr>
References: <CAHk-=whiXzqprmQNRui3LbKQwvM8fg4nyAzWcU5qZs+kxBVzrA@mail.gmail.com>
 <ZHRpH-JXAxA6DnzR@hovoldconsulting.com>
 <CAHk-=wh6sXSO63kka+EWEqq0tGwtOnXYFWMXPQ6T_wZa+Np3MQ@mail.gmail.com>
 <ZHSeOUpKtyc8VKx5@hovoldconsulting.com>
 <ZHTCK2_1pF61yWIr@hovoldconsulting.com>
 <CAHk-=wg7ihygotpO9x5a6QJO5oAom9o91==L_Kx-gUHvRYuXiQ@mail.gmail.com>
 <ZHYitt7P7W+8ZlSB@bombadil.infradead.org>
 <bav73qu5khl2dxkwpvy2hq34lyi56jn6yp7h5qlennka65kre3@zwwmrr55d2by>
 <ZHZRaFWvLEvkoCMA@bombadil.infradead.org>
 <CAHk-=wicgDftP9ogSagxiRNvVTm7+YfQpEBuEsoRbkWzsw=EZw@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAHk-=wicgDftP9ogSagxiRNvVTm7+YfQpEBuEsoRbkWzsw=EZw@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bEFkeGUxRkFmYzVDMytWaGFLdmJRdmZOb2ZicVdYTmNFZzg3bTJreDBKOUJE?=
 =?utf-8?B?dXpxNy9zVm5PbENEQTVIalJRZmFXMExkR21LYnNkTFlJd2tMSnBuMEc4L1Ry?=
 =?utf-8?B?V1lMWWdpZUVCSmJZYlBObzY1a0Y2bFpKOU5sd3BENDh4bm5ESTQ1VmE4dlFx?=
 =?utf-8?B?UHZ3czJRY1BqNjZXUzVBVmN6TTJvZjJuL0xYQUlMZmVnUFdSb0RCYWZhbUtK?=
 =?utf-8?B?MTRocG92N2xtYlMvTE9ER01sbm9RTmRSRVZHdE93U1FjZTBrNm42UjN1L0s2?=
 =?utf-8?B?U1lMbzBDcWl6V0kwSHRZbHBJSlFQM3JGd1B6dVZQV0t6dTBLL0VMSlFNN3lZ?=
 =?utf-8?B?MlpWdVNVY2FRN3BmTllyQVlZK05sWWhhRHNtNnpvZHA1TzVIUDdSdlFLeGhN?=
 =?utf-8?B?Y0Fvb0o3RXdRdjB2UmVDbGp0WTllLzYyM1gvUHJFWnRBcjNRWFU4OVFuRWtw?=
 =?utf-8?B?ckFrWjF2cmNmZnI3d1hRZnpZM2pEQW41aW91eHNWVjFlVlY5bGxsQUdVT0dC?=
 =?utf-8?B?Y2tidUxYNlJzZHJMSUVXSHkrb0FxRkk1M242UWxwMVBmU0VxdnVOMEN6MUF4?=
 =?utf-8?B?aFZ0dm9pd0VQNGpCL1VrUkl6cHdmT0NUeWRiQzAzRmpzaVdqTGpZWmY2ekd4?=
 =?utf-8?B?NUFjMjJFTGYzdzh6R1FUdlF2dXdKcUFROHZaZ3NENDI5WEY0RGFmZnY4OVQx?=
 =?utf-8?B?UmRySzJEZTJTVnJ3WUs4UWlEREphYVhwa044K2l6QmtMM0p2M1lFRUxhWGFv?=
 =?utf-8?B?aCtsUFY2Z2dsM1JrWi9LOHBFRTRWRlY5RkNnRVVoYVRSeEMxUGVEK2Y1Uk1S?=
 =?utf-8?B?b3VHbEFyZjZWcTZZMjB5NEMwSlBhRVNVbm5XOWZ6UFJnRFlzKzIxT1BpSGx6?=
 =?utf-8?B?SzArY0NnY1ZkZmlxK2xUeHdRUWNxOGV5V1VNeUhtR01jbVk4UHc1d0NSQUM0?=
 =?utf-8?B?OGJYWUJHc0RkMVpBaU9XVHNtWWNPRVhBYTZYUnl0UVBMMEQ0eUFMc1lCcHRt?=
 =?utf-8?B?RmZMckdkaCtvRFUwVUN1QjZYUVRnV2t4OTdZZWlPLy9mWm5zcU84QzZXdysy?=
 =?utf-8?B?dUtqRyttcFNtUFNtZXZDd3JvWmYvSFQ1NzM4blRwMEV1ZjBXNHVMOStvL25o?=
 =?utf-8?B?YkRnTy9lRXNQc2c3K0ZFbVdwdmlibmpGM2NwdTVrR1FDbVZPVXpsNHM1Qng4?=
 =?utf-8?B?U2JaTjFkVzJsZEMzZFFYMUxBZGtvOHZxdURPdEZhdzhuckdWUEEvaUd5OEdj?=
 =?utf-8?B?aGFJQnRaUmdZNkJJQWwzVEhEVGU0M0Zka3d3YTdlclVYSGIyaTAzdUo4NHo0?=
 =?utf-8?B?U0VJck8wQWMvK29CejVkL2tlSTA2M0E3bXhTWUw1WHNJSGZ5dmZCYTNIZFE2?=
 =?utf-8?B?WUtOZURzQVYzSk0rd1dJZzFGRGo5M2xmdE5iNmJzRHZSNjhDUzZhK0ZSbWVu?=
 =?utf-8?B?OGFjVXkvZzBvbW1sWm5tdjlrMDBzWk1CQTF2eVVNeVM0VmcrZjY4djZvYWRY?=
 =?utf-8?B?RjIvTXJCY25XVEhIVmZQaEZDRnFFT25GNzFTUHhkOXEzOEVSVnZxUUloWU1Q?=
 =?utf-8?B?dk9nWC94ZTBxdk8vYXhqa1hVYnh1ckFvTGo0ejJJV29xQ29MVE14MFkzUmxo?=
 =?utf-8?B?OEdib3hoNWVlZ0MxMEdZbWJ1Q01YTHhjZWhLajBIVWpETU52cnBrekpTeXpy?=
 =?utf-8?B?RUdwSlZxSFEvN2EwVVB3K0QvaFN5dGg0a0RXWHE5VXFzMVBkZDVyQUp5eDly?=
 =?utf-8?B?ZXdQS2dPODVoTnpmMGg5V01FN2xua0lkSlhER2ZYYS9kZ2E2akFLUjYxa1Z0?=
 =?utf-8?B?Q3Y2VjBkQmIvZE9NdFVDQ3J0bTZSYXozVkVmSTU3TzQvaHM4N2RGM05obFZF?=
 =?utf-8?B?cDdQUFJYMmp6bnp1VHRKWVZManBSYXpsd3dYNDlpejNKaFNFVnlVeGxxSHZF?=
 =?utf-8?B?N3pQSEQ2dmU2YzNjbWVqWW1ZVzJzVGlicTBvTmY3cm9KSTE3enE3cyt2VVlp?=
 =?utf-8?B?RG52TzZqazdDaHFDaUVBQVVMdVRJMUY1ZkZkZFpIMDdUR2FjM2k5M0E1WWwx?=
 =?utf-8?B?WldQdmZ0T2FhbDBQZis1VEVnZ1ZXVjhTYk9BT1o5VGJlOFFxZFJxbCsxa3Iz?=
 =?utf-8?B?R2p4S21VS0pES2tvaWxscmhXMEx1TC9jaWJwclAvL1pGTnczOTZkMTd4NnN3?=
 =?utf-8?B?UVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 89ee4cd6-6ea8-4caf-e415-08db61981d56
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6139.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2023 05:30:16.1069
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: z06qJFqqRJ2bHGmGY2i/EWFxxBiYfSIKGXi+kjnfoGZYkPE2Ffk8ZHeBpx/iAoRyQ/Qnf7yOL8yioFAXE0e6ZVyoJmCChnicUar9jn1oiPg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6174
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, May 30, 2023 at 06:17:11PM -0400, Linus Torvalds wrote:
>On Tue, May 30, 2023 at 3:41 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>>
>> OK thanks! So just to confirm, it seems fine to return the same error
>> code if duplicates wait, or do you prefer for some reason for there to
>> be an exception and return -EEXIST if the module did succeed in the
>> duplicate case?
>
>I think either should be fine, since either was possible before.
>
>By definition, these are module loads being done in parallel, and so
>any of them "could" have been the first, and returned success before.
>
>And by extension, any of them could have been not first, and returned
>-EEXIST if somebody else loaded the same module first.
>
>So that "somebody else did a load" code:
>
>        if (idempotent(&idem, file_inode(f))) {
>                wait_for_completion(&idem.complete);
>                return idem.ret;
>        }
>
>could certainly have made the return value be something like
>
>        return idem.ret ? : -EEXIST;

yes, this is what I had in mind.

>
>instead of that "return idem.ret".
>
>But it does seem simpler - and more in line with the conceptual
>"loading the same module is an idempotent operation" of the patch -
>to just always return the success value to all of them.
>
>After all, they all did in some sense succeed to get that module
>loaded, even if it was a communal effort, and some threads did more
>than others...
>
>As mentioned, I don't think it can matter either way, since any of the
>callers might as well have been the successful one, and they would
>basically have to act the same way regardless (ie "somebody else
>succeeded" and "you succeeded" are basically equivalent return

agreed, it will just be a slightly different behavior if finit_module()
is called twice and the first call is already in the process of
initializing the module, i.e. complete_formation() was already called,
putting the module in the MODULE_STATE_COMING state, as per
kernel/module/main.c:add_unformed_module():

	/*
	 * 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)
		err = -EEXIST;
	else
		err = -EBUSY;
	goto out;

in userspace we already deal with that in a special way and should be
compatible with returning 0 for all practical purposes.

thanks
Lucas De Marchi

>values). If the module was a prerequisite for another module being
>loaded, either -EEXIST or 0 _is_ a success case.
>
>             Linus