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
