Prev: seq_file: add function to write binary data
Next: kbuild: let addtree result in absolute paths only
From: Andi Kleen on 6 May 2008 14:40
KOSAKI Motohiro wrote:
>> > on heavy workload, call_usermodehelper() may failure
>> > because it use kzmalloc(GFP_ATOMIC).
>>
>> Better just fix it to not use GFP_ATOMIC in the first place.
>
> Ah, makes sense.
> I'll try toi create that patch.

Thanks.

>
> but if GFP_KERNEL is used, We still need error check, IMHO.

Yes, but no retry (or if you're sure you cannot fail use __GFP_NOFAIL
too, but that is nasty because it has some risk of deadlock under severe
oom conditions)

-Andi


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: KOSAKI Motohiro on 6 May 2008 16:30
> > but if GFP_KERNEL is used, We still need error check, IMHO.
>
> Yes, but no retry (or if you're sure you cannot fail use __GFP_NOFAIL
> too, but that is nasty because it has some risk of deadlock under severe
> oom conditions)

in general coding style, you are right.

but not down-to-earth idea in that case.
call_usermodehelper() is just wrapper of fork-exec.

I don't hope change largely exec() code patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andi Kleen on 6 May 2008 08:50
KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com> writes:

> on heavy workload, call_usermodehelper() may failure
> because it use kzmalloc(GFP_ATOMIC).

Better just fix it to not use GFP_ATOMIC in the first place.

-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: KOSAKI Motohiro on 6 May 2008 09:10
> > on heavy workload, call_usermodehelper() may failure
> > because it use kzmalloc(GFP_ATOMIC).
>
> Better just fix it to not use GFP_ATOMIC in the first place.

Ah, makes sense.
I'll try toi create that patch.

but if GFP_KERNEL is used, We still need error check, IMHO.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Paul Menage on 7 May 2008 01:10
On Tue, May 6, 2008 at 4:02 AM, KOSAKI Motohiro
<kosaki.motohiro(a)jp.fujitsu.com> wrote:
>
> on heavy workload, call_usermodehelper() may failure
> because it use kzmalloc(GFP_ATOMIC).
>
> but userland want receive release notificcation even heavy workload.
>
> thus, We should retry if -ENOMEM happend.
>
>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com>
> CC: "Paul Menage" <menage(a)google.com>
> CC: Li Zefan <lizf(a)cn.fujitsu.com>
>
> ---
> kernel/cgroup.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> Index: b/kernel/cgroup.c
> ===================================================================
> --- a/kernel/cgroup.c 2008-04-29 18:00:53.000000000 +0900
> +++ b/kernel/cgroup.c 2008-05-06 20:28:23.000000000 +0900
> @@ -3072,6 +3072,8 @@ void __css_put(struct cgroup_subsys_stat
> */
> static void cgroup_release_agent(struct work_struct *work)
> {
> + int err;
> +
> BUG_ON(work != &release_agent_work);
> mutex_lock(&cgroup_mutex);
> spin_lock(&release_list_lock);
> @@ -3111,7 +3113,13 @@ static void cgroup_release_agent(struct
> * since the exec could involve hitting disk and hence
> * be a slow process */
> mutex_unlock(&cgroup_mutex);
> - call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
> +
> +retry:
> + err = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
> + if (err == -ENOMEM) {
> + schedule();
> + goto retry;
> + }

I'm not sure that an infinite loop retry is a great idea. Assuming
that call_usermodehelper() is changed to not use GFP_ATOMIC (which
should be safe at least in the case when UMG_WAIT_EXEC is set, since
we can clearly sleep in that case) then I'd be inclined not to check.
If we're so low on memory that we can't fork/exec, then the helper
binary itself could just as easily OOM as soon as we've launched it.
So there's still no guarantee. Userspace should just have to check for
empty cgroups occasionally even if they are using notify_on_release.

Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: seq_file: add function to write binary data
Next: kbuild: let addtree result in absolute paths only