Re: [PATCH v4 06/10] migration: introudce migrate_ilru_pages

[Minchan Kim <minchan.kim@gmail.com>]

On Wed, Jul 27, 2011 at 01:13:57PM -0700, Andrew Morton wrote:
> On Mon,  4 Jul 2011 23:04:39 +0900
> Minchan Kim <minchan.kim@gmail.com> wrote:
> 
> > This patch defines new APIs to put back new page into old page's position as LRU order.
> > for LRU churning of compaction.
> > 
> > The idea I suggested in LSF/MM is simple.
> >
> > ...
> >
> > +static bool same_lru(struct page *page, struct page *prev)
> > +{
> > +	bool ret = false;
> > +	if (!prev || !PageLRU(prev))
> 
> Both parts of this test need explanations so readers can understand why
> they are here.
> 
> > +		goto out;
> > +
> > +	if (unlikely(PageUnevictable(prev)))
> 
> As does this.

Will do.

> 
> > +		goto out;
> > +
> > +	if (page_lru_base_type(page) != page_lru_base_type(prev))
> > +		goto out;
> 
> This (and testing for PageLRU) is the only part of this function whcih
> is sufficiently obvious to leave undocumented.

> 
> > +	ret = true;
> > +out:
> > +	return ret;
> > +}
> > +
> > +void putback_ilru_pages(struct inorder_lru *l)
> > +{
> > +	struct zone *zone;
> > +	struct page *page, *page2, *prev;
> > +
> > +	list_for_each_ilru_entry_safe(page, page2, l, ilru) {
> > +		ilru_list_del(page, l);
> > +		dec_zone_page_state(page, NR_ISOLATED_ANON +
> > +				page_is_file_cache(page));
> > +		zone = page_zone(page);
> > +		spin_lock_irq(&zone->lru_lock);
> > +		prev = page->ilru.prev_page;
> > +		if (same_lru(page, prev)) {
> > +			putback_page_to_lru(page, prev);
> > +			spin_unlock_irq(&zone->lru_lock);
> > +			put_page(page);
> > +		} else {
> > +			spin_unlock_irq(&zone->lru_lock);
> > +			putback_lru_page(page);
> > +		}
> > +	}
> > +}
> 
> This function takes lru_lock at lest once per page, up to twice per
> page.  The spinlocking frequency here could be optimised tremendously.

Yes. Mel is pointed out and I sent a [8/10] patch about it.

> 
> The trick of hanging onto zone->lru_lock is the zone didn't change gets
> hard if we want to do a put_page() inside the loop.
> 
> We have functions "putback_page_to_lru()" and "putback_lru_page()". 
> Ugh.  Can we think of better naming?

Yes. The name is bad but It's disappeared at [8/10].

> 
> Does this function even need to exist if CONFIG_MIGRATION=n?

Of course, NOT.
I will nullify it in case of no migration.

> 
> > +/*
> >   * Restore a potential migration pte to a working pte entry
> >   */
> >
> > ...
> >
> > +void __put_ilru_pages(struct page *page, struct page *newpage,
> > +		struct inorder_lru *prev_lru, struct inorder_lru *ihead)
> 
> The function name leaves me wondering where we put the pages, and
> there's no documentation telling me.

It seems remained thing to me is to add documentation.
I will add documentation in next version.

> 
> > +{
> > +	struct page *prev_page;
> > +	struct zone *zone;
> > +	prev_page = page->ilru.prev_page;
> > +	/*
> > +	 * A page that has been migrated has all references
> > +	 * removed and will be freed. A page that has not been
> > +	 * migrated will have kepts its references and be
> > +	 * restored.
> > +	 */
> > +	ilru_list_del(page, prev_lru);
> > +	dec_zone_page_state(page, NR_ISOLATED_ANON +
> > +			page_is_file_cache(page));
> > +
> > +	/*
> > +	 * Move the new page to the LRU. If migration was not successful
> > +	 * then this will free the page.
> > +	 */
> > +	zone = page_zone(newpage);
> > +	spin_lock_irq(&zone->lru_lock);
> > +	if (same_lru(page, prev_page)) {
> > +		putback_page_to_lru(newpage, prev_page);
> > +		spin_unlock_irq(&zone->lru_lock);
> > +		/*
> > +		 * The newpage replaced LRU position of old page and
> > +		 * old one would be freed. So let's adjust prev_page of pages
> > +		 * remained in inorder_lru list.
> > +		 */
> > +		adjust_ilru_prev_page(ihead, page, newpage);
> > +		put_page(newpage);
> > +	} else {
> > +		spin_unlock_irq(&zone->lru_lock);
> > +		putback_lru_page(newpage);
> > +	}
> 
> The same spinlocking frequency issue.
> 
> > +	putback_lru_page(page);
> > +}
> > +
> >
> > ...
> >
> > +int migrate_ilru_pages(struct inorder_lru *ihead, new_page_t get_new_page,
> > +		unsigned long private, bool offlining, bool sync)
> > +{
> > +	int retry = 1;
> > +	int nr_failed = 0;
> > +	int pass = 0;
> > +	struct page *page, *page2;
> > +	struct inorder_lru *prev;
> > +	int swapwrite = current->flags & PF_SWAPWRITE;
> > +	int rc;
> > +
> > +	if (!swapwrite)
> > +		current->flags |= PF_SWAPWRITE;
> > +
> > +	for (pass = 0; pass < 10 && retry; pass++) {
> 
> That ten-passes thing was too ugly to live, and now it's breeding.  Argh.

Personally, I hope we remove it.
I will consider it later version or as independent patch.
Thanks for the review, Andrew!
-- 
Kind regards,
Minchan Kim