Re: [PATCH 29/33] autonuma: page_autonuma

[Andrea Arcangeli <aarcange@redhat.com>]

Hi KOSAKI,

On Thu, Oct 04, 2012 at 04:09:40PM -0400, KOSAKI Motohiro wrote:
> > +struct page_autonuma *lookup_page_autonuma(struct page *page)
> > +{
> > +	unsigned long pfn = page_to_pfn(page);
> > +	unsigned long offset;
> > +	struct page_autonuma *base;
> > +
> > +	base = NODE_DATA(page_to_nid(page))->node_page_autonuma;
> > +#ifdef CONFIG_DEBUG_VM
> > +	/*
> > +	 * The sanity checks the page allocator does upon freeing a
> > +	 * page can reach here before the page_autonuma arrays are
> > +	 * allocated when feeding a range of pages to the allocator
> > +	 * for the first time during bootup or memory hotplug.
> > +	 */
> > +	if (unlikely(!base))
> > +		return NULL;
> > +#endif
> 
> When using CONFIG_DEBUG_VM, please just use BUG_ON instead of additional
> sanity check. Otherwise only MM people might fault to find a real bug.

Agreed. But I just tried to stick to the page_cgroup.c model. I
suggest you send a patch to fix it in mm/page_cgroup.c, then I'll
synchronize mm/page_autonuma.c with whatever lands in page_cgroup.c.

The idea is that in the future it'd be nice to unify those with a
common implementation. And the closer page_cgroup.c and
page_autonuma.c are, the less work it'll be to update them to use a
common framework. And if it's never going to be worth it to unify it
(if it generates more code than it saves), well then keeping the code
as similar as possible, is still beneficial so it's easier to review both.

> And I have additional question here. What's happen if memory hotplug occur
> and several autonuma_last_nid will point to invalid node id? My quick skimming
> didn't find hotplug callback code.

last_nid is statistical info so if it's random it's ok (I didn't add
bugchecks to trap uninitialized cases to it, maybe I should?).

sparse_init_one_section also initializes it, and that's invoked by
sparse_add_one_section.

Also those fields are also initialized when the page is freed the
first time to add it to the buddy, but I didn't want to depend on
that, I thought an explicit init post-allocation would be more robust.

By reviewing it the only thing I found is that I was wasting a bit of
.text for 32bit builds (CONFIG_SPARSEMEM=n).

diff --git a/mm/page_autonuma.c b/mm/page_autonuma.c
index d400d7f..303b427 100644
--- a/mm/page_autonuma.c
+++ b/mm/page_autonuma.c
@@ -14,7 +14,7 @@ void __meminit page_autonuma_map_init(struct page *page,
 		page_autonuma->autonuma_last_nid = -1;
 }
 
-static void __meminit __pgdat_autonuma_init(struct pglist_data *pgdat)
+static void __paginginit __pgdat_autonuma_init(struct pglist_data *pgdat)
 {
 	spin_lock_init(&pgdat->autonuma_migrate_lock);
 	pgdat->autonuma_migrate_nr_pages = 0;
@@ -29,7 +29,7 @@ static void __meminit __pgdat_autonuma_init(struct pglist_data *pgdat)
 
 static unsigned long total_usage;
 
-void __meminit pgdat_autonuma_init(struct pglist_data *pgdat)
+void __paginginit pgdat_autonuma_init(struct pglist_data *pgdat)
 {
 	__pgdat_autonuma_init(pgdat);
 	pgdat->node_page_autonuma = NULL;
@@ -131,7 +131,7 @@ struct page_autonuma *lookup_page_autonuma(struct page *page)
 	return section->section_page_autonuma + pfn;
 }
 
-void __meminit pgdat_autonuma_init(struct pglist_data *pgdat)
+void __paginginit pgdat_autonuma_init(struct pglist_data *pgdat)
 {
 	__pgdat_autonuma_init(pgdat);
 }


So those can be freed if it's a non sparsemem build. The caller has
__paginging init too so it should be ok.


The other page_autonuma.c places invoked only by sparsemem hotplug
code are using meminit so in theory it should work (I haven't tested
it yet).

Thanks for the review!
Andrea

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>