Re: [PATCH 1/5] Lumpy Reclaim V3

[Andy Whitcroft <apw@shadowen.org>]

Christoph Lameter wrote:
> On Tue, 27 Feb 2007, Andy Whitcroft wrote:
> 
>> +static int __isolate_lru_page(struct page *page, int active)
>> +{
>> +	int ret = -EINVAL;
>> +
>> +	if (PageLRU(page) && (PageActive(page) == active)) {
>> +		ret = -EBUSY;
>> +		if (likely(get_page_unless_zero(page))) {
>> +			/*
>> +			 * Be careful not to clear PageLRU until after we're
>> +			 * sure the page is not being freed elsewhere -- the
>> +			 * page release code relies on it.
>> +			 */
>> +			ClearPageLRU(page);
>> +			ret = 0;
> 
> Is that really necessary? PageLRU is clear when a page is freed right? 
> And clearing PageLRU requires the zone->lru_lock since we have to move it 
> off the LRU.

Although the PageLRU is stable as we have the zone->lru_lock we cannot
take the page off the LRU unless we can take a reference to it
preventing it from being released.  The page release code relies on the
the caller who takes the reference count to zero having exclusive access
to the page to release it.  To prevent a race with
put_page/__page_cache_release we cannot touch the page once its count
drops to zero, which occurs before the PageLRU is cleared.  PageLRU
being sampled outside the zone->lru_lock in that path to avoid taking
the lock if not required.

> 
>> -			ClearPageLRU(page);
>> -			target = dst;
>> +		active = PageActive(page);
> 
> Why are we saving the active state? Page cannot be moved between LRUs 
> while we hold the lru lock anyways.

This is used later in the function for comparison against all of the
other pages in the 'order' sized area rooted about the target page.  Its
mearly an optimisation.

-apw