Re: [PATCH V2 2/7] mm: move MADV_FREE pages into LRU_INACTIVE_FILE list

[Minchan Kim]

Hi Shaohua,

On Fri, Feb 10, 2017 at 09:30:09AM -0800, Shaohua Li wrote:

< snip >

> > > +static inline bool page_is_lazyfree(struct page *page)
> > > +{
> > > +	return PageAnon(page) && !PageSwapBacked(page);
> > > +}
> > > +
> > 
> > trivial:
> > 
> > How about using PageLazyFree for consistency with other PageXXX?
> > As well, use SetPageLazyFree/ClearPageLazyFree rather than using
> > raw {Set,Clear}PageSwapBacked.
> 
> So SetPageLazyFree == ClearPageSwapBacked, that would be weird. I personally
> prefer directly using {Set, Clear}PageSwapBacked, because reader can
> immediately know what's happening. If using the PageLazyFree, people always
> need to refer the code and check the relationship between PageLazyFree and
> PageSwapBacked.

I was not against so I was about to sending "No problem" now but I found your
patch 5 which accounts lazyfreeable pages in zone/node stat and handle them
in lru list management. Hmm, I think now we don't handle lazyfree pages with
separate LRU list so it's awkward to me although it may work. So, my idea is
we can handle it through wrapper regardless of LRU management.

For instance,

void SetLazyFreePage(struct page *page)
{
	if (!TestSetPageSwapBacked(page))
		inc_zone_page_state(page, NR_ZONE_LAZYFREE);
}


void ClearLazyFreePage(struct page *page)
{
	if (TestClearPageSwapBacked(page))
		dec_zone_page_state(page, NR_ZONE_LAZYFREE);
}

madvise_free_pte_range:
	SetLageFreePage(page);

activate_page,shrink_page_list:
	ClearLazyFreePage(page);

free_pages_prepare:
	if (PageMappingFlags(page)) {
		if (PageLazyFreePage(page))
			dec_zone_page_state(page, NR_ZONE_LAZYFREE);
		page->mapping = NULL;
	}

Surely, it's orthgonal issue regardless of using wrapper but it might
nudge you to use wrapper.

>  
> > >  static __always_inline void __update_lru_size(struct lruvec *lruvec,
> > >  				enum lru_list lru, enum zone_type zid,
> > >  				int nr_pages)
> > > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > > index 45e91dd..486494e 100644
> > > --- a/include/linux/swap.h
> > > +++ b/include/linux/swap.h
> > > @@ -279,7 +279,7 @@ extern void lru_add_drain_cpu(int cpu);
> > >  extern void lru_add_drain_all(void);
> > >  extern void rotate_reclaimable_page(struct page *page);
> > >  extern void deactivate_file_page(struct page *page);
> > > -extern void deactivate_page(struct page *page);
> > > +extern void mark_page_lazyfree(struct page *page);
> > 
> > trivial:
> > 
> > How about "deactivate_lazyfree_page"? IMO, it would show intention
> > clear that move the lazy free page to inactive list.
> > 
> > It's just matter of preference so I'm not strong against.
> 
> Yes, I thought about the name a little bit. Don't think we should use
> deactivate, because it sounds that only works for active page, while the
> function works for both active/inactive pages. I'm open to any suggestions.

Indeed.

I don't have better idea, either so my last suggestion is "demote_lazyfree_page".
It seems there are several papers/wikipedia to use *demote* in LRU managment.