Re: [PATCH v6 1/3] mm: introduce compaction and migration for virtioballooned pages

[Mel Gorman]

On Wed, Aug 08, 2012 at 07:53:19PM -0300, Rafael Aquini wrote:
> Memory fragmentation introduced by ballooning might reduce significantly
> the number of 2MB contiguous memory blocks that can be used within a guest,
> thus imposing performance penalties associated with the reduced number of
> transparent huge pages that could be used by the guest workload.
> 
> This patch introduces the helper functions as well as the necessary changes
> to teach compaction and migration bits how to cope with pages which are
> part of a guest memory balloon, in order to make them movable by memory
> compaction procedures.
> 
> Signed-off-by: Rafael Aquini <aquini@xxxxxxxxxx>

Mostly looks ok but I have one question;

> <SNIP>
>
> +/* putback_lru_page() counterpart for a ballooned page */
> +bool putback_balloon_page(struct page *page)
> +{
> +	if (WARN_ON(!movable_balloon_page(page)))
> +		return false;
> +
> +	if (likely(trylock_page(page))) {
> +		if (movable_balloon_page(page)) {
> +			__putback_balloon_page(page);
> +			put_page(page);
> +			unlock_page(page);
> +			return true;
> +		}
> +		unlock_page(page);
> +	}

You might have answered this already as I skipped over a few revisions
and if you have, sorry about that and please add a comment :)

This trylock_page looks risky as it looks like it can fail if another
process running compaction tries to isolate this page. It locks the page,
finds it cant and releases the lock but in the meantime this trylock can
fail. It triggers a WARN_ON so we'll get a bug report but it leaves the
reference count elevated and this page has now leaked.

Why not just lock_page(page)? As you have already isolated this page you
know that the lock is only going to be held by a parallel compacting
process checking the reference count and the delay will be short. As a
bonus you can drop the WARN_ON check in the caller and make this void as
the WARN_ON check in the caller becomes redundant.

-- 
Mel Gorman
SUSE Labs
--
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/