Messages in this thread | | | Date | Mon, 14 Nov 2022 09:57:56 +0100 | Subject | Re: [PATCH v2 2/2] module: Merge same-name module load requests | From | David Hildenbrand <> |
| |
On 12.11.22 02:47, Luis Chamberlain wrote: > On Wed, Oct 19, 2022 at 02:00:55PM +0200, Petr Pavlu wrote: >> On 10/18/22 20:33, Luis Chamberlain wrote: >>> On Sat, Oct 15, 2022 at 11:27:10AM +0200, Petr Pavlu wrote: >>>> The patch does address a regression observed after commit 6e6de3dee51a >>>> ("kernel/module.c: Only return -EEXIST for modules that have finished >>>> loading"). I guess it can have a Fixes tag added to the patch. >>>> >>>> I think it is hard to split this patch into parts because the implemented >>>> "optimization" is the fix. >>> >>> git describe --contains 6e6de3dee51a >>> v5.3-rc1~38^2~6 >>> >>> I'm a bit torn about this situation. Reverting 6e6de3dee51a would be the >>> right thing to do, but without it, it still leaves the issue reported >>> by Prarit Bhargava. We need a way to resolve the issue on stable and >>> then your optimizations can be applied on top. >> >> Simpler could be to do the following: >> >> diff --git a/kernel/module/main.c b/kernel/module/main.c >> index d02d39c7174e..0302ac387e93 100644 >> --- a/kernel/module/main.c >> +++ b/kernel/module/main.c >> @@ -2386,7 +2386,8 @@ static bool finished_loading(const char *name) >> sched_annotate_sleep(); >> mutex_lock(&module_mutex); >> mod = find_module_all(name, strlen(name), true); >> - ret = !mod || mod->state == MODULE_STATE_LIVE; >> + ret = !mod || mod->state == MODULE_STATE_LIVE >> + || mod->state == MODULE_STATE_GOING; >> mutex_unlock(&module_mutex); >> >> return ret; >> @@ -2566,7 +2567,8 @@ static int add_unformed_module(struct module *mod) >> mutex_lock(&module_mutex); >> old = find_module_all(mod->name, strlen(mod->name), true); >> if (old != NULL) { >> - if (old->state != MODULE_STATE_LIVE) { >> + if (old->state == MODULE_STATE_COMING >> + || old->state == MODULE_STATE_UNFORMED) { >> /* Wait in case it fails to load. */ >> mutex_unlock(&module_mutex); >> err = wait_event_interruptible(module_wq, >> @@ -2575,7 +2577,7 @@ static int add_unformed_module(struct module *mod) >> goto out_unlocked; >> goto again; >> } >> - err = -EEXIST; >> + err = old->state != MODULE_STATE_LIVE ? -EBUSY : -EEXIST; >> goto out; >> } >> mod_update_bounds(mod); > > > Prarit, can you verify this still does not break the issue you reported? > David, does this also fix your issue?
I didn't try, but from a quick glimpse I assume no. Allocating module space happens before handling eventual duplicates right now, before a module even is "alive" and in the MODULE_STATE_UNFORMED state.
But maybe I am missing something important.
-- Thanks,
David / dhildenb
| |