Re: [PATCH 2/2] module: add support to avoid duplicates early on load

[Lucas De Marchi]

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@xxxxxxxxxx> 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