CONFIG_SPARSEMEM=y CONFIG_CGROUP_MEM_RES_CTLR=y cgroup_disable=memory
bootup is oopsing in mem_cgroup_update_mapped_file_stat(). !SPARSEMEM
is fine because its lookup_page_cgroup() contains an explicit check for
NULL node_page_cgroup, but the SPARSEMEM version was missing a check for
NULL section->page_cgroup.
Signed-off-by: Hugh Dickins <[email protected]>
---
Should go in as a fix to
memcg-add-file-based-rss-accounting.patch
but it's curious that's the first thing to suffer from this divergence.
Perhaps this is the wrong fix, and there should be an explicit
mem_cgroup_disable() check somewhere else; but it would then seem
dangerous that SPARSEMEM and !SPARSEMEM diverge in this way,
and there are lots of lookup_page_cgroup NULL tests around.
mm/page_cgroup.c | 2 ++
1 file changed, 2 insertions(+)
--- 2.6.30-rc3-mm1/mm/page_cgroup.c 2009-04-29 21:01:06.000000000 +0100
+++ mmotm/mm/page_cgroup.c 2009-04-29 21:12:04.000000000 +0100
@@ -99,6 +99,8 @@ struct page_cgroup *lookup_page_cgroup(s
unsigned long pfn = page_to_pfn(page);
struct mem_section *section = __pfn_to_section(pfn);
+ if (!section->page_cgroup)
+ return NULL;
return section->page_cgroup + pfn;
}
On Wed, 29 Apr 2009 22:13:33 +0100 (BST)
Hugh Dickins <[email protected]> wrote:
> CONFIG_SPARSEMEM=y CONFIG_CGROUP_MEM_RES_CTLR=y cgroup_disable=memory
> bootup is oopsing in mem_cgroup_update_mapped_file_stat(). !SPARSEMEM
> is fine because its lookup_page_cgroup() contains an explicit check for
> NULL node_page_cgroup, but the SPARSEMEM version was missing a check for
> NULL section->page_cgroup.
>
Ouch, it's curious this bug alive now.. thank you.
Acked-by: KAMEZAWA Hiroyuki <[email protected]>
I think this patch itself is sane but.. Balbir, could you see "caller" ?
It seems strange.
> Signed-off-by: Hugh Dickins <[email protected]>
> ---
> Should go in as a fix to
> memcg-add-file-based-rss-accounting.patch
> but it's curious that's the first thing to suffer from this divergence.
>
> Perhaps this is the wrong fix, and there should be an explicit
> mem_cgroup_disable() check somewhere else; but it would then seem
> dangerous that SPARSEMEM and !SPARSEMEM diverge in this way,
> and there are lots of lookup_page_cgroup NULL tests around.
>
> mm/page_cgroup.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> --- 2.6.30-rc3-mm1/mm/page_cgroup.c 2009-04-29 21:01:06.000000000 +0100
> +++ mmotm/mm/page_cgroup.c 2009-04-29 21:12:04.000000000 +0100
> @@ -99,6 +99,8 @@ struct page_cgroup *lookup_page_cgroup(s
> unsigned long pfn = page_to_pfn(page);
> struct mem_section *section = __pfn_to_section(pfn);
>
> + if (!section->page_cgroup)
> + return NULL;
> return section->page_cgroup + pfn;
> }
>
>
* KAMEZAWA Hiroyuki <[email protected]> [2009-04-30 09:06:46]:
> On Wed, 29 Apr 2009 22:13:33 +0100 (BST)
> Hugh Dickins <[email protected]> wrote:
>
> > CONFIG_SPARSEMEM=y CONFIG_CGROUP_MEM_RES_CTLR=y cgroup_disable=memory
> > bootup is oopsing in mem_cgroup_update_mapped_file_stat(). !SPARSEMEM
> > is fine because its lookup_page_cgroup() contains an explicit check for
> > NULL node_page_cgroup, but the SPARSEMEM version was missing a check for
> > NULL section->page_cgroup.
> >
> Ouch, it's curious this bug alive now.. thank you.
>
> Acked-by: KAMEZAWA Hiroyuki <[email protected]>
>
> I think this patch itself is sane but.. Balbir, could you see "caller" ?
> It seems strange.
Ideally we need to have a disabled check in
mem_cgroup_update_mapped_file_stat(), but it seems as if this fix is
better and fixes a larger scenario and the root cause of
lookup_page_cgroup() OOPSing. It would not hurt to check for
mem_cgroup_disabled() though, but too many checks might spoil the
party for frequent operations.
Kame, do you mean you wanted me to check if I am using
lookup_page_cgroup() correctly?
Hugh, Thank you very much for finding and fixing the problem!
Acked-by: Balbir Singh <[email protected]>
--
Balbir
On Thu, 30 Apr 2009 10:22:40 +0530
Balbir Singh <[email protected]> wrote:
> * KAMEZAWA Hiroyuki <[email protected]> [2009-04-30 09:06:46]:
>
> > On Wed, 29 Apr 2009 22:13:33 +0100 (BST)
> > Hugh Dickins <[email protected]> wrote:
> >
> > > CONFIG_SPARSEMEM=y CONFIG_CGROUP_MEM_RES_CTLR=y cgroup_disable=memory
> > > bootup is oopsing in mem_cgroup_update_mapped_file_stat(). !SPARSEMEM
> > > is fine because its lookup_page_cgroup() contains an explicit check for
> > > NULL node_page_cgroup, but the SPARSEMEM version was missing a check for
> > > NULL section->page_cgroup.
> > >
> > Ouch, it's curious this bug alive now.. thank you.
> >
> > Acked-by: KAMEZAWA Hiroyuki <[email protected]>
> >
> > I think this patch itself is sane but.. Balbir, could you see "caller" ?
> > It seems strange.
>
> Ideally we need to have a disabled check in
> mem_cgroup_update_mapped_file_stat(), but it seems as if this fix is
> better and fixes a larger scenario and the root cause of
> lookup_page_cgroup() OOPSing. It would not hurt to check for
> mem_cgroup_disabled() though, but too many checks might spoil the
> party for frequent operations.
>
> Kame, do you mean you wanted me to check if I am using
> lookup_page_cgroup() correctly?
>
Yes. I have no complaints to this patch but just curious.
Anyway thanks.
Regards,
-Kame