Re: [PATCH] mm: use-once cleanup

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Fri, 2006-07-28 at 16:34 +0100, Mel Gorman wrote:
> On (17/07/06 22:40), Peter Zijlstra didst pronounce:
> > Hi,
> > 
> > This is yet another implementation of the PG_useonce cleanup spoken of
> > during the VM summit.
> > 
> > The idea is to mark a page PG_useonce on pagecache entry and modify the
> > page_referenced() check to retry on the inactive list instead of
> > promotion to the active list when PG_useonce is set. PG_useonce is
> > cleared on first use.
> > 
> 
> It wouldn't hurt to write what problem useonce fixes directly into the source
> somewhere as a comment. "Page is new to the pagecache" doesn't really say a
> whole lot and it takes some brain swizzling to see what's going on.
> 
> Disclaimer reading the patch as well. I haven't been reading through this code
> in a *long* time and I have not been reviewing  the patches that preceeded
> this one. Take the feedback with a grain of salt.

Still appreciated, time is valuable.

> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > ---
> >  include/linux/mm_inline.h  |    2 ++
> >  include/linux/page-flags.h |    5 +++++
> >  mm/filemap.c               |   22 +++++++++++++---------
> >  mm/page_alloc.c            |    9 ++++++---
> >  mm/shmem.c                 |    7 ++-----
> >  mm/swap.c                  |   11 ++---------
> >  mm/vmscan.c                |   36 +++++++++++-------------------------
> >  7 files changed, 41 insertions(+), 51 deletions(-)
> > 
> > Index: linux-2.6/include/linux/mm_inline.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/mm_inline.h	2006-07-17 22:31:06.000000000 +0200
> > +++ linux-2.6/include/linux/mm_inline.h	2006-07-17 22:35:32.000000000 +0200
> > @@ -37,5 +37,7 @@ del_page_from_lru(struct zone *zone, str
> >  	} else {
> >  		zone->nr_inactive--;
> >  	}
> > +	if (PageUseOnce(page))
> > +		ClearPageUseOnce(page);
> >  }
> >  
> > Index: linux-2.6/include/linux/page-flags.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/page-flags.h	2006-07-17 22:31:09.000000000 +0200
> > +++ linux-2.6/include/linux/page-flags.h	2006-07-17 22:39:14.000000000 +0200
> > @@ -86,6 +86,7 @@
> >  #define PG_nosave_free		18	/* Free, should not be written */
> >  #define PG_buddy		19	/* Page is free, on buddy lists */
> >  
> > +#define PG_useonce		20	/* Page is new to the pagecache */
> >  
> 
> Is there a way the useonce pages can be tracked without using another
> page bit? (I'm guessing not but extra page bits are always worth a
> stare)

't is what we've been trying to fudge for a long time now, alas no known
alternative.

> >  #if (BITS_PER_LONG > 32)
> >  /*
> > @@ -247,6 +248,10 @@
> >  #define SetPageUncached(page)	set_bit(PG_uncached, &(page)->flags)
> >  #define ClearPageUncached(page)	clear_bit(PG_uncached, &(page)->flags)
> >  
> > +#define PageUseOnce(page)	test_bit(PG_useonce, &(page)->flags)
> > +#define SetPageUseOnce(page)	set_bit(PG_useonce, &(page)->flags)
> > +#define ClearPageUseOnce(page)	clear_bit(PG_useonce, &(page)->flags)
> > +
> >  struct page;	/* forward declaration */
> >  
> >  int test_clear_page_dirty(struct page *page);
> > Index: linux-2.6/mm/filemap.c
> > ===================================================================
> > --- linux-2.6.orig/mm/filemap.c	2006-07-17 22:31:17.000000000 +0200
> > +++ linux-2.6/mm/filemap.c	2006-07-17 22:35:32.000000000 +0200
> > @@ -444,6 +444,18 @@ int add_to_page_cache(struct page *page,
> >  		error = radix_tree_insert(&mapping->page_tree, offset, page);
> >  		if (!error) {
> >  			page_cache_get(page);
> > +			/*
> > +			 * shmem_getpage()
> > +			 *   lookup_swap_cache()
> > +			 *   TestSetPageLocked()
> > +			 *   move_from_swap_cache()
> > +			 *     add_to_page_cache()
> > +			 *
> > +			 * That path calls us with a LRU page instead of a new
> > +			 * page. Don't set the hint for LRU pages.
> > +			 */
> > +			if (!PageLocked(page))
> > +				SetPageUseOnce(page);
> 
> Ok, I get this part. On insertion into the page cache, mark the page
> useonce so later we'll detect if the really useonce or not.
> 
> >  			SetPageLocked(page);
> >  			page->mapping = mapping;
> >  			page->index = offset;
> > @@ -884,7 +896,6 @@ void do_generic_mapping_read(struct addr
> >  	unsigned long offset;
> >  	unsigned long last_index;
> >  	unsigned long next_index;
> > -	unsigned long prev_index;
> >  	loff_t isize;
> >  	struct page *cached_page;
> >  	int error;
> > @@ -893,7 +904,6 @@ void do_generic_mapping_read(struct addr
> >  	cached_page = NULL;
> >  	index = *ppos >> PAGE_CACHE_SHIFT;
> >  	next_index = index;
> > -	prev_index = ra.prev_page;
> >  	last_index = (*ppos + desc->count + PAGE_CACHE_SIZE-1) >> PAGE_CACHE_SHIFT;
> >  	offset = *ppos & ~PAGE_CACHE_MASK;
> >  
> > @@ -940,13 +950,7 @@ page_ok:
> >  		if (mapping_writably_mapped(mapping))
> >  			flush_dcache_page(page);
> >  
> > -		/*
> > -		 * When (part of) the same page is read multiple times
> > -		 * in succession, only mark it as accessed the first time.
> > -		 */
> > -		if (prev_index != index)
> > -			mark_page_accessed(page);
> > -		prev_index = index;
> > +		mark_page_accessed(page);
> >  
> 
> Ok, I also get this part. Because mark_page_accessed() is now only
> setting the referenced bit, we can call it multiple times without
> messing up LRU ordering.

*nod*

> >  		/*
> >  		 * Ok, we have the page, and it's up-to-date, so
> > Index: linux-2.6/mm/page_alloc.c
> > ===================================================================
> > --- linux-2.6.orig/mm/page_alloc.c	2006-07-17 22:31:17.000000000 +0200
> > +++ linux-2.6/mm/page_alloc.c	2006-07-17 22:38:25.000000000 +0200
> > @@ -154,7 +154,8 @@ static void bad_page(struct page *page)
> >  			1 << PG_slab    |
> >  			1 << PG_swapcache |
> >  			1 << PG_writeback |
> > -			1 << PG_buddy );
> > +			1 << PG_buddy |
> > +			1 << PG_useonce);
> >  	set_page_count(page, 0);
> >  	reset_page_mapcount(page);
> >  	page->mapping = NULL;
> > @@ -389,7 +390,8 @@ static inline int free_pages_check(struc
> >  			1 << PG_swapcache |
> >  			1 << PG_writeback |
> >  			1 << PG_reserved |
> > -			1 << PG_buddy ))))
> > +			1 << PG_buddy |
> > +			1 << PF_useonce ))))
> 
> Looks like a typo there. PF_useonce is not defined anywhere.

Yeah, that's what I get for text editing a patch after testing :-(
PG_useonce was indeed intended. Already fixed in the patch I send to
Andy offlist.

> >  		bad_page(page);
> >  	if (PageDirty(page))
> >  		__ClearPageDirty(page);
> > @@ -538,7 +540,8 @@ static int prep_new_page(struct page *pa
> >  			1 << PG_swapcache |
> >  			1 << PG_writeback |
> >  			1 << PG_reserved |
> > -			1 << PG_buddy ))))
> > +			1 << PG_buddy |
> > +			1 << PG_useonce ))))
> >  		bad_page(page);
> >  
> >  	/*
> > Index: linux-2.6/mm/shmem.c
> > ===================================================================
> > --- linux-2.6.orig/mm/shmem.c	2006-07-17 22:31:17.000000000 +0200
> > +++ linux-2.6/mm/shmem.c	2006-07-17 22:35:32.000000000 +0200
> > @@ -1567,11 +1567,8 @@ static void do_shmem_file_read(struct fi
> >  			 */
> >  			if (mapping_writably_mapped(mapping))
> >  				flush_dcache_page(page);
> > -			/*
> > -			 * Mark the page accessed if we read the beginning.
> > -			 */
> > -			if (!offset)
> > -				mark_page_accessed(page);
> > +
> > +			mark_page_accessed(page);
> 
> Also fine. mark_page_accessed() will not mess up the ordering.
> 
> >  		} else {
> >  			page = ZERO_PAGE(0);
> >  			page_cache_get(page);
> > Index: linux-2.6/mm/swap.c
> > ===================================================================
> > --- linux-2.6.orig/mm/swap.c	2006-07-17 22:31:18.000000000 +0200
> > +++ linux-2.6/mm/swap.c	2006-07-17 22:35:32.000000000 +0200
> > @@ -114,19 +114,11 @@ void fastcall activate_page(struct page 
> >  
> >  /*
> >   * Mark a page as having seen activity.
> > - *
> > - * inactive,unreferenced	->	inactive,referenced
> > - * inactive,referenced		->	active,unreferenced
> > - * active,unreferenced		->	active,referenced
> >   */
> >  void fastcall mark_page_accessed(struct page *page)
> >  {
> > -	if (!PageActive(page) && PageReferenced(page) && PageLRU(page)) {
> > -		activate_page(page);
> > -		ClearPageReferenced(page);
> > -	} else if (!PageReferenced(page)) {
> > +	if (!PageReferenced(page))
> >  		SetPageReferenced(page);
> > -	}
> >  }
> 
> mark_page_accessed() now only marks a page referenced and no longer affects
> LRU ordering, that seems ok. However, the promotion from inactive to inactive
> is now taking a fairly different path and promotion may be happening much
> later if I'm reading things correctly. Promotion now takes place at either
> fault time or when scanning the lists for reclaim in shrink_page_list().
> 
> 1. When it reaches the end of the inactive list, the page is checked
> 2. If it's PG_useonce and referenced, the useonce bit is cleared and it's moved to the
>    start of the inactive list
> 3. If it's not PG_useonce but is referenced, it's moved to the active
>    list
 4. if its neither referenced or PG_useonce its a reclaim candidate and
    we proceed to try and free it.
> 
> Is that correct? 

Yes that is the intended semantics.

> It's fairly different to promotions possibly taking place
> every time mark_page_accessed() is called which a grep of mm/ implies
> can happen a lot. It may not be a problem, but it's a subtle change worth
> thinking about.

It is my understanding that mark_page_accessed() should do just that,
record that this page was touched, and that the current complications
come from the current use-once implementation.

Anyone who knows better, please speak up.

> >  
> >  EXPORT_SYMBOL(mark_page_accessed);
> > @@ -153,6 +145,7 @@ void fastcall lru_cache_add_active(struc
> >  	struct pagevec *pvec = &get_cpu_var(lru_add_active_pvecs);
> >  
> >  	page_cache_get(page);
> > +	ClearPageUseOnce(page);
> >  	if (!pagevec_add(pvec, page))
> >  		__pagevec_lru_add_active(pvec);
> >  	put_cpu_var(lru_add_active_pvecs);
> > Index: linux-2.6/mm/vmscan.c
> > ===================================================================
> > --- linux-2.6.orig/mm/vmscan.c	2006-07-17 22:31:18.000000000 +0200
> > +++ linux-2.6/mm/vmscan.c	2006-07-17 22:35:32.000000000 +0200
> > @@ -227,27 +227,6 @@ unsigned long shrink_slab(unsigned long 
> >  	return ret;
> >  }
> >  
> > -/* Called without lock on whether page is mapped, so answer is unstable */
> > -static inline int page_mapping_inuse(struct page *page)
> > -{
> > -	struct address_space *mapping;
> > -
> > -	/* Page is in somebody's page tables. */
> > -	if (page_mapped(page))
> > -		return 1;
> > -
> > -	/* Be more reluctant to reclaim swapcache than pagecache */
> > -	if (PageSwapCache(page))
> > -		return 1;
> > -
> > -	mapping = page_mapping(page);
> > -	if (!mapping)
> > -		return 0;
> > -
> > -	/* File is mmap'd by somebody? */
> > -	return mapping_mapped(mapping);
> > -}
> > -
> >  static inline int is_page_cache_freeable(struct page *page)
> >  {
> >  	return page_count(page) - !!PagePrivate(page) == 2;
> > @@ -456,8 +435,13 @@ static unsigned long shrink_page_list(st
> >  
> >  		referenced = page_referenced(page, 1);
> >  		/* In active use or really unfreeable?  Activate it. */
> > -		if (referenced && page_mapping_inuse(page))
> > +		if (referenced) {
> > +			if (PageUseOnce(page)) {
> > +				ClearPageUseOnce(page);
> > +				goto keep_locked;
> > +			}
> >  			goto activate_locked;
> > +		}
> >  
> 
> This could do with a comment explaining that this is where inactive
> pages that are not marked UseOnce but are referenced gets promoted to
> the active list.

Hmm perhaps you are right, I might have been looking at this code for
too long to not find it obvious ;-)

> 
> >  #ifdef CONFIG_SWAP
> >  		/*
> > @@ -551,6 +535,7 @@ static unsigned long shrink_page_list(st
> >  			goto keep_locked;
> >  
> >  free_it:
> > +		ClearPageUseOnce(page);
> >  		unlock_page(page);
> >  		nr_reclaimed++;
> >  		if (!pagevec_add(&freed_pvec, page))
> > @@ -724,6 +709,7 @@ static void shrink_active_list(unsigned 
> >  	struct page *page;
> >  	struct pagevec pvec;
> >  	int reclaim_mapped = 0;
> > +	int referenced;
> >  
> >  	if (sc->may_swap) {
> >  		long mapped_ratio;
> > @@ -780,10 +766,10 @@ static void shrink_active_list(unsigned 
> >  		cond_resched();
> >  		page = lru_to_page(&l_hold);
> >  		list_del(&page->lru);
> > +		referenced = page_referenced(page, 0);
> >  		if (page_mapped(page)) {
> > -			if (!reclaim_mapped ||
> > -			    (total_swap_pages == 0 && PageAnon(page)) ||
> > -			    page_referenced(page, 0)) {
> > +			if (referenced || !reclaim_mapped ||
> > +			    (total_swap_pages == 0 && PageAnon(page))) {
> >  				list_add(&page->lru, &l_active);
> >  				continue;
> >  			}
> > 
> 
> I don't get why this reordering is necessary for useonce.

You are right in that its a fairly separate issue; this change would
make the reference bit for all active pages have the same lifetime.
That is, currently mapped pages can have their reference bit stick
for several revolution of the hand. Making them appear hot longer than
intended, OTOH this whole discrimination against mapped pages makes
them last longer anyway. (I would so love to get rid of that)

Perhaps this change should go on its own. It came from the use-once
cleanup Rik did some time ago.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/