lkml.org 
[lkml]   [2008]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch v2] vmscan: protect zone rotation stats by lru lock
On Mon, 01 Dec 2008 03:00:35 +0100
Johannes Weiner <hannes@saeurebad.de> wrote:

> The zone's rotation statistics must not be accessed without the
> corresponding LRU lock held. Fix an unprotected write in
> shrink_active_list().
>

I don't think it really matters. It's quite common in that code to do
unlocked, racy update to statistics such as this. Because on those
rare occasions where a race does happen, there's a small glitch in the
reclaim logic which nobody will notice anyway.

Of course, this does need to be done with some care, to ensure the
glitch _will_ be small. If such a race would cause the scanner to go
off and reclaim 2^32 pages, well, that's not so good.

>
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1243,32 +1243,32 @@ static void shrink_active_list(unsigned
> /* page_referenced clears PageReferenced */
> if (page_mapping_inuse(page) &&
> page_referenced(page, 0, sc->mem_cgroup))
> pgmoved++;
>
> list_add(&page->lru, &l_inactive);
> }
>
> + spin_lock_irq(&zone->lru_lock);
> /*
> * Count referenced pages from currently used mappings as
> * rotated, even though they are moved to the inactive list.
> * This helps balance scan pressure between file and anonymous
> * pages in get_scan_ratio.
> */
> zone->recent_rotated[!!file] += pgmoved;
>
> /*
> * Move the pages to the [file or anon] inactive list.
> */
> pagevec_init(&pvec, 1);
>
> pgmoved = 0;
> lru = LRU_BASE + file * LRU_FILE;
> - spin_lock_irq(&zone->lru_lock);

We've unnecessarily moved a pile of other things inside the locked
region as well, needlessly extending the lock hold times.

> while (!list_empty(&l_inactive)) {
> page = lru_to_page(&l_inactive);
> prefetchw_prev_lru_page(page, &l_inactive, flags);
> VM_BUG_ON(PageLRU(page));
> SetPageLRU(page);
> VM_BUG_ON(!PageActive(page));
> ClearPageActive(page);
>

You'll note that the code which _uses_ these values does so without
holding the lock. So get_scan_ratio() sees incoherent values of
recent_scanned[0] and recent_scanned[1]. As is common in this code,
that is OK and deliberate.

It's also racy here:

if (unlikely(zone->recent_scanned[0] > anon / 4)) {
spin_lock_irq(&zone->lru_lock);
zone->recent_scanned[0] /= 2;
zone->recent_rotated[0] /= 2;
spin_unlock_irq(&zone->lru_lock);
}

failing to recheck the comparison after taking the lock..


\
 
 \ /
  Last update: 2008-12-01 22:45    [W:2.453 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site