* [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage
@ 2011-09-24 0:54 Steven Rostedt
2011-09-24 2:34 ` Greg Thelen
2011-09-26 0:43 ` KAMEZAWA Hiroyuki
0 siblings, 2 replies; 8+ messages in thread
From: Steven Rostedt @ 2011-09-24 0:54 UTC (permalink / raw)
To: LKML
Cc: Greg Thelen, KAMEZAWA Hiroyuki, Balbir Singh, Daisuke Nishimura,
Andrew Morton, Thomas Gleixner, PeterZijlstra, ChristophLameter
From: Steven Rostedt <srostedt@redhat.com>
The code in memcg_check_events() calls this_cpu_read() on
different variables without disabling preemption, and can cause
the calculations to be done from two different CPU variables.
Disable preemption throughout the check to keep apples and oranges
from becoming a mixed drink.
[ Added this_cpu to __this_cpu conversion by Johannes ]
Cc: Greg Thelen <gthelen@google.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/20110919212641.015320989@goodmis.org
---
mm/memcontrol.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 3508777..f823fc8 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -683,8 +683,8 @@ static bool __memcg_event_check(struct mem_cgroup *mem, int target)
{
unsigned long val, next;
- val = this_cpu_read(mem->stat->events[MEM_CGROUP_EVENTS_COUNT]);
- next = this_cpu_read(mem->stat->targets[target]);
+ val = __this_cpu_read(mem->stat->events[MEM_CGROUP_EVENTS_COUNT]);
+ next = __this_cpu_read(mem->stat->targets[target]);
/* from time_after() in jiffies.h */
return ((long)next - (long)val < 0);
}
@@ -693,7 +693,7 @@ static void __mem_cgroup_target_update(struct mem_cgroup *mem, int target)
{
unsigned long val, next;
- val = this_cpu_read(mem->stat->events[MEM_CGROUP_EVENTS_COUNT]);
+ val = __this_cpu_read(mem->stat->events[MEM_CGROUP_EVENTS_COUNT]);
switch (target) {
case MEM_CGROUP_TARGET_THRESH:
@@ -709,7 +709,7 @@ static void __mem_cgroup_target_update(struct mem_cgroup *mem, int target)
return;
}
- this_cpu_write(mem->stat->targets[target], next);
+ __this_cpu_write(mem->stat->targets[target], next);
}
/*
@@ -718,6 +718,7 @@ static void __mem_cgroup_target_update(struct mem_cgroup *mem, int target)
*/
static void memcg_check_events(struct mem_cgroup *mem, struct page *page)
{
+ preempt_disable();
/* threshold event is triggered in finer grain than soft limit */
if (unlikely(__memcg_event_check(mem, MEM_CGROUP_TARGET_THRESH))) {
mem_cgroup_threshold(mem);
@@ -737,6 +738,7 @@ static void memcg_check_events(struct mem_cgroup *mem, struct page *page)
}
#endif
}
+ preempt_enable();
}
static struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
--
1.7.3.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage
2011-09-24 0:54 [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage Steven Rostedt
@ 2011-09-24 2:34 ` Greg Thelen
2011-09-24 2:39 ` Greg Thelen
2011-09-26 0:43 ` KAMEZAWA Hiroyuki
1 sibling, 1 reply; 8+ messages in thread
From: Greg Thelen @ 2011-09-24 2:34 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, KAMEZAWA Hiroyuki, Balbir Singh, Daisuke Nishimura,
Andrew Morton, Thomas Gleixner, PeterZijlstra, ChristophLameter
On Fri, Sep 23, 2011 at 5:54 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> From: Steven Rostedt <srostedt@redhat.com>
>
> The code in memcg_check_events() calls this_cpu_read() on
> different variables without disabling preemption, and can cause
> the calculations to be done from two different CPU variables.
>
> Disable preemption throughout the check to keep apples and oranges
> from becoming a mixed drink.
>
> [ Added this_cpu to __this_cpu conversion by Johannes ]
>
> Cc: Greg Thelen <gthelen@google.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Christoph Lameter <cl@linux.com>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> Link: http://lkml.kernel.org/r/20110919212641.015320989@goodmis.org
Looks good to me. Thanks.
Reviewed-by: Greg Thelen <gthelen@google.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage
2011-09-24 2:34 ` Greg Thelen
@ 2011-09-24 2:39 ` Greg Thelen
2011-09-24 2:41 ` Steven Rostedt
0 siblings, 1 reply; 8+ messages in thread
From: Greg Thelen @ 2011-09-24 2:39 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, KAMEZAWA Hiroyuki, bsingharora, Daisuke Nishimura,
Andrew Morton, Thomas Gleixner, PeterZijlstra, ChristophLameter
cc: updated Balbir Singh email address
On Fri, Sep 23, 2011 at 7:34 PM, Greg Thelen <gthelen@google.com> wrote:
> On Fri, Sep 23, 2011 at 5:54 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> From: Steven Rostedt <srostedt@redhat.com>
>>
>> The code in memcg_check_events() calls this_cpu_read() on
>> different variables without disabling preemption, and can cause
>> the calculations to be done from two different CPU variables.
>>
>> Disable preemption throughout the check to keep apples and oranges
>> from becoming a mixed drink.
>>
>> [ Added this_cpu to __this_cpu conversion by Johannes ]
>>
>> Cc: Greg Thelen <gthelen@google.com>
>> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
Correction:
Cc: Balbir Singh <bsingharora@gmail.com>
>> Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Christoph Lameter <cl@linux.com>
>> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
>> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>> Link: http://lkml.kernel.org/r/20110919212641.015320989@goodmis.org
>
> Looks good to me. Thanks.
>
> Reviewed-by: Greg Thelen <gthelen@google.com>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage
2011-09-24 2:39 ` Greg Thelen
@ 2011-09-24 2:41 ` Steven Rostedt
2011-09-27 10:21 ` Balbir Singh
0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2011-09-24 2:41 UTC (permalink / raw)
To: Greg Thelen
Cc: LKML, KAMEZAWA Hiroyuki, bsingharora, Daisuke Nishimura,
Andrew Morton, Thomas Gleixner, PeterZijlstra, ChristophLameter
On Fri, 2011-09-23 at 19:39 -0700, Greg Thelen wrote:
> cc: updated Balbir Singh email address
>
> On Fri, Sep 23, 2011 at 7:34 PM, Greg Thelen <gthelen@google.com> wrote:
> > On Fri, Sep 23, 2011 at 5:54 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >> From: Steven Rostedt <srostedt@redhat.com>
> >>
> >> The code in memcg_check_events() calls this_cpu_read() on
> >> different variables without disabling preemption, and can cause
> >> the calculations to be done from two different CPU variables.
> >>
> >> Disable preemption throughout the check to keep apples and oranges
> >> from becoming a mixed drink.
> >>
> >> [ Added this_cpu to __this_cpu conversion by Johannes ]
> >>
> >> Cc: Greg Thelen <gthelen@google.com>
> >> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> >> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
>
> Correction:
> Cc: Balbir Singh <bsingharora@gmail.com>
Thanks, I was starting to hate that "return mail to sender" thing.
-- Steve
>
> >> Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: Thomas Gleixner <tglx@linutronix.de>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: Christoph Lameter <cl@linux.com>
> >> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> >> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> >> Link: http://lkml.kernel.org/r/20110919212641.015320989@goodmis.org
> >
> > Looks good to me. Thanks.
> >
> > Reviewed-by: Greg Thelen <gthelen@google.com>
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage
2011-09-24 0:54 [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage Steven Rostedt
2011-09-24 2:34 ` Greg Thelen
@ 2011-09-26 0:43 ` KAMEZAWA Hiroyuki
2011-09-26 23:45 ` Andrew Morton
1 sibling, 1 reply; 8+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-09-26 0:43 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Greg Thelen, Balbir Singh, Daisuke Nishimura, Andrew Morton,
Thomas Gleixner, PeterZijlstra, ChristophLameter
On Fri, 23 Sep 2011 20:54:42 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> From: Steven Rostedt <srostedt@redhat.com>
>
> The code in memcg_check_events() calls this_cpu_read() on
> different variables without disabling preemption, and can cause
> the calculations to be done from two different CPU variables.
>
> Disable preemption throughout the check to keep apples and oranges
> from becoming a mixed drink.
>
> [ Added this_cpu to __this_cpu conversion by Johannes ]
>
> Cc: Greg Thelen <gthelen@google.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Christoph Lameter <cl@linux.com>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> Link: http://lkml.kernel.org/r/20110919212641.015320989@goodmis.org
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Andrew, could you pick this up ?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage
2011-09-26 0:43 ` KAMEZAWA Hiroyuki
@ 2011-09-26 23:45 ` Andrew Morton
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2011-09-26 23:45 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Steven Rostedt, LKML, Greg Thelen, Balbir Singh,
Daisuke Nishimura, Thomas Gleixner, PeterZijlstra,
ChristophLameter, Andrew Morton
On Mon, 26 Sep 2011 09:43:22 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Fri, 23 Sep 2011 20:54:42 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > From: Steven Rostedt <srostedt@redhat.com>
> >
> > The code in memcg_check_events() calls this_cpu_read() on
> > different variables without disabling preemption, and can cause
> > the calculations to be done from two different CPU variables.
> >
> > Disable preemption throughout the check to keep apples and oranges
> > from becoming a mixed drink.
> >
> > [ Added this_cpu to __this_cpu conversion by Johannes ]
> >
> > Cc: Greg Thelen <gthelen@google.com>
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> > Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Christoph Lameter <cl@linux.com>
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > Link: http://lkml.kernel.org/r/20110919212641.015320989@goodmis.org
>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
> Andrew, could you pick this up ?
The patch needed rework due to other changes we have pending in there.
From: Steven Rostedt <srostedt@redhat.com>
Subject: memcg: Fix race condition in memcg_check_events() with this_cpu usage
Various code in memcontrol.c () calls this_cpu_read() on the calculations
to be done from two different percpu variables, or does an open-coded
read-modify-write on a single percpu variable.
Disable preemption throughout these operations so that the writes go to
the correct palces.
[ Added this_cpu to __this_cpu conversion by Johannes ]
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Greg Thelen <gthelen@google.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
mm/memcontrol.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff -puN mm/memcontrol.c~memcg-fix-race-condition-in-memcg_check_events-with-this_cpu-usage mm/memcontrol.c
--- a/mm/memcontrol.c~memcg-fix-race-condition-in-memcg_check_events-with-this_cpu-usage
+++ a/mm/memcontrol.c
@@ -687,8 +687,8 @@ static bool __memcg_event_check(struct m
{
unsigned long val, next;
- val = this_cpu_read(memcg->stat->events[MEM_CGROUP_EVENTS_COUNT]);
- next = this_cpu_read(memcg->stat->targets[target]);
+ val = __this_cpu_read(memcg->stat->events[MEM_CGROUP_EVENTS_COUNT]);
+ next = __this_cpu_read(memcg->stat->targets[target]);
/* from time_after() in jiffies.h */
return ((long)next - (long)val < 0);
}
@@ -697,7 +697,7 @@ static void __mem_cgroup_target_update(s
{
unsigned long val, next;
- val = this_cpu_read(memcg->stat->events[MEM_CGROUP_EVENTS_COUNT]);
+ val = __this_cpu_read(memcg->stat->events[MEM_CGROUP_EVENTS_COUNT]);
switch (target) {
case MEM_CGROUP_TARGET_THRESH:
@@ -713,7 +713,7 @@ static void __mem_cgroup_target_update(s
return;
}
- this_cpu_write(memcg->stat->targets[target], next);
+ __this_cpu_write(memcg->stat->targets[target], next);
}
/*
@@ -722,6 +722,7 @@ static void __mem_cgroup_target_update(s
*/
static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
{
+ preempt_disable();
/* threshold event is triggered in finer grain than soft limit */
if (unlikely(__memcg_event_check(memcg, MEM_CGROUP_TARGET_THRESH))) {
mem_cgroup_threshold(memcg);
@@ -741,6 +742,7 @@ static void memcg_check_events(struct me
}
#endif
}
+ preempt_enable();
}
static struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
_
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage
2011-09-24 2:41 ` Steven Rostedt
@ 2011-09-27 10:21 ` Balbir Singh
2011-09-27 16:16 ` Christoph Lameter
0 siblings, 1 reply; 8+ messages in thread
From: Balbir Singh @ 2011-09-27 10:21 UTC (permalink / raw)
To: Steven Rostedt
Cc: Greg Thelen, LKML, KAMEZAWA Hiroyuki, Daisuke Nishimura,
Andrew Morton, Thomas Gleixner, PeterZijlstra, ChristophLameter
On Sat, Sep 24, 2011 at 8:11 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2011-09-23 at 19:39 -0700, Greg Thelen wrote:
>> cc: updated Balbir Singh email address
>>
>> On Fri, Sep 23, 2011 at 7:34 PM, Greg Thelen <gthelen@google.com> wrote:
>> > On Fri, Sep 23, 2011 at 5:54 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> >> From: Steven Rostedt <srostedt@redhat.com>
>> >>
>> >> The code in memcg_check_events() calls this_cpu_read() on
>> >> different variables without disabling preemption, and can cause
>> >> the calculations to be done from two different CPU variables.
>> >>
>> >> Disable preemption throughout the check to keep apples and oranges
>> >> from becoming a mixed drink.
>> >>
>> >> [ Added this_cpu to __this_cpu conversion by Johannes ]
>> >>
>> >> Cc: Greg Thelen <gthelen@google.com>
>> >> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>> >> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
>>
>> Correction:
>> Cc: Balbir Singh <bsingharora@gmail.com>
>
> Thanks, I was starting to hate that "return mail to sender" thing.
>
Thanks for the correction. The change looks good to me
Acked-by: Balbir Singh <bsingharora@gmail.com>
Balbir
Balbir Singh
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage
2011-09-27 10:21 ` Balbir Singh
@ 2011-09-27 16:16 ` Christoph Lameter
0 siblings, 0 replies; 8+ messages in thread
From: Christoph Lameter @ 2011-09-27 16:16 UTC (permalink / raw)
To: Balbir Singh
Cc: Steven Rostedt, Greg Thelen, LKML, KAMEZAWA Hiroyuki,
Daisuke Nishimura, Andrew Morton, Thomas Gleixner, PeterZijlstra
On Tue, 27 Sep 2011, Balbir Singh wrote:
> Thanks for the correction. The change looks good to me
Same here.
Acked-by: Christoph Lameter <cl@gentwo.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-09-27 16:16 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-24 0:54 [PATCH] memcg: Fix race condition in memcg_check_events() with this_cpu usage Steven Rostedt
2011-09-24 2:34 ` Greg Thelen
2011-09-24 2:39 ` Greg Thelen
2011-09-24 2:41 ` Steven Rostedt
2011-09-27 10:21 ` Balbir Singh
2011-09-27 16:16 ` Christoph Lameter
2011-09-26 0:43 ` KAMEZAWA Hiroyuki
2011-09-26 23:45 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox