Messages in this thread Patch in this message | | | Date | Thu, 13 Apr 2023 11:44:51 +0200 (CEST) | From | Miroslav Benes <> | Subject | Re: [PATCH v2 2/2] modules/kmod: replace implementation with a sempahore |
| |
Hi,
On Wed, 5 Apr 2023, Luis Chamberlain wrote:
s/sempahore/semaphore/ in the subject
> Simplfy the concurrency delimiter we user for kmod with the semaphore.
"Simplify the concurrency delimiter we use for kmod with the semaphore."
(two typos)
> I had used the kmod strategy to try to implement a similar concurrency > delimiter for the kernel_read*() calls from the finit_module() path > so to reduce vmalloc() memory pressure. That effort didn't provid yet
s/provid/provide/
> conclusive results, but one thing that did became clear is we can use
s/did // (or s/became/become/)
> the suggested alternative solution with semaphores which Linus hinted > at instead of using the atomic / wait strategy. > > I've stress tested this with kmod test 0008: > > time /data/linux-next/tools/testing/selftests/kmod/kmod.sh -t 0008 > > And I get only a *slight* delay. That delay however is small, a few > seconds for a full test loop run that runs 150 times, for about ~30-40 > seconds. The small delay is worth the simplfication IMHO. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > kernel/module/kmod.c | 26 +++++++------------------- > 1 file changed, 7 insertions(+), 19 deletions(-) > > diff --git a/kernel/module/kmod.c b/kernel/module/kmod.c > index b717134ebe17..925eb85b8346 100644 > --- a/kernel/module/kmod.c > +++ b/kernel/module/kmod.c > @@ -40,8 +40,7 @@ > * effect. Systems like these are very unlikely if modules are enabled. > */ > #define MAX_KMOD_CONCURRENT 50 > -static atomic_t kmod_concurrent_max = ATOMIC_INIT(MAX_KMOD_CONCURRENT); > -static DECLARE_WAIT_QUEUE_HEAD(kmod_wq); > +static DEFINE_SEMAPHORE(kmod_concurrent_max, MAX_KMOD_CONCURRENT); > > /* > * This is a restriction on having *all* MAX_KMOD_CONCURRENT threads > @@ -148,29 +147,18 @@ int __request_module(bool wait, const char *fmt, ...) > if (ret) > return ret; > > - if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) { > - pr_warn_ratelimited("request_module: kmod_concurrent_max (%u) close to 0 (max_modprobes: %u), for module %s, throttling...", > - atomic_read(&kmod_concurrent_max), > - MAX_KMOD_CONCURRENT, module_name); > - ret = wait_event_killable_timeout(kmod_wq, > - atomic_dec_if_positive(&kmod_concurrent_max) >= 0, > - MAX_KMOD_ALL_BUSY_TIMEOUT * HZ); > - if (!ret) { > - pr_warn_ratelimited("request_module: modprobe %s cannot be processed, kmod busy with %d threads for more than %d seconds now", > - module_name, MAX_KMOD_CONCURRENT, MAX_KMOD_ALL_BUSY_TIMEOUT); > - return -ETIME; > - } else if (ret == -ERESTARTSYS) { > - pr_warn_ratelimited("request_module: sigkill sent for modprobe %s, giving up", module_name); > - return ret; > - } > + ret = down_timeout(&kmod_concurrent_max, MAX_KMOD_ALL_BUSY_TIMEOUT);
MAX_KMOD_ALL_BUSY_TIMEOUT * HZ ?
The simplification is very nice.
Miroslav
| |