On 1/18/23 01:04, Luis Chamberlain wrote:
On Tue, Dec 13, 2022 at 11:17:42AM +0100, Petr Mladek wrote:
On Mon 2022-12-12 21:09:19, Luis Chamberlain wrote:
- *Fixing* a kernel regression by adding new expected API for testing
against -EBUSY seems not ideal.
IMHO, the right solution is to fix the subsystems so that they send only one uevent.
Makes sense, but that can take time and some folks are stuck on old kernels and perhaps porting fixes for this on subsystems may take time to land to some enterprisy kernels. And then there is also systemd that issues the requests too, at least that was reflected in commit 6e6de3dee51a ("kernel/module.c: Only return -EEXIST for modules that have finished loading") that commit claims it was systemd issueing the requests which I mean to interpret finit_module(), not calling modprobe.
The rationale for making a regression fix with a new userspace return value is fair given the old fix made things even much worse the point some kernel boots would fail. So the rationale to suggest we *must* short-cut parallel loads as effectively as possible seems sensible *iff* that could not make things worse too but sadly I've found an isssue proactively with this fix, or at least that this issue is also not fixed:
./tools/testing/selftests/kmod/kmod.sh -t 0006 Tue Jan 17 23:18:13 UTC 2023 Running test: kmod_test_0006 - run #0 kmod_test_0006: OK! - loading kmod test kmod_test_0006: FAIL, test expects SUCCESS (0) - got -EINVAL (-22)
Custom trigger configuration for: test_kmod0 Number of threads: 50 Test_case: TEST_KMOD_FS_TYPE (2) driver: test_module fs: xfs
Test completed
When can multiple get_fs_type() calls be issued on a system? When mounting a large number of filesystems. Sadly though this issue seems to have gone unnoticed for a while now. Even reverting commit 6e6de3dee51a doesn't fix it, and I've run into issues with trying to bisect, first due to missing Kees' patch which fixes a compiler failure on older kernel [0] and now I'm seeing this while trying to build v5.1:
ld: arch/x86/boot/compressed/pgtable_64.o:(.bss+0x0): multiple definition of `__force_order'; arch/x86/boot/compressed/kaslr_64.o:(.bss+0x0): first defined here ld: warning: arch/x86/boot/compressed/efi_thunk_64.o: missing .note.GNU-stack section implies executable stack ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker ld: arch/x86/boot/compressed/head_64.o: warning: relocation in read-only section `.head.text' ld: warning: arch/x86/boot/compressed/vmlinux has a LOAD segment with RWX permissions ld: warning: creating DT_TEXTREL in a PIE make[2]: *** [arch/x86/boot/compressed/Makefile:118: arch/x86/boot/compressed/vmlinux] Error 1 make[1]: *** [arch/x86/boot/Makefile:112: arch/x86/boot/compressed/vmlinux] Error 2 make: *** [arch/x86/Makefile:283: bzImage] Error 2
[0] http://lore.kernel.org/lkml/20220213182443.4037039-1-keescook@chromium.org
But we should try to bisect to see what cauased the above kmod test 0006 to start failing.
It is not clear to me from your description if the observed failure of kmod_test_0006 is related to the fix in this thread.
The problem was not possible for me to reproduce on my system. My test was on an 8-CPU x86_64 machine using v6.2-rc4 with "defconfig + kvm_guest.config + tools/testing/selftests/kmod/config".
Could you perhaps trace the test to determine where the EINVAL value comes from?
The question is how the module loader would deal with "broken" subsystems. Petr Pavlu, please, fixme. I think that there are more subsystems doing this ugly thing.
I personally thing that returning -EBUSY is better than serializing all the loads. It makes eventual problem easier to reproduce and fix.
I agree with this assessment, however given the multiple get_fs_type() calls as an example, I am not sure if there are other areas which rely on the old busy-wait mechanism.
*If* we knew this issue was not so common I'd go so far as to say we should pr_warn_once() on failure, but at this point in time I think it'd be pretty chatty.
I don't yet have confidence that the new fast track to -EXIST or -EBUSY may not create regressions, so the below I think would be too chatty. If it wasn't true, I'd say we should keep record of these uses so we fix the callers.
A similar fast track logic was present prior to 6e6de3dee51a. The fix in this thread doesn't bring a completely new behavior but rather restores the previous one. The fix should have only two differences: a window when parallel loads are detected is extended, the return code is -EBUSY instead of -EEXIST.
diff --git a/kernel/module/main.c b/kernel/module/main.c index d3be89de706d..d1ad0b510cb8 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2589,13 +2589,6 @@ static int add_unformed_module(struct module *mod) 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.*/@@ -2610,6 +2603,15 @@ static int add_unformed_module(struct module *mod) out: mutex_unlock(&module_mutex); out_unlocked:
- /*
* We get an error here only when there is an attempt to load the* same module. Subsystems should strive to only issue one request* for a needed module. Multiple requests might happen when more devices* of the same type trigger load of a particular module.*/- if (err)
pr_warn_once("%: dropping duplicate module request, err: %d\n",mod->name, err);}
I'm not sure if this would be the right thing to do.
What I think would be good to fix is the pattern utilized by some cpufreq (and edac) modules. It is a combination of them being loaded per-CPU and using a cooperative pattern to allow only one module of each such type on the system. They can be viewed more as a whole-platform drivers that should be attempted to be loaded only once. It is still on my todo list to post a patch to cpufreq maintainers to start a discussion on this.
Another consideration on the kernel side could be to try grouping other currently per-CPU loaded modules.
However, in general, a system can have many hardware pieces of the same type and it looks correct to me that each is individually exposed in sysfs and via uevents with their set of needed modules.
I would say that if this turns out to be a further issue in practice, udevd could optimize its process and avoid same-module loads when it is bringing up to life multiple devices.
Thanks, Petr