Mailing List Archive: Linux: Kernel

[PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty()

Index | Next | Previous | View Threaded

kamezawa.hiroyu at jp

May 11, 2012, 2:45 AM

Post #1 of 7 (74 views)
Permalink

[PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty()

The conditions are handled as -EBUSY, _now_.

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar [at] linux>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu [at] jp>
---
mm/hugetlb.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 1d3c8ea9..824f07b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1922,8 +1922,11 @@ int hugetlb_force_memcg_empty(struct cgroup *cgroup)
int ret = 0, idx = 0;

do {
- if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
+ if (cgroup_task_count(cgroup)
+ || !list_empty(&cgroup->children)) {
+ ret = -EBUSY;
goto out;
+ }
/*
* If the task doing the cgroup_rmdir got a signal
* we don't really need to loop till the hugetlb resource
--
1.7.4.1

--
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/

akpm at linux-foundation

May 11, 2012, 2:17 PM

Post #2 of 7 (74 views)
Permalink

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty() [In reply to]

On Fri, 11 May 2012 18:45:18 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu [at] jp> wrote:

> The conditions are handled as -EBUSY, _now_.

The changelog is poor. I rewrote it to

: hugetlb_force_memcg_empty() incorrectly returns 0 (success) when the
: cgroup is found to be busy. Return -EBUSY instead.

But it still doesn't tell us the end-user-visible effects of the bug.
It should.

--
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 13, 2012, 6:07 PM

Post #3 of 7 (68 views)
Permalink

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty() [In reply to]

(2012/05/12 6:17), Andrew Morton wrote:

> On Fri, 11 May 2012 18:45:18 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu [at] jp> wrote:
>
>> The conditions are handled as -EBUSY, _now_.
>
> The changelog is poor. I rewrote it to
>
> : hugetlb_force_memcg_empty() incorrectly returns 0 (success) when the
> : cgroup is found to be busy. Return -EBUSY instead.
>
> But it still doesn't tell us the end-user-visible effects of the bug.
> It should.
>

Ah, sorry. How about this ?

The force_empty interface allows to make the memcg only when the cgroup
doesn't include any tasks.

# echo 0 > /cgroup/xxxx/memory.force_empty

If cgroup isn't empty, force_empty does nothing and retruns -EBUSY in usual
memcg, memcontrol.c. But hugetlb implementation has inconsitency with
it and returns 0 and do nothing. Fix it to return -EBUSY.

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/

tj at kernel

May 14, 2012, 11:15 AM

Post #4 of 7 (69 views)
Permalink

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty() [In reply to]

On Fri, May 11, 2012 at 06:45:18PM +0900, KAMEZAWA Hiroyuki wrote:
> - if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
> + if (cgroup_task_count(cgroup)
> + || !list_empty(&cgroup->children)) {
> + ret = -EBUSY;
> goto out;

Why break the line? It doesn't go over 80 col.

Thanks.

--
tejun
--
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/

tj at kernel

May 14, 2012, 11:32 AM

Post #5 of 7 (68 views)
Permalink

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty() [In reply to]

On Mon, May 14, 2012 at 11:15:56AM -0700, Tejun Heo wrote:
> On Fri, May 11, 2012 at 06:45:18PM +0900, KAMEZAWA Hiroyuki wrote:
> > - if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
> > + if (cgroup_task_count(cgroup)
> > + || !list_empty(&cgroup->children)) {
> > + ret = -EBUSY;
> > goto out;
>
> Why break the line? It doesn't go over 80 col.

Ooh, it does. Sorry, my bad. But still, isn't it more usual to leave
the operator in the preceding line and align the start of the second
line with the first? ie.

if (cgroup_task_count(cgroup) ||
!list_empty(&cgroup->children)) {

Thanks.

--
tejun
--
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 14, 2012, 6:10 PM

Post #6 of 7 (69 views)
Permalink

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty() [In reply to]

(2012/05/15 3:32), Tejun Heo wrote:

> On Mon, May 14, 2012 at 11:15:56AM -0700, Tejun Heo wrote:
>> On Fri, May 11, 2012 at 06:45:18PM +0900, KAMEZAWA Hiroyuki wrote:
>>> - if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
>>> + if (cgroup_task_count(cgroup)
>>> + || !list_empty(&cgroup->children)) {
>>> + ret = -EBUSY;
>>> goto out;
>>
>> Why break the line? It doesn't go over 80 col.
>
> Ooh, it does. Sorry, my bad. But still, isn't it more usual to leave
> the operator in the preceding line and align the start of the second
> line with the first? ie.
>
> if (cgroup_task_count(cgroup) ||
> !list_empty(&cgroup->children)) {
>

How about this ?
==
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu [at] jp>
Date: Fri, 27 Apr 2012 13:19:19 +0900
Subject: [PATCH] memcg: fix error code in hugetlb_force_memcg_empty()

Changelog:
- clean up.
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar [at] linux>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu [at] jp>
---
mm/hugetlb.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 1d3c8ea9..82ec623 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1922,8 +1922,11 @@ int hugetlb_force_memcg_empty(struct cgroup *cgroup)
int ret = 0, idx = 0;

do {
- if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
+ if (cgroup_task_count(cgroup) ||
+ !list_empty(&cgroup->children)){
+ ret = -EBUSY;
goto out;
+ }
/*
* If the task doing the cgroup_rmdir got a signal
* we don't really need to loop till the hugetlb resource
--
1.7.4.1

--
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/

tj at kernel

May 15, 2012, 8:12 AM

Post #7 of 7 (69 views)
Permalink

Re: [PATCH v3 1/6] memcg: fix error code in hugetlb_force_memcg_empty() [In reply to]

On Tue, May 15, 2012 at 10:10:34AM +0900, KAMEZAWA Hiroyuki wrote:
> (2012/05/15 3:32), Tejun Heo wrote:
>
> > On Mon, May 14, 2012 at 11:15:56AM -0700, Tejun Heo wrote:
> >> On Fri, May 11, 2012 at 06:45:18PM +0900, KAMEZAWA Hiroyuki wrote:
> >>> - if (cgroup_task_count(cgroup) || !list_empty(&cgroup->children))
> >>> + if (cgroup_task_count(cgroup)
> >>> + || !list_empty(&cgroup->children)) {
> >>> + ret = -EBUSY;
> >>> goto out;
> >>
> >> Why break the line? It doesn't go over 80 col.
> >
> > Ooh, it does. Sorry, my bad. But still, isn't it more usual to leave
> > the operator in the preceding line and align the start of the second
> > line with the first? ie.
> >
> > if (cgroup_task_count(cgroup) ||
> > !list_empty(&cgroup->children)) {
> >
>
>
> How about this ?
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu [at] jp>
> Date: Fri, 27 Apr 2012 13:19:19 +0900
> Subject: [PATCH] memcg: fix error code in hugetlb_force_memcg_empty()
>
> Changelog:
> - clean up.
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar [at] linux>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu [at] jp>

Heh, it was a nitpick anyway. Please feel free to add my reviewed-by
for the whole series.

Thank you!

--
tejun
--
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