[patch] mm: memcg: close race between charge and putback

[patch] mm: memcg: close race between charge and putback

[Johannes Weiner]

There is a potential race between a thread charging a page and another
thread putting it back to the LRU list:

charge:                         putback:
SetPageCgroupUsed               SetPageLRU
PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU

The order of setting one flag and checking the other is crucial,
otherwise the charge may observe !PageLRU while the putback observes
!PageCgroupUsed and the page is not linked to the memcg LRU at all.

Global memory pressure may fix this by trying to isolate and putback
the page for reclaim, where that putback would link it to the memcg
LRU again.  Without that, the memory cgroup is undeletable due to a
charge whose physical page can not be found and moved out.

Signed-off-by: Johannes Weiner <jweiner@redhat.com>
---
 mm/memcontrol.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d63dfb2..17708e1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -990,6 +990,16 @@ void mem_cgroup_add_lru_list(struct page *page, enum lru_list lru)
 		return;
 	pc = lookup_page_cgroup(page);
 	VM_BUG_ON(PageCgroupAcctLRU(pc));
+	/*
+	 * putback:				charge:
+	 * SetPageLRU				SetPageCgroupUsed
+	 * smp_mb				smp_mb
+	 * PageCgroupUsed && add to memcg LRU	PageLRU && add to memcg LRU
+	 *
+	 * Ensure that one of the two sides adds the page to the memcg
+	 * LRU during a race.
+	 */
+	smp_mb();
 	if (!PageCgroupUsed(pc))
 		return;
 	/* Ensure pc->mem_cgroup is visible after reading PCG_USED. */
@@ -1041,7 +1051,16 @@ static void mem_cgroup_lru_add_after_commit(struct page *page)
 	unsigned long flags;
 	struct zone *zone = page_zone(page);
 	struct page_cgroup *pc = lookup_page_cgroup(page);
-
+	/*
+	 * putback:				charge:
+	 * SetPageLRU				SetPageCgroupUsed
+	 * smp_mb				smp_mb
+	 * PageCgroupUsed && add to memcg LRU	PageLRU && add to memcg LRU
+	 *
+	 * Ensure that one of the two sides adds the page to the memcg
+	 * LRU during a race.
+	 */
+	smp_mb();
 	/* taking care of that the page is added to LRU while we commit it */
 	if (likely(!PageLRU(page)))
 		return;
-- 
1.7.6

[patch] mm: memcg: close race between charge and putback

[Johannes Weiner]

There is a potential race between a thread charging a page and another
thread putting it back to the LRU list:

charge:                         putback:
SetPageCgroupUsed               SetPageLRU
PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU

The order of setting one flag and checking the other is crucial,
otherwise the charge may observe !PageLRU while the putback observes
!PageCgroupUsed and the page is not linked to the memcg LRU at all.

Global memory pressure may fix this by trying to isolate and putback
the page for reclaim, where that putback would link it to the memcg
LRU again.  Without that, the memory cgroup is undeletable due to a
charge whose physical page can not be found and moved out.

Signed-off-by: Johannes Weiner <jweiner@redhat.com>
---
 mm/memcontrol.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d63dfb2..17708e1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -990,6 +990,16 @@ void mem_cgroup_add_lru_list(struct page *page, enum lru_list lru)
 		return;
 	pc = lookup_page_cgroup(page);
 	VM_BUG_ON(PageCgroupAcctLRU(pc));
+	/*
+	 * putback:				charge:
+	 * SetPageLRU				SetPageCgroupUsed
+	 * smp_mb				smp_mb
+	 * PageCgroupUsed && add to memcg LRU	PageLRU && add to memcg LRU
+	 *
+	 * Ensure that one of the two sides adds the page to the memcg
+	 * LRU during a race.
+	 */
+	smp_mb();
 	if (!PageCgroupUsed(pc))
 		return;
 	/* Ensure pc->mem_cgroup is visible after reading PCG_USED. */
@@ -1041,7 +1051,16 @@ static void mem_cgroup_lru_add_after_commit(struct page *page)
 	unsigned long flags;
 	struct zone *zone = page_zone(page);
 	struct page_cgroup *pc = lookup_page_cgroup(page);
-
+	/*
+	 * putback:				charge:
+	 * SetPageLRU				SetPageCgroupUsed
+	 * smp_mb				smp_mb
+	 * PageCgroupUsed && add to memcg LRU	PageLRU && add to memcg LRU
+	 *
+	 * Ensure that one of the two sides adds the page to the memcg
+	 * LRU during a race.
+	 */
+	smp_mb();
 	/* taking care of that the page is added to LRU while we commit it */
 	if (likely(!PageLRU(page)))
 		return;
-- 
1.7.6

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: memcg: close race between charge and putback

[KAMEZAWA Hiroyuki]

On Thu,  8 Sep 2011 09:40:22 +0200
Johannes Weiner <jweiner@redhat.com> wrote:

> There is a potential race between a thread charging a page and another
> thread putting it back to the LRU list:
> 
> charge:                         putback:
> SetPageCgroupUsed               SetPageLRU
> PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> 

I assumed that all pages are charged before added to LRU.
(i.e. event happens in charge->lru_lock->putback order.)

But hmm, this assumption may be bad for maintainance.
Do you find a code which adds pages to LRU before charge ?

Hmm, if there are codes which recharge the page to other memcg,
it will cause bug and my assumption may be harmful.

> The order of setting one flag and checking the other is crucial,
> otherwise the charge may observe !PageLRU while the putback observes
> !PageCgroupUsed and the page is not linked to the memcg LRU at all.
> 
> Global memory pressure may fix this by trying to isolate and putback
> the page for reclaim, where that putback would link it to the memcg
> LRU again.  Without that, the memory cgroup is undeletable due to a
> charge whose physical page can not be found and moved out.
> 
> Signed-off-by: Johannes Weiner <jweiner@redhat.com>


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


> ---
>  mm/memcontrol.c |   21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d63dfb2..17708e1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -990,6 +990,16 @@ void mem_cgroup_add_lru_list(struct page *page, enum lru_list lru)
>  		return;
>  	pc = lookup_page_cgroup(page);
>  	VM_BUG_ON(PageCgroupAcctLRU(pc));
> +	/*
> +	 * putback:				charge:
> +	 * SetPageLRU				SetPageCgroupUsed
> +	 * smp_mb				smp_mb
> +	 * PageCgroupUsed && add to memcg LRU	PageLRU && add to memcg LRU
> +	 *
> +	 * Ensure that one of the two sides adds the page to the memcg
> +	 * LRU during a race.
> +	 */
> +	smp_mb();
>  	if (!PageCgroupUsed(pc))
>  		return;
>  	/* Ensure pc->mem_cgroup is visible after reading PCG_USED. */
> @@ -1041,7 +1051,16 @@ static void mem_cgroup_lru_add_after_commit(struct page *page)
>  	unsigned long flags;
>  	struct zone *zone = page_zone(page);
>  	struct page_cgroup *pc = lookup_page_cgroup(page);
> -
> +	/*
> +	 * putback:				charge:
> +	 * SetPageLRU				SetPageCgroupUsed
> +	 * smp_mb				smp_mb
> +	 * PageCgroupUsed && add to memcg LRU	PageLRU && add to memcg LRU
> +	 *
> +	 * Ensure that one of the two sides adds the page to the memcg
> +	 * LRU during a race.
> +	 */
> +	smp_mb();
>  	/* taking care of that the page is added to LRU while we commit it */
>  	if (likely(!PageLRU(page)))
>  		return;
> -- 
> 1.7.6
> 
> 

Re: [patch] mm: memcg: close race between charge and putback

[KAMEZAWA Hiroyuki]

On Thu,  8 Sep 2011 09:40:22 +0200
Johannes Weiner <jweiner@redhat.com> wrote:

> There is a potential race between a thread charging a page and another
> thread putting it back to the LRU list:
> 
> charge:                         putback:
> SetPageCgroupUsed               SetPageLRU
> PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> 

I assumed that all pages are charged before added to LRU.
(i.e. event happens in charge->lru_lock->putback order.)

But hmm, this assumption may be bad for maintainance.
Do you find a code which adds pages to LRU before charge ?

Hmm, if there are codes which recharge the page to other memcg,
it will cause bug and my assumption may be harmful.

> The order of setting one flag and checking the other is crucial,
> otherwise the charge may observe !PageLRU while the putback observes
> !PageCgroupUsed and the page is not linked to the memcg LRU at all.
> 
> Global memory pressure may fix this by trying to isolate and putback
> the page for reclaim, where that putback would link it to the memcg
> LRU again.  Without that, the memory cgroup is undeletable due to a
> charge whose physical page can not be found and moved out.
> 
> Signed-off-by: Johannes Weiner <jweiner@redhat.com>


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


> ---
>  mm/memcontrol.c |   21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d63dfb2..17708e1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -990,6 +990,16 @@ void mem_cgroup_add_lru_list(struct page *page, enum lru_list lru)
>  		return;
>  	pc = lookup_page_cgroup(page);
>  	VM_BUG_ON(PageCgroupAcctLRU(pc));
> +	/*
> +	 * putback:				charge:
> +	 * SetPageLRU				SetPageCgroupUsed
> +	 * smp_mb				smp_mb
> +	 * PageCgroupUsed && add to memcg LRU	PageLRU && add to memcg LRU
> +	 *
> +	 * Ensure that one of the two sides adds the page to the memcg
> +	 * LRU during a race.
> +	 */
> +	smp_mb();
>  	if (!PageCgroupUsed(pc))
>  		return;
>  	/* Ensure pc->mem_cgroup is visible after reading PCG_USED. */
> @@ -1041,7 +1051,16 @@ static void mem_cgroup_lru_add_after_commit(struct page *page)
>  	unsigned long flags;
>  	struct zone *zone = page_zone(page);
>  	struct page_cgroup *pc = lookup_page_cgroup(page);
> -
> +	/*
> +	 * putback:				charge:
> +	 * SetPageLRU				SetPageCgroupUsed
> +	 * smp_mb				smp_mb
> +	 * PageCgroupUsed && add to memcg LRU	PageLRU && add to memcg LRU
> +	 *
> +	 * Ensure that one of the two sides adds the page to the memcg
> +	 * LRU during a race.
> +	 */
> +	smp_mb();
>  	/* taking care of that the page is added to LRU while we commit it */
>  	if (likely(!PageLRU(page)))
>  		return;
> -- 
> 1.7.6
> 
> 

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: memcg: close race between charge and putback

[Johannes Weiner]

On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu,  8 Sep 2011 09:40:22 +0200
> Johannes Weiner <jweiner@redhat.com> wrote:
> 
> > There is a potential race between a thread charging a page and another
> > thread putting it back to the LRU list:
> > 
> > charge:                         putback:
> > SetPageCgroupUsed               SetPageLRU
> > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > 
> 
> I assumed that all pages are charged before added to LRU.
> (i.e. event happens in charge->lru_lock->putback order.)
> 
> But hmm, this assumption may be bad for maintainance.
> Do you find a code which adds pages to LRU before charge ?
> 
> Hmm, if there are codes which recharge the page to other memcg,
> it will cause bug and my assumption may be harmful.

Swap slots are read optimistically into swapcache and put to the LRU,
then charged upon fault.  Fuse apparently recharges uncharged LRU
pages.  That's why we have the lrucare stuff in the first place, no?
Or did I misunderstand your question?

Re: [patch] mm: memcg: close race between charge and putback

[Johannes Weiner]

On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu,  8 Sep 2011 09:40:22 +0200
> Johannes Weiner <jweiner@redhat.com> wrote:
> 
> > There is a potential race between a thread charging a page and another
> > thread putting it back to the LRU list:
> > 
> > charge:                         putback:
> > SetPageCgroupUsed               SetPageLRU
> > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > 
> 
> I assumed that all pages are charged before added to LRU.
> (i.e. event happens in charge->lru_lock->putback order.)
> 
> But hmm, this assumption may be bad for maintainance.
> Do you find a code which adds pages to LRU before charge ?
> 
> Hmm, if there are codes which recharge the page to other memcg,
> it will cause bug and my assumption may be harmful.

Swap slots are read optimistically into swapcache and put to the LRU,
then charged upon fault.  Fuse apparently recharges uncharged LRU
pages.  That's why we have the lrucare stuff in the first place, no?
Or did I misunderstand your question?

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: memcg: close race between charge and putback

[KAMEZAWA Hiroyuki]

On Thu, 8 Sep 2011 10:54:04 +0200
Johannes Weiner <jweiner@redhat.com> wrote:

> On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu,  8 Sep 2011 09:40:22 +0200
> > Johannes Weiner <jweiner@redhat.com> wrote:
> > 
> > > There is a potential race between a thread charging a page and another
> > > thread putting it back to the LRU list:
> > > 
> > > charge:                         putback:
> > > SetPageCgroupUsed               SetPageLRU
> > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > 
> > 
> > I assumed that all pages are charged before added to LRU.
> > (i.e. event happens in charge->lru_lock->putback order.)
> > 
> > But hmm, this assumption may be bad for maintainance.
> > Do you find a code which adds pages to LRU before charge ?
> > 
> > Hmm, if there are codes which recharge the page to other memcg,
> > it will cause bug and my assumption may be harmful.
> 
> Swap slots are read optimistically into swapcache and put to the LRU,
> then charged upon fault.  

Yes, then swap charge removes page from LRU before charge.
IIUC, it needed to do so because page->mem_cgroup may be replaced.

> Fuse apparently recharges uncharged LRU pages.
Yes and No. IIUC, it was like page migraion and remove old page 
and add new page to radix-tree.

> That's why we have the lrucare stuff in the first place, no?
You're right.

> Or did I misunderstand your question?
> 

I just wondered whether you find a new one or possible user.

Thanks,
-Kame

Re: [patch] mm: memcg: close race between charge and putback

[KAMEZAWA Hiroyuki]

On Thu, 8 Sep 2011 10:54:04 +0200
Johannes Weiner <jweiner@redhat.com> wrote:

> On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu,  8 Sep 2011 09:40:22 +0200
> > Johannes Weiner <jweiner@redhat.com> wrote:
> > 
> > > There is a potential race between a thread charging a page and another
> > > thread putting it back to the LRU list:
> > > 
> > > charge:                         putback:
> > > SetPageCgroupUsed               SetPageLRU
> > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > 
> > 
> > I assumed that all pages are charged before added to LRU.
> > (i.e. event happens in charge->lru_lock->putback order.)
> > 
> > But hmm, this assumption may be bad for maintainance.
> > Do you find a code which adds pages to LRU before charge ?
> > 
> > Hmm, if there are codes which recharge the page to other memcg,
> > it will cause bug and my assumption may be harmful.
> 
> Swap slots are read optimistically into swapcache and put to the LRU,
> then charged upon fault.  

Yes, then swap charge removes page from LRU before charge.
IIUC, it needed to do so because page->mem_cgroup may be replaced.

> Fuse apparently recharges uncharged LRU pages.
Yes and No. IIUC, it was like page migraion and remove old page 
and add new page to radix-tree.

> That's why we have the lrucare stuff in the first place, no?
You're right.

> Or did I misunderstand your question?
> 

I just wondered whether you find a new one or possible user.

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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: memcg: close race between charge and putback

[Johannes Weiner]

On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 8 Sep 2011 10:54:04 +0200
> Johannes Weiner <jweiner@redhat.com> wrote:
> 
> > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > 
> > > > There is a potential race between a thread charging a page and another
> > > > thread putting it back to the LRU list:
> > > > 
> > > > charge:                         putback:
> > > > SetPageCgroupUsed               SetPageLRU
> > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > 
> > > 
> > > I assumed that all pages are charged before added to LRU.
> > > (i.e. event happens in charge->lru_lock->putback order.)
> > > 
> > > But hmm, this assumption may be bad for maintainance.
> > > Do you find a code which adds pages to LRU before charge ?
> > > 
> > > Hmm, if there are codes which recharge the page to other memcg,
> > > it will cause bug and my assumption may be harmful.
> > 
> > Swap slots are read optimistically into swapcache and put to the LRU,
> > then charged upon fault.  
> 
> Yes, then swap charge removes page from LRU before charge.
> IIUC, it needed to do so because page->mem_cgroup may be replaced.

But only from the memcg LRU.  It's still on the global per-zone LRU,
so reclaim could isolate/putback it during the charge.  And then

> > > > charge:                         putback:
> > > > SetPageCgroupUsed               SetPageLRU
> > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU

applies.

And yes, it needs to fix up *pc->mem_cgroup's LRU statistics before
the pointer get's overwritten.

Re: [patch] mm: memcg: close race between charge and putback

[Johannes Weiner]

On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 8 Sep 2011 10:54:04 +0200
> Johannes Weiner <jweiner@redhat.com> wrote:
> 
> > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > 
> > > > There is a potential race between a thread charging a page and another
> > > > thread putting it back to the LRU list:
> > > > 
> > > > charge:                         putback:
> > > > SetPageCgroupUsed               SetPageLRU
> > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > 
> > > 
> > > I assumed that all pages are charged before added to LRU.
> > > (i.e. event happens in charge->lru_lock->putback order.)
> > > 
> > > But hmm, this assumption may be bad for maintainance.
> > > Do you find a code which adds pages to LRU before charge ?
> > > 
> > > Hmm, if there are codes which recharge the page to other memcg,
> > > it will cause bug and my assumption may be harmful.
> > 
> > Swap slots are read optimistically into swapcache and put to the LRU,
> > then charged upon fault.  
> 
> Yes, then swap charge removes page from LRU before charge.
> IIUC, it needed to do so because page->mem_cgroup may be replaced.

But only from the memcg LRU.  It's still on the global per-zone LRU,
so reclaim could isolate/putback it during the charge.  And then

> > > > charge:                         putback:
> > > > SetPageCgroupUsed               SetPageLRU
> > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU

applies.

And yes, it needs to fix up *pc->mem_cgroup's LRU statistics before
the pointer get's overwritten.

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: memcg: close race between charge and putback

[KAMEZAWA Hiroyuki]

On Thu, 8 Sep 2011 11:33:16 +0200
Johannes Weiner <jweiner@redhat.com> wrote:

> On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu, 8 Sep 2011 10:54:04 +0200
> > Johannes Weiner <jweiner@redhat.com> wrote:
> > 
> > > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > 
> > > > > There is a potential race between a thread charging a page and another
> > > > > thread putting it back to the LRU list:
> > > > > 
> > > > > charge:                         putback:
> > > > > SetPageCgroupUsed               SetPageLRU
> > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > 
> > > > 
> > > > I assumed that all pages are charged before added to LRU.
> > > > (i.e. event happens in charge->lru_lock->putback order.)
> > > > 
> > > > But hmm, this assumption may be bad for maintainance.
> > > > Do you find a code which adds pages to LRU before charge ?
> > > > 
> > > > Hmm, if there are codes which recharge the page to other memcg,
> > > > it will cause bug and my assumption may be harmful.
> > > 
> > > Swap slots are read optimistically into swapcache and put to the LRU,
> > > then charged upon fault.  
> > 
> > Yes, then swap charge removes page from LRU before charge.
> > IIUC, it needed to do so because page->mem_cgroup may be replaced.
> 
> But only from the memcg LRU.  It's still on the global per-zone LRU,
> so reclaim could isolate/putback it during the charge.  And then
> 
> > > > > charge:                         putback:
> > > > > SetPageCgroupUsed               SetPageLRU
> > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> 
> applies.
> 

Hmm, in this case, I thought memcg puts back the page to its LRU by itself
under lru_loc after charge and the race was hidden.

> And yes, it needs to fix up *pc->mem_cgroup's LRU statistics before
> the pointer get's overwritten.
> 


Thanks,
-Kame

Re: [patch] mm: memcg: close race between charge and putback

[KAMEZAWA Hiroyuki]

On Thu, 8 Sep 2011 11:33:16 +0200
Johannes Weiner <jweiner@redhat.com> wrote:

> On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu, 8 Sep 2011 10:54:04 +0200
> > Johannes Weiner <jweiner@redhat.com> wrote:
> > 
> > > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > 
> > > > > There is a potential race between a thread charging a page and another
> > > > > thread putting it back to the LRU list:
> > > > > 
> > > > > charge:                         putback:
> > > > > SetPageCgroupUsed               SetPageLRU
> > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > 
> > > > 
> > > > I assumed that all pages are charged before added to LRU.
> > > > (i.e. event happens in charge->lru_lock->putback order.)
> > > > 
> > > > But hmm, this assumption may be bad for maintainance.
> > > > Do you find a code which adds pages to LRU before charge ?
> > > > 
> > > > Hmm, if there are codes which recharge the page to other memcg,
> > > > it will cause bug and my assumption may be harmful.
> > > 
> > > Swap slots are read optimistically into swapcache and put to the LRU,
> > > then charged upon fault.  
> > 
> > Yes, then swap charge removes page from LRU before charge.
> > IIUC, it needed to do so because page->mem_cgroup may be replaced.
> 
> But only from the memcg LRU.  It's still on the global per-zone LRU,
> so reclaim could isolate/putback it during the charge.  And then
> 
> > > > > charge:                         putback:
> > > > > SetPageCgroupUsed               SetPageLRU
> > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> 
> applies.
> 

Hmm, in this case, I thought memcg puts back the page to its LRU by itself
under lru_loc after charge and the race was hidden.

> And yes, it needs to fix up *pc->mem_cgroup's LRU statistics before
> the pointer get's overwritten.
> 


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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: memcg: close race between charge and putback

[Johannes Weiner]

On Thu, Sep 08, 2011 at 06:42:21PM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 8 Sep 2011 11:33:16 +0200
> Johannes Weiner <jweiner@redhat.com> wrote:
> 
> > On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Thu, 8 Sep 2011 10:54:04 +0200
> > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > 
> > > > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > 
> > > > > > There is a potential race between a thread charging a page and another
> > > > > > thread putting it back to the LRU list:
> > > > > > 
> > > > > > charge:                         putback:
> > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > > 
> > > > > 
> > > > > I assumed that all pages are charged before added to LRU.
> > > > > (i.e. event happens in charge->lru_lock->putback order.)
> > > > > 
> > > > > But hmm, this assumption may be bad for maintainance.
> > > > > Do you find a code which adds pages to LRU before charge ?
> > > > > 
> > > > > Hmm, if there are codes which recharge the page to other memcg,
> > > > > it will cause bug and my assumption may be harmful.
> > > > 
> > > > Swap slots are read optimistically into swapcache and put to the LRU,
> > > > then charged upon fault.  
> > > 
> > > Yes, then swap charge removes page from LRU before charge.
> > > IIUC, it needed to do so because page->mem_cgroup may be replaced.
> > 
> > But only from the memcg LRU.  It's still on the global per-zone LRU,
> > so reclaim could isolate/putback it during the charge.  And then
> > 
> > > > > > charge:                         putback:
> > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > 
> > applies.
> 
> Hmm, in this case, I thought memcg puts back the page to its LRU by itself
> under lru_loc after charge and the race was hidden.

But it locklessly checks PageLRU and bails if it's cleared and that is
the problem: it's not guaranteed that PageLRU is observed on the
charging CPU when the putback side bailed because of PageCgroupUsed.

My barrier puts this in order and makes sure one of the two succeeds.

Re: [patch] mm: memcg: close race between charge and putback

[Johannes Weiner]

On Thu, Sep 08, 2011 at 06:42:21PM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 8 Sep 2011 11:33:16 +0200
> Johannes Weiner <jweiner@redhat.com> wrote:
> 
> > On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Thu, 8 Sep 2011 10:54:04 +0200
> > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > 
> > > > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > 
> > > > > > There is a potential race between a thread charging a page and another
> > > > > > thread putting it back to the LRU list:
> > > > > > 
> > > > > > charge:                         putback:
> > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > > 
> > > > > 
> > > > > I assumed that all pages are charged before added to LRU.
> > > > > (i.e. event happens in charge->lru_lock->putback order.)
> > > > > 
> > > > > But hmm, this assumption may be bad for maintainance.
> > > > > Do you find a code which adds pages to LRU before charge ?
> > > > > 
> > > > > Hmm, if there are codes which recharge the page to other memcg,
> > > > > it will cause bug and my assumption may be harmful.
> > > > 
> > > > Swap slots are read optimistically into swapcache and put to the LRU,
> > > > then charged upon fault.  
> > > 
> > > Yes, then swap charge removes page from LRU before charge.
> > > IIUC, it needed to do so because page->mem_cgroup may be replaced.
> > 
> > But only from the memcg LRU.  It's still on the global per-zone LRU,
> > so reclaim could isolate/putback it during the charge.  And then
> > 
> > > > > > charge:                         putback:
> > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > 
> > applies.
> 
> Hmm, in this case, I thought memcg puts back the page to its LRU by itself
> under lru_loc after charge and the race was hidden.

But it locklessly checks PageLRU and bails if it's cleared and that is
the problem: it's not guaranteed that PageLRU is observed on the
charging CPU when the putback side bailed because of PageCgroupUsed.

My barrier puts this in order and makes sure one of the two succeeds.

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: memcg: close race between charge and putback

[KAMEZAWA Hiroyuki]

On Thu, 8 Sep 2011 11:53:49 +0200
Johannes Weiner <jweiner@redhat.com> wrote:

> On Thu, Sep 08, 2011 at 06:42:21PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu, 8 Sep 2011 11:33:16 +0200
> > Johannes Weiner <jweiner@redhat.com> wrote:
> > 
> > > On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > On Thu, 8 Sep 2011 10:54:04 +0200
> > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > 
> > > > > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > > 
> > > > > > > There is a potential race between a thread charging a page and another
> > > > > > > thread putting it back to the LRU list:
> > > > > > > 
> > > > > > > charge:                         putback:
> > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > > > 
> > > > > > 
> > > > > > I assumed that all pages are charged before added to LRU.
> > > > > > (i.e. event happens in charge->lru_lock->putback order.)
> > > > > > 
> > > > > > But hmm, this assumption may be bad for maintainance.
> > > > > > Do you find a code which adds pages to LRU before charge ?
> > > > > > 
> > > > > > Hmm, if there are codes which recharge the page to other memcg,
> > > > > > it will cause bug and my assumption may be harmful.
> > > > > 
> > > > > Swap slots are read optimistically into swapcache and put to the LRU,
> > > > > then charged upon fault.  
> > > > 
> > > > Yes, then swap charge removes page from LRU before charge.
> > > > IIUC, it needed to do so because page->mem_cgroup may be replaced.
> > > 
> > > But only from the memcg LRU.  It's still on the global per-zone LRU,
> > > so reclaim could isolate/putback it during the charge.  And then
> > > 
> > > > > > > charge:                         putback:
> > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > 
> > > applies.
> > 
> > Hmm, in this case, I thought memcg puts back the page to its LRU by itself
> > under lru_loc after charge and the race was hidden.
> 
> But it locklessly checks PageLRU and bails if it's cleared and that is

I think PageLRU check is done under zone->lru_lock. 

> the problem: it's not guaranteed that PageLRU is observed on the
> charging CPU when the putback side bailed because of PageCgroupUsed.
> 
zone->lru_lock is no help ?

> My barrier puts this in order and makes sure one of the two succeeds.
> 
Thanks,
-Kame

Re: [patch] mm: memcg: close race between charge and putback

[KAMEZAWA Hiroyuki]

On Thu, 8 Sep 2011 11:53:49 +0200
Johannes Weiner <jweiner@redhat.com> wrote:

> On Thu, Sep 08, 2011 at 06:42:21PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu, 8 Sep 2011 11:33:16 +0200
> > Johannes Weiner <jweiner@redhat.com> wrote:
> > 
> > > On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > On Thu, 8 Sep 2011 10:54:04 +0200
> > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > 
> > > > > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > > 
> > > > > > > There is a potential race between a thread charging a page and another
> > > > > > > thread putting it back to the LRU list:
> > > > > > > 
> > > > > > > charge:                         putback:
> > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > > > 
> > > > > > 
> > > > > > I assumed that all pages are charged before added to LRU.
> > > > > > (i.e. event happens in charge->lru_lock->putback order.)
> > > > > > 
> > > > > > But hmm, this assumption may be bad for maintainance.
> > > > > > Do you find a code which adds pages to LRU before charge ?
> > > > > > 
> > > > > > Hmm, if there are codes which recharge the page to other memcg,
> > > > > > it will cause bug and my assumption may be harmful.
> > > > > 
> > > > > Swap slots are read optimistically into swapcache and put to the LRU,
> > > > > then charged upon fault.  
> > > > 
> > > > Yes, then swap charge removes page from LRU before charge.
> > > > IIUC, it needed to do so because page->mem_cgroup may be replaced.
> > > 
> > > But only from the memcg LRU.  It's still on the global per-zone LRU,
> > > so reclaim could isolate/putback it during the charge.  And then
> > > 
> > > > > > > charge:                         putback:
> > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > 
> > > applies.
> > 
> > Hmm, in this case, I thought memcg puts back the page to its LRU by itself
> > under lru_loc after charge and the race was hidden.
> 
> But it locklessly checks PageLRU and bails if it's cleared and that is

I think PageLRU check is done under zone->lru_lock. 

> the problem: it's not guaranteed that PageLRU is observed on the
> charging CPU when the putback side bailed because of PageCgroupUsed.
> 
zone->lru_lock is no help ?

> My barrier puts this in order and makes sure one of the two succeeds.
> 
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: memcg: close race between charge and putback

[Johannes Weiner]

On Fri, Sep 09, 2011 at 09:28:53AM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 8 Sep 2011 11:53:49 +0200
> Johannes Weiner <jweiner@redhat.com> wrote:
> 
> > On Thu, Sep 08, 2011 at 06:42:21PM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Thu, 8 Sep 2011 11:33:16 +0200
> > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > 
> > > > On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > On Thu, 8 Sep 2011 10:54:04 +0200
> > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > 
> > > > > > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > > > 
> > > > > > > > There is a potential race between a thread charging a page and another
> > > > > > > > thread putting it back to the LRU list:
> > > > > > > > 
> > > > > > > > charge:                         putback:
> > > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > > > > 
> > > > > > > 
> > > > > > > I assumed that all pages are charged before added to LRU.
> > > > > > > (i.e. event happens in charge->lru_lock->putback order.)
> > > > > > > 
> > > > > > > But hmm, this assumption may be bad for maintainance.
> > > > > > > Do you find a code which adds pages to LRU before charge ?
> > > > > > > 
> > > > > > > Hmm, if there are codes which recharge the page to other memcg,
> > > > > > > it will cause bug and my assumption may be harmful.
> > > > > > 
> > > > > > Swap slots are read optimistically into swapcache and put to the LRU,
> > > > > > then charged upon fault.  
> > > > > 
> > > > > Yes, then swap charge removes page from LRU before charge.
> > > > > IIUC, it needed to do so because page->mem_cgroup may be replaced.
> > > > 
> > > > But only from the memcg LRU.  It's still on the global per-zone LRU,
> > > > so reclaim could isolate/putback it during the charge.  And then
> > > > 
> > > > > > > > charge:                         putback:
> > > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > 
> > > > applies.
> > > 
> > > Hmm, in this case, I thought memcg puts back the page to its LRU by itself
> > > under lru_loc after charge and the race was hidden.
> > 
> > But it locklessly checks PageLRU and bails if it's cleared and that is
> 
> I think PageLRU check is done under zone->lru_lock.

Yes, but only if a preliminary, lockless check observed PageLRU being
set:

static void mem_cgroup_lru_add_after_commit(struct page *page)
{
        unsigned long flags;
        struct zone *zone = page_zone(page);
        struct page_cgroup *pc = lookup_page_cgroup(page);
        /*
         * putback:                             charge:
         * SetPageLRU                           SetPageCgroupUsed
         * smp_mb                               smp_mb
         * PageCgroupUsed && add to memcg LRU   PageLRU && add to memcg LRU
         *
         * Ensure that one of the two sides adds the page to the memcg
         * LRU during a race.
         */
        smp_mb();
        /* taking care of that the page is added to LRU while we commit it */
        if (likely(!PageLRU(page)))
                return;
        spin_lock_irqsave(&zone->lru_lock, flags);
        /* link when the page is linked to LRU but page_cgroup isn't */
        if (PageLRU(page) && !PageCgroupAcctLRU(pc))
                mem_cgroup_add_lru_list(page, page_lru(page));
        spin_unlock_irqrestore(&zone->lru_lock, flags);
}

Without the barriers, the preliminary check may see !PageLRU while a
racing putback observed !PageCgroupUsed and nobody will add the page
to the memcg-LRU.

> > the problem: it's not guaranteed that PageLRU is observed on the
> > charging CPU when the putback side bailed because of PageCgroupUsed.
> > 
> zone->lru_lock is no help ?

Removing the preliminary check is not an option.  This is the path for
regular file cache and the common case is !PageLRU.  We do not want to
acquire the lock in estimated 99.999% of the cases.

Re: [patch] mm: memcg: close race between charge and putback

[Johannes Weiner]

On Fri, Sep 09, 2011 at 09:28:53AM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 8 Sep 2011 11:53:49 +0200
> Johannes Weiner <jweiner@redhat.com> wrote:
> 
> > On Thu, Sep 08, 2011 at 06:42:21PM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Thu, 8 Sep 2011 11:33:16 +0200
> > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > 
> > > > On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > On Thu, 8 Sep 2011 10:54:04 +0200
> > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > 
> > > > > > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > > > 
> > > > > > > > There is a potential race between a thread charging a page and another
> > > > > > > > thread putting it back to the LRU list:
> > > > > > > > 
> > > > > > > > charge:                         putback:
> > > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > > > > 
> > > > > > > 
> > > > > > > I assumed that all pages are charged before added to LRU.
> > > > > > > (i.e. event happens in charge->lru_lock->putback order.)
> > > > > > > 
> > > > > > > But hmm, this assumption may be bad for maintainance.
> > > > > > > Do you find a code which adds pages to LRU before charge ?
> > > > > > > 
> > > > > > > Hmm, if there are codes which recharge the page to other memcg,
> > > > > > > it will cause bug and my assumption may be harmful.
> > > > > > 
> > > > > > Swap slots are read optimistically into swapcache and put to the LRU,
> > > > > > then charged upon fault.  
> > > > > 
> > > > > Yes, then swap charge removes page from LRU before charge.
> > > > > IIUC, it needed to do so because page->mem_cgroup may be replaced.
> > > > 
> > > > But only from the memcg LRU.  It's still on the global per-zone LRU,
> > > > so reclaim could isolate/putback it during the charge.  And then
> > > > 
> > > > > > > > charge:                         putback:
> > > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > 
> > > > applies.
> > > 
> > > Hmm, in this case, I thought memcg puts back the page to its LRU by itself
> > > under lru_loc after charge and the race was hidden.
> > 
> > But it locklessly checks PageLRU and bails if it's cleared and that is
> 
> I think PageLRU check is done under zone->lru_lock.

Yes, but only if a preliminary, lockless check observed PageLRU being
set:

static void mem_cgroup_lru_add_after_commit(struct page *page)
{
        unsigned long flags;
        struct zone *zone = page_zone(page);
        struct page_cgroup *pc = lookup_page_cgroup(page);
        /*
         * putback:                             charge:
         * SetPageLRU                           SetPageCgroupUsed
         * smp_mb                               smp_mb
         * PageCgroupUsed && add to memcg LRU   PageLRU && add to memcg LRU
         *
         * Ensure that one of the two sides adds the page to the memcg
         * LRU during a race.
         */
        smp_mb();
        /* taking care of that the page is added to LRU while we commit it */
        if (likely(!PageLRU(page)))
                return;
        spin_lock_irqsave(&zone->lru_lock, flags);
        /* link when the page is linked to LRU but page_cgroup isn't */
        if (PageLRU(page) && !PageCgroupAcctLRU(pc))
                mem_cgroup_add_lru_list(page, page_lru(page));
        spin_unlock_irqrestore(&zone->lru_lock, flags);
}

Without the barriers, the preliminary check may see !PageLRU while a
racing putback observed !PageCgroupUsed and nobody will add the page
to the memcg-LRU.

> > the problem: it's not guaranteed that PageLRU is observed on the
> > charging CPU when the putback side bailed because of PageCgroupUsed.
> > 
> zone->lru_lock is no help ?

Removing the preliminary check is not an option.  This is the path for
regular file cache and the common case is !PageLRU.  We do not want to
acquire the lock in estimated 99.999% of the cases.

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: memcg: close race between charge and putback

[KAMEZAWA Hiroyuki]

On Fri, 9 Sep 2011 08:28:55 +0200
Johannes Weiner <jweiner@redhat.com> wrote:

> On Fri, Sep 09, 2011 at 09:28:53AM +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu, 8 Sep 2011 11:53:49 +0200
> > Johannes Weiner <jweiner@redhat.com> wrote:
> > 
> > > On Thu, Sep 08, 2011 at 06:42:21PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > On Thu, 8 Sep 2011 11:33:16 +0200
> > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > 
> > > > > On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > > On Thu, 8 Sep 2011 10:54:04 +0200
> > > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > > 
> > > > > > > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > > > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > > > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > > > > 
> > > > > > > > > There is a potential race between a thread charging a page and another
> > > > > > > > > thread putting it back to the LRU list:
> > > > > > > > > 
> > > > > > > > > charge:                         putback:
> > > > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I assumed that all pages are charged before added to LRU.
> > > > > > > > (i.e. event happens in charge->lru_lock->putback order.)
> > > > > > > > 
> > > > > > > > But hmm, this assumption may be bad for maintainance.
> > > > > > > > Do you find a code which adds pages to LRU before charge ?
> > > > > > > > 
> > > > > > > > Hmm, if there are codes which recharge the page to other memcg,
> > > > > > > > it will cause bug and my assumption may be harmful.
> > > > > > > 
> > > > > > > Swap slots are read optimistically into swapcache and put to the LRU,
> > > > > > > then charged upon fault.  
> > > > > > 
> > > > > > Yes, then swap charge removes page from LRU before charge.
> > > > > > IIUC, it needed to do so because page->mem_cgroup may be replaced.
> > > > > 
> > > > > But only from the memcg LRU.  It's still on the global per-zone LRU,
> > > > > so reclaim could isolate/putback it during the charge.  And then
> > > > > 
> > > > > > > > > charge:                         putback:
> > > > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > 
> > > > > applies.
> > > > 
> > > > Hmm, in this case, I thought memcg puts back the page to its LRU by itself
> > > > under lru_loc after charge and the race was hidden.
> > > 
> > > But it locklessly checks PageLRU and bails if it's cleared and that is
> > 
> > I think PageLRU check is done under zone->lru_lock.
> 
> Yes, but only if a preliminary, lockless check observed PageLRU being
> set:
> 
> static void mem_cgroup_lru_add_after_commit(struct page *page)
> {
>         unsigned long flags;
>         struct zone *zone = page_zone(page);
>         struct page_cgroup *pc = lookup_page_cgroup(page);
>         /*
>          * putback:                             charge:
>          * SetPageLRU                           SetPageCgroupUsed
>          * smp_mb                               smp_mb
>          * PageCgroupUsed && add to memcg LRU   PageLRU && add to memcg LRU
>          *
>          * Ensure that one of the two sides adds the page to the memcg
>          * LRU during a race.
>          */
>         smp_mb();
>         /* taking care of that the page is added to LRU while we commit it */
>         if (likely(!PageLRU(page)))
>                 return;
>         spin_lock_irqsave(&zone->lru_lock, flags);
>         /* link when the page is linked to LRU but page_cgroup isn't */
>         if (PageLRU(page) && !PageCgroupAcctLRU(pc))
>                 mem_cgroup_add_lru_list(page, page_lru(page));
>         spin_unlock_irqrestore(&zone->lru_lock, flags);
> }
> 
> Without the barriers, the preliminary check may see !PageLRU while a
> racing putback observed !PageCgroupUsed and nobody will add the page
> to the memcg-LRU.
> 

Ok, thank you for clarification. I understand the point.

Thanks,
-Kame

Re: [patch] mm: memcg: close race between charge and putback

[KAMEZAWA Hiroyuki]

On Fri, 9 Sep 2011 08:28:55 +0200
Johannes Weiner <jweiner@redhat.com> wrote:

> On Fri, Sep 09, 2011 at 09:28:53AM +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu, 8 Sep 2011 11:53:49 +0200
> > Johannes Weiner <jweiner@redhat.com> wrote:
> > 
> > > On Thu, Sep 08, 2011 at 06:42:21PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > On Thu, 8 Sep 2011 11:33:16 +0200
> > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > 
> > > > > On Thu, Sep 08, 2011 at 06:19:01PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > > On Thu, 8 Sep 2011 10:54:04 +0200
> > > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > > 
> > > > > > > On Thu, Sep 08, 2011 at 05:30:42PM +0900, KAMEZAWA Hiroyuki wrote:
> > > > > > > > On Thu,  8 Sep 2011 09:40:22 +0200
> > > > > > > > Johannes Weiner <jweiner@redhat.com> wrote:
> > > > > > > > 
> > > > > > > > > There is a potential race between a thread charging a page and another
> > > > > > > > > thread putting it back to the LRU list:
> > > > > > > > > 
> > > > > > > > > charge:                         putback:
> > > > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I assumed that all pages are charged before added to LRU.
> > > > > > > > (i.e. event happens in charge->lru_lock->putback order.)
> > > > > > > > 
> > > > > > > > But hmm, this assumption may be bad for maintainance.
> > > > > > > > Do you find a code which adds pages to LRU before charge ?
> > > > > > > > 
> > > > > > > > Hmm, if there are codes which recharge the page to other memcg,
> > > > > > > > it will cause bug and my assumption may be harmful.
> > > > > > > 
> > > > > > > Swap slots are read optimistically into swapcache and put to the LRU,
> > > > > > > then charged upon fault.  
> > > > > > 
> > > > > > Yes, then swap charge removes page from LRU before charge.
> > > > > > IIUC, it needed to do so because page->mem_cgroup may be replaced.
> > > > > 
> > > > > But only from the memcg LRU.  It's still on the global per-zone LRU,
> > > > > so reclaim could isolate/putback it during the charge.  And then
> > > > > 
> > > > > > > > > charge:                         putback:
> > > > > > > > > SetPageCgroupUsed               SetPageLRU
> > > > > > > > > PageLRU && add to memcg LRU     PageCgroupUsed && add to memcg LRU
> > > > > 
> > > > > applies.
> > > > 
> > > > Hmm, in this case, I thought memcg puts back the page to its LRU by itself
> > > > under lru_loc after charge and the race was hidden.
> > > 
> > > But it locklessly checks PageLRU and bails if it's cleared and that is
> > 
> > I think PageLRU check is done under zone->lru_lock.
> 
> Yes, but only if a preliminary, lockless check observed PageLRU being
> set:
> 
> static void mem_cgroup_lru_add_after_commit(struct page *page)
> {
>         unsigned long flags;
>         struct zone *zone = page_zone(page);
>         struct page_cgroup *pc = lookup_page_cgroup(page);
>         /*
>          * putback:                             charge:
>          * SetPageLRU                           SetPageCgroupUsed
>          * smp_mb                               smp_mb
>          * PageCgroupUsed && add to memcg LRU   PageLRU && add to memcg LRU
>          *
>          * Ensure that one of the two sides adds the page to the memcg
>          * LRU during a race.
>          */
>         smp_mb();
>         /* taking care of that the page is added to LRU while we commit it */
>         if (likely(!PageLRU(page)))
>                 return;
>         spin_lock_irqsave(&zone->lru_lock, flags);
>         /* link when the page is linked to LRU but page_cgroup isn't */
>         if (PageLRU(page) && !PageCgroupAcctLRU(pc))
>                 mem_cgroup_add_lru_list(page, page_lru(page));
>         spin_unlock_irqrestore(&zone->lru_lock, flags);
> }
> 
> Without the barriers, the preliminary check may see !PageLRU while a
> racing putback observed !PageCgroupUsed and nobody will add the page
> to the memcg-LRU.
> 

Ok, thank you for clarification. I understand the point.

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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>