[PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[Greg Thelen]

mem_cgroup_get_limit() returns a byte limit as a unsigned 64 bit value,
which is converted to a page count by mem_cgroup_out_of_memory().  Prior
to this patch the conversion could overflow on 32 bit platforms
yielding a limit of zero.

Signed-off-by: Greg Thelen <gthelen@google.com>
---
 mm/oom_kill.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7dcca55..3fcac51 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -538,7 +538,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	struct task_struct *p;
 
 	check_panic_on_oom(CONSTRAINT_MEMCG, gfp_mask, 0, NULL);
-	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
+	limit = min(mem_cgroup_get_limit(mem) >> PAGE_SHIFT, (u64)ULONG_MAX);
 	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, limit, mem, NULL);
-- 
1.7.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[Balbir Singh]

On Wed, Jan 26, 2011 at 1:59 PM, Greg Thelen <gthelen@google.com> wrote:
> mem_cgroup_get_limit() returns a byte limit as a unsigned 64 bit value,
> which is converted to a page count by mem_cgroup_out_of_memory().  Prior
> to this patch the conversion could overflow on 32 bit platforms
> yielding a limit of zero.
>

Why would the overflow occur? Due to the right shift being used with
unsigned long long? I am afraid I don't have quick access to a 32 bit
box to test the patch. May be I too drained to understand the problem,
can you post the corresponding assembly for analysis?

Balbir

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[Johannes Weiner]

On Wed, Jan 26, 2011 at 12:29:15AM -0800, Greg Thelen wrote:
> mem_cgroup_get_limit() returns a byte limit as a unsigned 64 bit value,
> which is converted to a page count by mem_cgroup_out_of_memory().  Prior
> to this patch the conversion could overflow on 32 bit platforms
> yielding a limit of zero.

Balbir: It can truncate, because the conversion shrinks the required
bits of this 64-bit number by only PAGE_SHIFT (12).  Trying to store
the resulting up to 52 significant bits in a 32-bit integer will cut
up to 20 significant bits off.

> Signed-off-by: Greg Thelen <gthelen@google.com>
> ---
>  mm/oom_kill.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 7dcca55..3fcac51 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -538,7 +538,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
>  	struct task_struct *p;
>  
>  	check_panic_on_oom(CONSTRAINT_MEMCG, gfp_mask, 0, NULL);
> -	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
> +	limit = min(mem_cgroup_get_limit(mem) >> PAGE_SHIFT, (u64)ULONG_MAX);

I would much prefer using min_t(u64, ...).  To make it really, really
explicit that this is 64-bit arithmetic.  But that is just me, no
correctness issue.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[Greg Thelen]

Johannes Weiner <hannes@cmpxchg.org> writes:

> On Wed, Jan 26, 2011 at 12:29:15AM -0800, Greg Thelen wrote:
>> mem_cgroup_get_limit() returns a byte limit as a unsigned 64 bit value,
>> which is converted to a page count by mem_cgroup_out_of_memory().  Prior
>> to this patch the conversion could overflow on 32 bit platforms
>> yielding a limit of zero.
>
> Balbir: It can truncate, because the conversion shrinks the required
> bits of this 64-bit number by only PAGE_SHIFT (12).  Trying to store
> the resulting up to 52 significant bits in a 32-bit integer will cut
> up to 20 significant bits off.
>
>> Signed-off-by: Greg Thelen <gthelen@google.com>
>> ---
>>  mm/oom_kill.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
>> index 7dcca55..3fcac51 100644
>> --- a/mm/oom_kill.c
>> +++ b/mm/oom_kill.c
>> @@ -538,7 +538,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
>>  	struct task_struct *p;
>>  
>>  	check_panic_on_oom(CONSTRAINT_MEMCG, gfp_mask, 0, NULL);
>> -	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
>> +	limit = min(mem_cgroup_get_limit(mem) >> PAGE_SHIFT, (u64)ULONG_MAX);
>
> I would much prefer using min_t(u64, ...).  To make it really, really
> explicit that this is 64-bit arithmetic.  But that is just me, no
> correctness issue.
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

I agree that min_t() is clearer.  Does the following look better?

Author: Greg Thelen <gthelen@google.com>
Date:   Wed Jan 26 00:05:59 2011 -0800

    oom: handle truncation in mem_cgroup_out_of_memory()
    
    mem_cgroup_get_limit() returns a byte limit as an unsigned 64 bit value.
    mem_cgroup_out_of_memory() converts this byte limit to an unsigned long
    page count.  Prior to this patch, the 32 bit version of
    mem_cgroup_out_of_memory() would silently truncate the most significant
    20 bits from byte limit when constructing the limit as a page count.
    For byte limits with the lowest 44 bits set to zero, this truncation
    would compute a page limit of zero.
    
    This patch checks for such large byte limits that cannot be converted to
    page counts without loosing information.  In such situations, where a 32
    bit page counter is too small to represent the corresponding byte count,
    select a maximal page count.
    
    Signed-off-by: Greg Thelen <gthelen@google.com>

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7dcca55..0164060 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -538,7 +538,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	struct task_struct *p;
 
 	check_panic_on_oom(CONSTRAINT_MEMCG, gfp_mask, 0, NULL);
-	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
+	limit = min_t(u64, mem_cgroup_get_limit(mem) >> PAGE_SHIFT, ULONG_MAX);
 	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, limit, mem, NULL);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[Johannes Weiner]

On Wed, Jan 26, 2011 at 09:33:09AM -0800, Greg Thelen wrote:
> Johannes Weiner <hannes@cmpxchg.org> writes:
> 
> > On Wed, Jan 26, 2011 at 12:29:15AM -0800, Greg Thelen wrote:
> >> mem_cgroup_get_limit() returns a byte limit as a unsigned 64 bit value,
> >> which is converted to a page count by mem_cgroup_out_of_memory().  Prior
> >> to this patch the conversion could overflow on 32 bit platforms
> >> yielding a limit of zero.
> >
> > Balbir: It can truncate, because the conversion shrinks the required
> > bits of this 64-bit number by only PAGE_SHIFT (12).  Trying to store
> > the resulting up to 52 significant bits in a 32-bit integer will cut
> > up to 20 significant bits off.
> >
> >> Signed-off-by: Greg Thelen <gthelen@google.com>
> >> ---
> >>  mm/oom_kill.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> >> index 7dcca55..3fcac51 100644
> >> --- a/mm/oom_kill.c
> >> +++ b/mm/oom_kill.c
> >> @@ -538,7 +538,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
> >>  	struct task_struct *p;
> >>  
> >>  	check_panic_on_oom(CONSTRAINT_MEMCG, gfp_mask, 0, NULL);
> >> -	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
> >> +	limit = min(mem_cgroup_get_limit(mem) >> PAGE_SHIFT, (u64)ULONG_MAX);
> >
> > I would much prefer using min_t(u64, ...).  To make it really, really
> > explicit that this is 64-bit arithmetic.  But that is just me, no
> > correctness issue.
> >
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> I agree that min_t() is clearer.  Does the following look better?

Sweet, thank you Greg!

> Author: Greg Thelen <gthelen@google.com>
> Date:   Wed Jan 26 00:05:59 2011 -0800
> 
>     oom: handle truncation in mem_cgroup_out_of_memory()
>     
>     mem_cgroup_get_limit() returns a byte limit as an unsigned 64 bit value.
>     mem_cgroup_out_of_memory() converts this byte limit to an unsigned long
>     page count.  Prior to this patch, the 32 bit version of
>     mem_cgroup_out_of_memory() would silently truncate the most significant
>     20 bits from byte limit when constructing the limit as a page count.
>     For byte limits with the lowest 44 bits set to zero, this truncation
>     would compute a page limit of zero.
>     
>     This patch checks for such large byte limits that cannot be converted to
>     page counts without loosing information.  In such situations, where a 32
>     bit page counter is too small to represent the corresponding byte count,
>     select a maximal page count.
>     
>     Signed-off-by: Greg Thelen <gthelen@google.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

That being said, does this have any practical impact at all?  I mean,
this code runs when the cgroup limit is breached.  But if the number
of allowed pages (not bytes!) can not fit into 32 bits, it means you
have a group of processes using more than 16T.  On a 32-bit machine.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[Greg Thelen]

Johannes Weiner <hannes@cmpxchg.org> writes:

> On Wed, Jan 26, 2011 at 09:33:09AM -0800, Greg Thelen wrote:
>> Johannes Weiner <hannes@cmpxchg.org> writes:
>> 
>> > On Wed, Jan 26, 2011 at 12:29:15AM -0800, Greg Thelen wrote:
>> >> mem_cgroup_get_limit() returns a byte limit as a unsigned 64 bit value,
>> >> which is converted to a page count by mem_cgroup_out_of_memory().  Prior
>> >> to this patch the conversion could overflow on 32 bit platforms
>> >> yielding a limit of zero.
>> >
>> > Balbir: It can truncate, because the conversion shrinks the required
>> > bits of this 64-bit number by only PAGE_SHIFT (12).  Trying to store
>> > the resulting up to 52 significant bits in a 32-bit integer will cut
>> > up to 20 significant bits off.
>> >
>> >> Signed-off-by: Greg Thelen <gthelen@google.com>
>> >> ---
>> >>  mm/oom_kill.c |    2 +-
>> >>  1 files changed, 1 insertions(+), 1 deletions(-)
>> >> 
>> >> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
>> >> index 7dcca55..3fcac51 100644
>> >> --- a/mm/oom_kill.c
>> >> +++ b/mm/oom_kill.c
>> >> @@ -538,7 +538,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
>> >>  	struct task_struct *p;
>> >>  
>> >>  	check_panic_on_oom(CONSTRAINT_MEMCG, gfp_mask, 0, NULL);
>> >> -	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
>> >> +	limit = min(mem_cgroup_get_limit(mem) >> PAGE_SHIFT, (u64)ULONG_MAX);
>> >
>> > I would much prefer using min_t(u64, ...).  To make it really, really
>> > explicit that this is 64-bit arithmetic.  But that is just me, no
>> > correctness issue.
>> >
>> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>> 
>> I agree that min_t() is clearer.  Does the following look better?
>
> Sweet, thank you Greg!
>
>> Author: Greg Thelen <gthelen@google.com>
>> Date:   Wed Jan 26 00:05:59 2011 -0800
>> 
>>     oom: handle truncation in mem_cgroup_out_of_memory()
>>     
>>     mem_cgroup_get_limit() returns a byte limit as an unsigned 64 bit value.
>>     mem_cgroup_out_of_memory() converts this byte limit to an unsigned long
>>     page count.  Prior to this patch, the 32 bit version of
>>     mem_cgroup_out_of_memory() would silently truncate the most significant
>>     20 bits from byte limit when constructing the limit as a page count.
>>     For byte limits with the lowest 44 bits set to zero, this truncation
>>     would compute a page limit of zero.
>>     
>>     This patch checks for such large byte limits that cannot be converted to
>>     page counts without loosing information.  In such situations, where a 32
>>     bit page counter is too small to represent the corresponding byte count,
>>     select a maximal page count.
>>     
>>     Signed-off-by: Greg Thelen <gthelen@google.com>
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>
> That being said, does this have any practical impact at all?  I mean,
> this code runs when the cgroup limit is breached.  But if the number
> of allowed pages (not bytes!) can not fit into 32 bits, it means you
> have a group of processes using more than 16T.  On a 32-bit machine.

The value of this patch is up for debate.  I do not have an example
situation where this truncation causes the wrong thing to happen.  I
suppose it might be possible for a racing update to
memory.limit_in_bytes which grows the limit from a reasonable (example:
100M) limit to a large limit (example 1<<45) could benefit from this
patch.  I admit that this case seems pathological and may not be likely
or even worth bothering over.  If neither the memcg nor the oom
maintainers want the patch, then feel free to drop it.  I just noticed
the issue and thought it might be worth addressing.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[Andrew Morton]

On Wed, 26 Jan 2011 12:32:04 -0800
Greg Thelen <gthelen@google.com> wrote:

> > That being said, does this have any practical impact at all?  I mean,
> > this code runs when the cgroup limit is breached.  But if the number
> > of allowed pages (not bytes!) can not fit into 32 bits, it means you
> > have a group of processes using more than 16T.  On a 32-bit machine.
> 
> The value of this patch is up for debate.  I do not have an example
> situation where this truncation causes the wrong thing to happen.  I
> suppose it might be possible for a racing update to
> memory.limit_in_bytes which grows the limit from a reasonable (example:
> 100M) limit to a large limit (example 1<<45) could benefit from this
> patch.  I admit that this case seems pathological and may not be likely
> or even worth bothering over.  If neither the memcg nor the oom
> maintainers want the patch, then feel free to drop it.  I just noticed
> the issue and thought it might be worth addressing.

Ah.  I was scratching my head over that.

In zillions of places the kernel assumes that a 32-bit kernel has less
than 2^32 pages of memory, so the code as it stands is, umm, idiomatic.

But afaict the only way the patch makes a real-world difference is if
res_counter_read_u64() is busted?

And, as you point out, res_counter_read_u64() is indeed busted on
32-bit machines.  It has 25 callsites in mm/memcontrol.c - has anyone
looked at the implications of this?  What happens in all those
callsites if the counter is read during a count rollover?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[KAMEZAWA Hiroyuki]

On Wed, 26 Jan 2011 14:29:09 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 26 Jan 2011 12:32:04 -0800
> Greg Thelen <gthelen@google.com> wrote:
> 
> > > That being said, does this have any practical impact at all?  I mean,
> > > this code runs when the cgroup limit is breached.  But if the number
> > > of allowed pages (not bytes!) can not fit into 32 bits, it means you
> > > have a group of processes using more than 16T.  On a 32-bit machine.
> > 
> > The value of this patch is up for debate.  I do not have an example
> > situation where this truncation causes the wrong thing to happen.  I
> > suppose it might be possible for a racing update to
> > memory.limit_in_bytes which grows the limit from a reasonable (example:
> > 100M) limit to a large limit (example 1<<45) could benefit from this
> > patch.  I admit that this case seems pathological and may not be likely
> > or even worth bothering over.  If neither the memcg nor the oom
> > maintainers want the patch, then feel free to drop it.  I just noticed
> > the issue and thought it might be worth addressing.
> 
> Ah.  I was scratching my head over that.
> 
> In zillions of places the kernel assumes that a 32-bit kernel has less
> than 2^32 pages of memory, so the code as it stands is, umm, idiomatic.
> 

I think we can assume that. 

> But afaict the only way the patch makes a real-world difference is if
> res_counter_read_u64() is busted?
> 
> And, as you point out, res_counter_read_u64() is indeed busted on
> 32-bit machines.  It has 25 callsites in mm/memcontrol.c - has anyone
> looked at the implications of this?  What happens in all those
> callsites if the counter is read during a count rollover?
> 

I'll review. Against the roll-over, I think we just need to take lock.
So, res_counter_read_u64() implementation was wrong. It should take lock.
Please give me time.


THanks,
-Kame




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[BUGFIX] memcg: fix res_counter_read_u64 lock aware (Was Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[KAMEZAWA Hiroyuki]

On Thu, 27 Jan 2011 09:24:34 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote: > 
> 
> I'll review. Against the roll-over, I think we just need to take lock.
> So, res_counter_read_u64() implementation was wrong. It should take lock.
> Please give me time.
> 

As far as I can see usages of return value of res_counter_read_u64()
in memcontrol.c, all values are handle in u64 and no >> PAGE_SHIFT
to 'int' is not done. I'll see usage of u64 return value to
functions in other files from memcontrol.c

But, at least, this patch is required, I think. There are races.

==
res_counter_read_u64 reads u64 value without lock. It's dangerous
in 32bit environment. This patch adds lock.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 include/linux/res_counter.h |   13 ++++++++++++-
 kernel/res_counter.c        |    2 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

Index: mmotm-0125/include/linux/res_counter.h
===================================================================
--- mmotm-0125.orig/include/linux/res_counter.h
+++ mmotm-0125/include/linux/res_counter.h
@@ -68,7 +68,18 @@ struct res_counter {
  * @pos:     and the offset.
  */
 
-u64 res_counter_read_u64(struct res_counter *counter, int member);
+u64 res_counter_read_u64_locked(struct res_counter *counter, int member);
+
+static inline u64 res_counter_read_u64(struct res_counter *counter, int member)
+{
+	unsigned long flags;
+	u64 ret;
+
+	spin_lock_irqsave(&counter->lock, flags);
+	ret = res_counter_read_u64_locked(counter, member);
+	spin_unlock_irqrestore(&counter->lock, flags);
+	return ret;
+}
 
 ssize_t res_counter_read(struct res_counter *counter, int member,
 		const char __user *buf, size_t nbytes, loff_t *pos,
Index: mmotm-0125/kernel/res_counter.c
===================================================================
--- mmotm-0125.orig/kernel/res_counter.c
+++ mmotm-0125/kernel/res_counter.c
@@ -126,7 +126,7 @@ ssize_t res_counter_read(struct res_coun
 			pos, buf, s - buf);
 }
 
-u64 res_counter_read_u64(struct res_counter *counter, int member)
+u64 res_counter_read_u64_locked(struct res_counter *counter, int member)
 {
 	return *res_counter_member(counter, member);
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX] memcg: fix res_counter_read_u64 lock aware (Was Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[Andrew Morton]

On Thu, 27 Jan 2011 09:53:42 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> res_counter_read_u64 reads u64 value without lock. It's dangerous
> in 32bit environment. This patch adds lock.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  include/linux/res_counter.h |   13 ++++++++++++-
>  kernel/res_counter.c        |    2 +-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> Index: mmotm-0125/include/linux/res_counter.h
> ===================================================================
> --- mmotm-0125.orig/include/linux/res_counter.h
> +++ mmotm-0125/include/linux/res_counter.h
> @@ -68,7 +68,18 @@ struct res_counter {
>   * @pos:     and the offset.
>   */
>  
> -u64 res_counter_read_u64(struct res_counter *counter, int member);
> +u64 res_counter_read_u64_locked(struct res_counter *counter, int member);
> +
> +static inline u64 res_counter_read_u64(struct res_counter *counter, int member)
> +{
> +	unsigned long flags;
> +	u64 ret;
> +
> +	spin_lock_irqsave(&counter->lock, flags);
> +	ret = res_counter_read_u64_locked(counter, member);
> +	spin_unlock_irqrestore(&counter->lock, flags);
> +	return ret;
> +}
>  
>  ssize_t res_counter_read(struct res_counter *counter, int member,
>  		const char __user *buf, size_t nbytes, loff_t *pos,
> Index: mmotm-0125/kernel/res_counter.c
> ===================================================================
> --- mmotm-0125.orig/kernel/res_counter.c
> +++ mmotm-0125/kernel/res_counter.c
> @@ -126,7 +126,7 @@ ssize_t res_counter_read(struct res_coun
>  			pos, buf, s - buf);
>  }
>  
> -u64 res_counter_read_u64(struct res_counter *counter, int member)
> +u64 res_counter_read_u64_locked(struct res_counter *counter, int member)
>  {
>  	return *res_counter_member(counter, member);
>  }

We don't need the lock on 64-bit platforms!

And there's zero benefit to inlining the spin_lock/unlock(), given that
the function will always be making a function call anyway.

See i_size_read() for inspiration.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX] memcg: fix res_counter_read_u64 lock aware (Was Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[KAMEZAWA Hiroyuki]

On Wed, 26 Jan 2011 17:08:24 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 27 Jan 2011 09:53:42 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > res_counter_read_u64 reads u64 value without lock. It's dangerous
> > in 32bit environment. This patch adds lock.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  include/linux/res_counter.h |   13 ++++++++++++-
> >  kernel/res_counter.c        |    2 +-
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > Index: mmotm-0125/include/linux/res_counter.h
> > ===================================================================
> > --- mmotm-0125.orig/include/linux/res_counter.h
> > +++ mmotm-0125/include/linux/res_counter.h
> > @@ -68,7 +68,18 @@ struct res_counter {
> >   * @pos:     and the offset.
> >   */
> >  
> > -u64 res_counter_read_u64(struct res_counter *counter, int member);
> > +u64 res_counter_read_u64_locked(struct res_counter *counter, int member);
> > +
> > +static inline u64 res_counter_read_u64(struct res_counter *counter, int member)
> > +{
> > +	unsigned long flags;
> > +	u64 ret;
> > +
> > +	spin_lock_irqsave(&counter->lock, flags);
> > +	ret = res_counter_read_u64_locked(counter, member);
> > +	spin_unlock_irqrestore(&counter->lock, flags);
> > +	return ret;
> > +}
> >  
> >  ssize_t res_counter_read(struct res_counter *counter, int member,
> >  		const char __user *buf, size_t nbytes, loff_t *pos,
> > Index: mmotm-0125/kernel/res_counter.c
> > ===================================================================
> > --- mmotm-0125.orig/kernel/res_counter.c
> > +++ mmotm-0125/kernel/res_counter.c
> > @@ -126,7 +126,7 @@ ssize_t res_counter_read(struct res_coun
> >  			pos, buf, s - buf);
> >  }
> >  
> > -u64 res_counter_read_u64(struct res_counter *counter, int member)
> > +u64 res_counter_read_u64_locked(struct res_counter *counter, int member)
> >  {
> >  	return *res_counter_member(counter, member);
> >  }
> 
> We don't need the lock on 64-bit platforms!
> 
All res_counter function doesn't handle 32/64 bit differences. So, I did 
in this style for now.

> And there's zero benefit to inlining the spin_lock/unlock(), given that
> the function will always be making a function call anyway.
> 
Ok.

> See i_size_read() for inspiration.
> 

I hate res_counter ;) I'll update patch.

Thanks,
-Kame



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX] memcg: fix res_counter_read_u64 lock aware (Was Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[KAMEZAWA Hiroyuki]

On Wed, 26 Jan 2011 17:08:24 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 27 Jan 2011 09:53:42 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > res_counter_read_u64 reads u64 value without lock. It's dangerous
> > in 32bit environment. This patch adds lock.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  include/linux/res_counter.h |   13 ++++++++++++-
> >  kernel/res_counter.c        |    2 +-
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > Index: mmotm-0125/include/linux/res_counter.h
> > ===================================================================
> > --- mmotm-0125.orig/include/linux/res_counter.h
> > +++ mmotm-0125/include/linux/res_counter.h
> > @@ -68,7 +68,18 @@ struct res_counter {
> >   * @pos:     and the offset.
> >   */
> >  
> > -u64 res_counter_read_u64(struct res_counter *counter, int member);
> > +u64 res_counter_read_u64_locked(struct res_counter *counter, int member);
> > +
> > +static inline u64 res_counter_read_u64(struct res_counter *counter, int member)
> > +{
> > +	unsigned long flags;
> > +	u64 ret;
> > +
> > +	spin_lock_irqsave(&counter->lock, flags);
> > +	ret = res_counter_read_u64_locked(counter, member);
> > +	spin_unlock_irqrestore(&counter->lock, flags);
> > +	return ret;
> > +}
> >  
> >  ssize_t res_counter_read(struct res_counter *counter, int member,
> >  		const char __user *buf, size_t nbytes, loff_t *pos,
> > Index: mmotm-0125/kernel/res_counter.c
> > ===================================================================
> > --- mmotm-0125.orig/kernel/res_counter.c
> > +++ mmotm-0125/kernel/res_counter.c
> > @@ -126,7 +126,7 @@ ssize_t res_counter_read(struct res_coun
> >  			pos, buf, s - buf);
> >  }
> >  
> > -u64 res_counter_read_u64(struct res_counter *counter, int member)
> > +u64 res_counter_read_u64_locked(struct res_counter *counter, int member)
> >  {
> >  	return *res_counter_member(counter, member);
> >  }
> 
> We don't need the lock on 64-bit platforms!
> 
> And there's zero benefit to inlining the spin_lock/unlock(), given that
> the function will always be making a function call anyway.
> 
> See i_size_read() for inspiration.
> 

seq_counter can't be used for res_counter because I don't want to update
seq_counter at changing ->usae of res_counter. So, I'd like to just add
spinlock for this time.

I wonder making memcg's counter to use 32bit and record usage in the number
of pages may be a simple way...

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX] memcg: fix res_counter_read_u64 lock aware (Was Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[Andrew Morton]

On Thu, 27 Jan 2011 10:34:31 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> I wonder making memcg's counter to use 32bit and record usage in the number
> of pages may be a simple way...

That would be better, yes.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[BUGFIX v2] memcg: fix res_counter_read_u64 lock aware (Was Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[KAMEZAWA Hiroyuki]

Thank you for advices. This version doesn't use inlined function
and adds no overhead in 64bit.

Info:
res_counter_read_u64 is not frequently called in memcontrol.c now.
It's called at user-interface and interaction with other components.
This addition of lock will not add any performance troubles.

==
res_counter_read_u64 reads u64 value without lock. It's dangerous
in 32bit environment. This patch adds lock.

Changelog:
 - handle 32/64 bit in other funciton
 - avoid unnecessary inlining.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 kernel/res_counter.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

Index: mmotm-0125/kernel/res_counter.c
===================================================================
--- mmotm-0125.orig/kernel/res_counter.c
+++ mmotm-0125/kernel/res_counter.c
@@ -126,10 +126,24 @@ ssize_t res_counter_read(struct res_coun
 			pos, buf, s - buf);
 }
 
+#if BITS_PER_LONG == 32
+u64 res_counter_read_u64(struct res_counter *counter, int member)
+{
+	unsigned long flags;
+	u64 ret;
+
+	spin_lock_irqsave(&counter->lock, flags);
+	ret = *res_counter_member(counter, member);
+	spin_unlock_irqrestore(&counter->lock, flags);
+
+	return ret;
+}
+#else
 u64 res_counter_read_u64(struct res_counter *counter, int member)
 {
 	return *res_counter_member(counter, member);
 }
+#endif
 
 int res_counter_memparse_write_strategy(const char *buf,
 					unsigned long long *res)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX v2] memcg: fix res_counter_read_u64 lock aware (Was Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[Andrew Morton]

On Thu, 27 Jan 2011 10:43:39 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> --- mmotm-0125.orig/kernel/res_counter.c
> +++ mmotm-0125/kernel/res_counter.c
> @@ -126,10 +126,24 @@ ssize_t res_counter_read(struct res_coun
>  			pos, buf, s - buf);
>  }
>  
> +#if BITS_PER_LONG == 32
> +u64 res_counter_read_u64(struct res_counter *counter, int member)
> +{
> +	unsigned long flags;
> +	u64 ret;
> +
> +	spin_lock_irqsave(&counter->lock, flags);
> +	ret = *res_counter_member(counter, member);
> +	spin_unlock_irqrestore(&counter->lock, flags);
> +
> +	return ret;
> +}
> +#else
>  u64 res_counter_read_u64(struct res_counter *counter, int member)
>  {
>  	return *res_counter_member(counter, member);
>  }
> +#endif

_irqsave is only needed if the lock will be taken from irq context. 
Does that happen?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX v2] memcg: fix res_counter_read_u64 lock aware (Was Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[KAMEZAWA Hiroyuki]

On Wed, 26 Jan 2011 17:57:22 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 27 Jan 2011 10:43:39 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > --- mmotm-0125.orig/kernel/res_counter.c
> > +++ mmotm-0125/kernel/res_counter.c
> > @@ -126,10 +126,24 @@ ssize_t res_counter_read(struct res_coun
> >  			pos, buf, s - buf);
> >  }
> >  
> > +#if BITS_PER_LONG == 32
> > +u64 res_counter_read_u64(struct res_counter *counter, int member)
> > +{
> > +	unsigned long flags;
> > +	u64 ret;
> > +
> > +	spin_lock_irqsave(&counter->lock, flags);
> > +	ret = *res_counter_member(counter, member);
> > +	spin_unlock_irqrestore(&counter->lock, flags);
> > +
> > +	return ret;
> > +}
> > +#else
> >  u64 res_counter_read_u64(struct res_counter *counter, int member)
> >  {
> >  	return *res_counter_member(counter, member);
> >  }
> > +#endif
> 
> _irqsave is only needed if the lock will be taken from irq context. 
> Does that happen?
> 
I just obey current desing of res_counter, as bugfix.
This counter is designed to be safe against irq context.
Adding CC: to Balbir.

To be honest, it has never happened since res_counter is introduced. I imagine
there was a big plan when this counter was designed. But I think it will 
be never called other than memcg because cpu, blkio controller haven't
use res_counter, finally. And memcg tends to use per-cpu counter because of
performance.

If I need to remove irq flags from this function, I'll do in another patch
which changes total design of res_counter and make it not safe agaisnt irq context.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory()

[KAMEZAWA Hiroyuki]

On Wed, 26 Jan 2011 09:33:09 -0800
Greg Thelen <gthelen@google.com> wrote:

> Johannes Weiner <hannes@cmpxchg.org> writes:
> 
> > On Wed, Jan 26, 2011 at 12:29:15AM -0800, Greg Thelen wrote:
> >> mem_cgroup_get_limit() returns a byte limit as a unsigned 64 bit value,
> >> which is converted to a page count by mem_cgroup_out_of_memory().  Prior
> >> to this patch the conversion could overflow on 32 bit platforms
> >> yielding a limit of zero.
> >
> > Balbir: It can truncate, because the conversion shrinks the required
> > bits of this 64-bit number by only PAGE_SHIFT (12).  Trying to store
> > the resulting up to 52 significant bits in a 32-bit integer will cut
> > up to 20 significant bits off.
> >
> >> Signed-off-by: Greg Thelen <gthelen@google.com>
> >> ---
> >>  mm/oom_kill.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> >> index 7dcca55..3fcac51 100644
> >> --- a/mm/oom_kill.c
> >> +++ b/mm/oom_kill.c
> >> @@ -538,7 +538,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
> >>  	struct task_struct *p;
> >>  
> >>  	check_panic_on_oom(CONSTRAINT_MEMCG, gfp_mask, 0, NULL);
> >> -	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
> >> +	limit = min(mem_cgroup_get_limit(mem) >> PAGE_SHIFT, (u64)ULONG_MAX);
> >
> > I would much prefer using min_t(u64, ...).  To make it really, really
> > explicit that this is 64-bit arithmetic.  But that is just me, no
> > correctness issue.
> >
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> I agree that min_t() is clearer.  Does the following look better?
> 
> Author: Greg Thelen <gthelen@google.com>
> Date:   Wed Jan 26 00:05:59 2011 -0800
> 
>     oom: handle truncation in mem_cgroup_out_of_memory()
>     
>     mem_cgroup_get_limit() returns a byte limit as an unsigned 64 bit value.
>     mem_cgroup_out_of_memory() converts this byte limit to an unsigned long
>     page count.  Prior to this patch, the 32 bit version of
>     mem_cgroup_out_of_memory() would silently truncate the most significant
>     20 bits from byte limit when constructing the limit as a page count.
>     For byte limits with the lowest 44 bits set to zero, this truncation
>     would compute a page limit of zero.
>     
>     This patch checks for such large byte limits that cannot be converted to
>     page counts without loosing information.  In such situations, where a 32
>     bit page counter is too small to represent the corresponding byte count,
>     select a maximal page count.
>     
>     Signed-off-by: Greg Thelen <gthelen@google.com>
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 7dcca55..0164060 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -538,7 +538,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
>  	struct task_struct *p;
>  
>  	check_panic_on_oom(CONSTRAINT_MEMCG, gfp_mask, 0, NULL);
> -	limit = mem_cgroup_get_limit(mem) >> PAGE_SHIFT;
> +	limit = min_t(u64, mem_cgroup_get_limit(mem) >> PAGE_SHIFT, ULONG_MAX);
>  	read_lock(&tasklist_lock);
>  retry:
>  	p = select_bad_process(&points, limit, mem, NULL);
> 

Thank you for catching.

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>