Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1672752 > unrolled thread

Re: [PATCH v3 4/4] kmod: throttle kmod thread limit

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 4/4] kmod: throttle kmod thread limit Petr Mladek <pmladek@suse.com> - 2017-06-22 17:30 +0200
    Re: [PATCH v3 4/4] kmod: throttle kmod thread limit "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-23 18:20 +0200
      Re: [PATCH v3 4/4] kmod: throttle kmod thread limit "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-23 20:00 +0200
        Re: [PATCH v3 4/4] kmod: throttle kmod thread limit "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-23 21:20 +0200
          Re: [PATCH v3 4/4] kmod: throttle kmod thread limit Petr Mladek <pmladek@suse.com> - 2017-06-26 12:10 +0200
      Re: [PATCH v3 4/4] kmod: throttle kmod thread limit Petr Mladek <pmladek@suse.com> - 2017-06-26 12:00 +0200

#1672752 — Re: [PATCH v3 4/4] kmod: throttle kmod thread limit

On Fri 2017-05-26 14:12:28, Luis R. Rodriguez wrote:
> If we reach the limit of modprobe_limit threads running the next
> request_module() call will fail. The original reason for adding
> a kill was to do away with possible issues with in old circumstances
> which would create a recursive series of request_module() calls.
> We can do better than just be super aggressive and reject calls
> once we've reached the limit by simply making pending callers wait
> until the threshold has been reduced.
> 
> The only difference is the clutch helps with avoiding making
> request_module() requests fatal more often. With x86_64 qemu,
> with 4 cores, 4 GiB of RAM it takes the following run time to
> run both tests:
> 
> time ./kmod.sh -t 0008
> real    0m12.364s
> user    0m0.704s
> sys     0m5.373s
> 
> time ./kmod.sh -t 0009
> real    0m47.638s
> user    0m1.033s
> sys     0m5.425s
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  kernel/kmod.c                        | 16 +++++++---------
>  tools/testing/selftests/kmod/kmod.sh | 24 ++----------------------
>  2 files changed, 9 insertions(+), 31 deletions(-)
> 
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 3e346c700e80..46b12fed6fd0 100644
> --- 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. Also a #define should be
used to keep it in sync with the kmod_concurrent_max initialization.


> +		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.

I am sorry for the late review. The month ran really fast.

Best Regards,
Petr

[toc] | [next] | [standalone]

#1673668

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

[toc] | [prev] | [next] | [standalone]

#1673743

On Fri, Jun 23, 2017 at 06:16:19PM +0200, 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
> > > @@ -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 checked and it turns out we have no error paths after we consume a kmod
ticket, if you will. Once we bump with atomic_dec_if_positive() we assume
we're moving forward with an attempt, and the only failure path is already
bundled with a wake at the end of the __request_module() call.

Then the next question would be *who* exactly gets woken up next if we just
use wake_up() ? The common core wake up code varies depending on use and
all this reminded me of the complexity we just don't need, so I have now
converted to use swait. swait uses list_add() if empty and then iterates
with list_first_entry() on wakeup, so that should get the first item added
to the wait list.

Works with me. Will run a test a before v4 is sent, but since only 2 patches
are modified will only send a respective update for these 2 patches.

  Luis

[toc] | [prev] | [next] | [standalone]

#1673791

On Fri, Jun 23, 2017 at 07:56:11PM +0200, Luis R. Rodriguez wrote:
> On Fri, Jun 23, 2017 at 06:16:19PM +0200, 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
> > > > @@ -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 checked and it turns out we have no error paths after we consume a kmod
> ticket, if you will. Once we bump with atomic_dec_if_positive() we assume
> we're moving forward with an attempt, and the only failure path is already
> bundled with a wake at the end of the __request_module() call.
> 
> Then the next question would be *who* exactly gets woken up next if we just
> use wake_up() ? The common core wake up code varies depending on use and
> all this reminded me of the complexity we just don't need, so I have now
> converted to use swait. swait uses list_add() if empty and then iterates
> with list_first_entry() on wakeup, so that should get the first item added
> to the wait list.
> 
> Works with me. Will run a test a before v4 is sent, but since only 2 patches
> are modified will only send a respective update for these 2 patches.

Alright, this worked out well! Its just a tiny bit slower on test cases 0008
and 0009 (few seconds) but that's fine, its natural due to the lack of the
swake_up_all().

  Luis

[toc] | [prev] | [next] | [standalone]

#1674612

On Fri 2017-06-23 21:16:37, Luis R. Rodriguez wrote:
> On Fri, Jun 23, 2017 at 07:56:11PM +0200, Luis R. Rodriguez wrote:
> > On Fri, Jun 23, 2017 at 06:16:19PM +0200, 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
> > > > > @@ -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 checked and it turns out we have no error paths after we consume a kmod
> > ticket, if you will. Once we bump with atomic_dec_if_positive() we assume
> > we're moving forward with an attempt, and the only failure path is already
> > bundled with a wake at the end of the __request_module() call.
> > 
> > Then the next question would be *who* exactly gets woken up next if we just
> > use wake_up() ? The common core wake up code varies depending on use and
> > all this reminded me of the complexity we just don't need, so I have now
> > converted to use swait. swait uses list_add() if empty and then iterates
> > with list_first_entry() on wakeup, so that should get the first item added
> > to the wait list.
> > 
> > Works with me. Will run a test a before v4 is sent, but since only 2 patches
> > are modified will only send a respective update for these 2 patches.
> 
> Alright, this worked out well! Its just a tiny bit slower on test cases 0008
> and 0009 (few seconds) but that's fine, its natural due to the lack of the
> swake_up_all().

This is interesting. I guess that it was faster with swake_up_all()
because it worked as a speculative pre-wake. I mean that it takes some
time between adding a process into run-queue and really running it.
IMHO, swake_up_all() caused that __request_module() callers were
more often really running and trying to pass that
atomic_dec_if_positive(&kmod_concurrent_max) >= 0).

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1674580

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web