Re: [PATCH 30/35] autonuma: reset autonuma page data when pages arefreed

[Andrea Arcangeli]

On Tue, May 29, 2012 at 06:30:29PM +0200, Peter Zijlstra wrote:
> On Fri, 2012-05-25 at 19:02 +0200, Andrea Arcangeli wrote:
> > When pages are freed abort any pending migration. If knuma_migrated
> > arrives first it will notice because get_page_unless_zero would fail.
> 
> But knuma_migrated can run on a different cpu than this free is
> happening, ACCESS_ONCE() won't cure that.

knuma_migrated won't alter the last_nid and it generally won't work on
any page that has page_count() = 0.

last_nid is the false sharing avoidance information (btw, that really
better exist for every page, unlike the list node, which might be
limited maybe).

Then there's a second false sharing avoidance through the implicit
properties of the autonuma_migrate_head lrus and the
migration-cancellation in numa_hinting_fault_memory_follow_cpu (which
is why I wouldn't like the idea of an insert-only list, even if it
would save a pointer per page, but then I couldn't cancel the
migration when a false sharing is detected and knuma_migrated is
congested).

> What's that ACCESS_ONCE() good for?

The ACCESS_ONCE was used when setting last_nid, to tell gcc the value
can change from under it. It shouldn't alter the code emitted here and
probably it's superfluous in any case.

But considering that the page is being freed, I don't think it can
change from under us here so this was definitely superflous, numa
hinting page faults can't run on that page. I will remove it, thanks!

> 
> Also, you already have an autonuma_ hook right there, why add more
> #ifdeffery ?

Agreed, the #ifdef is in fact already cleaned up in page_autonuma,
with autonuma_page_free.

-       autonuma_migrate_page_remove(page);
-#ifdef CONFIG_AUTONUMA
-       ACCESS_ONCE(page->autonuma_last_nid) = -1;
-#endif
+       autonuma_free_page(page);

> 
> > Signed-off-by: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> > ---
> >  mm/page_alloc.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 3d1ee70..1d3163f 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -614,6 +614,10 @@ static inline int free_pages_check(struct page *page)
> >  		bad_page(page);
> >  		return 1;
> >  	}
> > +	autonuma_migrate_page_remove(page);
> > +#ifdef CONFIG_AUTONUMA
> > +	ACCESS_ONCE(page->autonuma_last_nid) = -1;
> > +#endif
> >  	if (page->flags & PAGE_FLAGS_CHECK_AT_PREP)
> >  		page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
> >  	return 0;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/