Re: [PATCH 01/14] mm,migration: Take a reference to the anon_vma before migrating

[Mel Gorman <mel@csn.ul.ie>]

On Tue, Apr 06, 2010 at 05:05:20PM -0700, Andrew Morton wrote:
> On Fri,  2 Apr 2010 17:02:35 +0100
> Mel Gorman <mel@csn.ul.ie> wrote:
> 
> > rmap_walk_anon() does not use page_lock_anon_vma() for looking up and
> > locking an anon_vma and it does not appear to have sufficient locking to
> > ensure the anon_vma does not disappear from under it.
> > 
> > This patch copies an approach used by KSM to take a reference on the
> > anon_vma while pages are being migrated. This should prevent rmap_walk()
> > running into nasty surprises later because anon_vma has been freed.
> > 
> 
> The code didn't exactly bend over backwards making itself easy for
> others to understand...
> 

anon_vma in general is not perfectly straight-forward. I clarify the
situation somewhat in Patch 3/14.

> > 
> > diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> > index d25bd22..567d43f 100644
> > --- a/include/linux/rmap.h
> > +++ b/include/linux/rmap.h
> > @@ -29,6 +29,9 @@ struct anon_vma {
> >  #ifdef CONFIG_KSM
> >  	atomic_t ksm_refcount;
> >  #endif
> > +#ifdef CONFIG_MIGRATION
> > +	atomic_t migrate_refcount;
> > +#endif
> 
> Some documentation here describing the need for this thing and its
> runtime semantics would be appropriate.
> 

Will come to that in Patch 3.

> >  	/*
> >  	 * NOTE: the LSB of the head.next is set by
> >  	 * mm_take_all_locks() _after_ taking the above lock. So the
> > @@ -81,6 +84,26 @@ static inline int ksm_refcount(struct anon_vma *anon_vma)
> >  	return 0;
> >  }
> >  #endif /* CONFIG_KSM */
> > +#ifdef CONFIG_MIGRATION
> > +static inline void migrate_refcount_init(struct anon_vma *anon_vma)
> > +{
> > +	atomic_set(&anon_vma->migrate_refcount, 0);
> > +}
> > +
> > +static inline int migrate_refcount(struct anon_vma *anon_vma)
> > +{
> > +	return atomic_read(&anon_vma->migrate_refcount);
> > +}
> > +#else
> > +static inline void migrate_refcount_init(struct anon_vma *anon_vma)
> > +{
> > +}
> > +
> > +static inline int migrate_refcount(struct anon_vma *anon_vma)
> > +{
> > +	return 0;
> > +}
> > +#endif /* CONFIG_MIGRATE */
> >  
> >  static inline struct anon_vma *page_anon_vma(struct page *page)
> >  {
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 6903abf..06e6316 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -542,6 +542,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
> >  	int rcu_locked = 0;
> >  	int charge = 0;
> >  	struct mem_cgroup *mem = NULL;
> > +	struct anon_vma *anon_vma = NULL;
> >  
> >  	if (!newpage)
> >  		return -ENOMEM;
> > @@ -598,6 +599,8 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
> >  	if (PageAnon(page)) {
> >  		rcu_read_lock();
> >  		rcu_locked = 1;
> > +		anon_vma = page_anon_vma(page);
> > +		atomic_inc(&anon_vma->migrate_refcount);
> 
> So no helper function for this.  I guess a grep for `migrate_refcount'
> will find it OK.
> 

It will, again I will expand on this in my response on patch 3.

> Can this count ever have a value > 1?   I guess so..
> 

KSM and migration could both conceivably take a refcount.

> >  	}
> >  
> >  	/*
> > @@ -637,6 +640,15 @@ skip_unmap:
> >  	if (rc)
> >  		remove_migration_ptes(page, page);
> >  rcu_unlock:
> > +
> > +	/* Drop an anon_vma reference if we took one */
> > +	if (anon_vma && atomic_dec_and_lock(&anon_vma->migrate_refcount, &anon_vma->lock)) {
> > +		int empty = list_empty(&anon_vma->head);
> > +		spin_unlock(&anon_vma->lock);
> > +		if (empty)
> > +			anon_vma_free(anon_vma);
> > +	}
> > +
> 
> So...  Why shouldn't this be testing ksm_refcount() too?
> 

It will in patch 3.

> Can we consolidate ksm_refcount and migrate_refcount into, err, `refcount'?
> 

Will expand on this again in the response to patch 3.

> >  	if (rcu_locked)
> >  		rcu_read_unlock();
> >  uncharge:
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index fcd593c..578d0fe 100644
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab