lkml.org 
[lkml]   [2017]   [Jun]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 4/4] kmod: throttle kmod thread limit
    On Fri 2017-06-23 18:16:19, Luis R. Rodriguez wrote:
    > On Thu, Jun 22, 2017 at 05:19:36PM +0200, Petr Mladek wrote:
    > > On Fri 2017-05-26 14:12:28, Luis R. Rodriguez wrote:
    > > > --- a/kernel/kmod.c
    > > > +++ b/kernel/kmod.c
    > > > @@ -163,14 +163,11 @@ int __request_module(bool wait, const char *fmt, ...)
    > > > return ret;
    > > >
    > > > if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
    > > > - /* We may be blaming an innocent here, but unlikely */
    > > > - if (kmod_loop_msg < 5) {
    > > > - printk(KERN_ERR
    > > > - "request_module: runaway loop modprobe %s\n",
    > > > - module_name);
    > > > - kmod_loop_msg++;
    > > > - }
    > > > - return -ENOMEM;
    > > > + pr_warn_ratelimited("request_module: kmod_concurrent_max (%u) close to 0 (max_modprobes: %u), for module %s\n, throttling...",
    > > > + atomic_read(&kmod_concurrent_max),
    > > > + 50, module_name);
    > >
    > > It is weird to pass the constant '50' via %s.
    >
    > The 50 was passed with %u, so I take it you meant it is odd to use a parameter
    > for it.

    Yeah, I meant %u and not %s.

    > > Also a #define should be
    > > used to keep it in sync with the kmod_concurrent_max initialization.
    >
    > OK.
    >
    > > > + wait_event_interruptible(kmod_wq,
    > > > + atomic_dec_if_positive(&kmod_concurrent_max) >= 0);
    > > > }
    > > >
    > > > trace_module_request(module_name, wait, _RET_IP_);
    > > > @@ -178,6 +175,7 @@ int __request_module(bool wait, const char *fmt, ...)
    > > > ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
    > > >
    > > > atomic_inc(&kmod_concurrent_max);
    > > > + wake_up_all(&kmod_wq);
    > >
    > > Does it make sense to wake up all waiters when we released the resource
    > > only for one? IMHO, a simple wake_up() should be here.
    >
    > Then we should wake_up() also on failure, otherwise we have the potential
    > to not wake some in a proper time.

    I think that we must wake_up() always when we increment
    kmod_concurrent_max. If the value was negative, the increment will
    allow exactly one process to pass that
    atomic_dec_if_positive(&kmod_concurrent_max) >= 0). It the value
    is positive, there must have been other wake_up() calls or there
    is no waiter.

    IMHO, this works because kmod_concurrent_max handling is atomic
    and race-less now. Also (s)wait_event_interruptible() is safe
    and does not allow to get into sleep when the resource is available.

    Anyway, it is great that you have double checked this.

    Best Regards,
    Petr

    \
     
     \ /
      Last update: 2017-06-26 11:55    [W:3.322 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site