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.

> 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 am sorry for the late review. The month ran really fast.

No worries!

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to