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

[Andrew Morton]

On Mon,  4 Jul 2011 23:04:39 +0900
Minchan Kim <minchan.kim@xxxxxxxxx> 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.

> +		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.

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?

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

> +/*
>   * 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.

> +{
> +	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.

> +		retry = 0;
> +		prev = ihead;
> +		list_for_each_ilru_entry_safe(page, page2, ihead, ilru) {
> +			cond_resched();
> +
> +			rc = unmap_and_move_ilru(get_new_page, private,
> +					page, pass > 2, offlining,
> +					sync, prev, ihead);
> +
> +			switch (rc) {
> +			case -ENOMEM:
> +				goto out;
> +			case -EAGAIN:
> +				retry++;
> +				prev = &page->ilru;
> +				break;
> +			case 0:
> +				break;
> +			default:
> +				/* Permanent failure */
> +				nr_failed++;
> +				break;
> +			}
> +		}
> +	}
> +	rc = 0;
> +out:
> +	if (!swapwrite)
> +		current->flags &= ~PF_SWAPWRITE;
> +
> +	if (rc)
> +		return rc;
> +
> +	return nr_failed + retry;
> +}
> +
>
> ...
>

--
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/