Mailing List Archive: Linux: Kernel

[PATCH] mm/cgroup.c add error check

Index | Next | Previous | View Threaded

kosaki.motohiro at jp

May 6, 2008, 4:02 AM

Post #1 of 8 (1463 views)
Permalink

[PATCH] mm/cgroup.c add error check

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 [at] jp>
CC: "Paul Menage" <menage [at] google>
CC: Li Zefan <lizf [at] cn>

---
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;
+ }
kfree(pathbuf);
mutex_lock(&cgroup_mutex);
spin_lock(&release_list_lock);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

andi at firstfloor

May 6, 2008, 5:43 AM

Post #2 of 8 (1401 views)
Permalink

Re: [PATCH] mm/cgroup.c add error check [In reply to]

KOSAKI Motohiro <kosaki.motohiro [at] jp> 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 [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

kosaki.motohiro at jp

May 6, 2008, 6:02 AM

Post #3 of 8 (1407 views)
Permalink

Re: [PATCH] mm/cgroup.c add error check [In reply to]

> > 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 [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

andi at firstfloor

May 6, 2008, 11:32 AM

Post #4 of 8 (1399 views)
Permalink

Re: [PATCH] mm/cgroup.c add error check [In reply to]

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 [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

kosaki.motohiro at jp

May 6, 2008, 1:19 PM

Post #5 of 8 (1424 views)
Permalink

Re: [PATCH] mm/cgroup.c add error check [In reply to]

> > 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 [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

menage at google

May 6, 2008, 10:08 PM

Post #6 of 8 (1382 views)
Permalink

Re: [PATCH] mm/cgroup.c add error check [In reply to]

On Tue, May 6, 2008 at 4:02 AM, KOSAKI Motohiro
<kosaki.motohiro [at] jp> 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 [at] jp>
> CC: "Paul Menage" <menage [at] google>
> CC: Li Zefan <lizf [at] cn>
>
> ---
> 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 [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

menage at google

May 7, 2008, 12:43 AM

Post #7 of 8 (1392 views)
Permalink

Re: [PATCH] mm/cgroup.c add error check [In reply to]

On Wed, May 7, 2008 at 12:45 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu [at] jp> wrote:
> Hmm, but It seems call_usermodehelper()'s fails in silent, no messages.
> A notify which can be silently dropped is useless.

True, but if we're so short of memory that we can't fork, userspace
probably won't be able to do much about the notification even if it
gets it.

> Can we add 'printk("notify_on_release: ... is failed")' or some workaround ?
>

Sounds reasonable for debugging, but don't expect any automated
middleware to notice it.

Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

kamezawa.hiroyu at jp

May 7, 2008, 12:45 AM

Post #8 of 8 (1400 views)
Permalink

Re: [PATCH] mm/cgroup.c add error check [In reply to]

On Tue, 6 May 2008 22:08:08 -0700
"Paul Menage" <menage [at] google> wrote:

> On Tue, May 6, 2008 at 4:02 AM, KOSAKI Motohiro
> <kosaki.motohiro [at] jp> 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 [at] jp>
> > CC: "Paul Menage" <menage [at] google>
> > CC: Li Zefan <lizf [at] cn>
> >
> > ---
> > 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.
>
Hmm, but It seems call_usermodehelper()'s fails in silent, no messages.
A notify which can be silently dropped is useless.
Can we add 'printk("notify_on_release: ... is failed")' or some workaround ?

Thanks,
-Kame

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads