Re: [patch 3/3] mm: PageActive no testset

[Marcelo Tosatti <marcelo.tosatti@cyclades.com>]

Hi Nick,

On Wed, Jan 18, 2006 at 11:40:58AM +0100, Nick Piggin wrote:
> PG_active is protected by zone->lru_lock, it does not need TestSet/TestClear
> operations.

page->flags bits (including PG_active and PG_lru bits) are touched by
several codepaths which do not hold zone->lru_lock. 

AFAICT zone->lru_lock guards access to the LRU list, and no more than
that.

Moreover, what about consistency of the rest of page->flags bits?

PPC for example implements test_and_set_bit() with:

	lwarx	reg, addr   (load and create reservation for 32-bit addr)
	or 	reg, BITOP_MASK(nr)	
	stwcx	reg, addr  (store word upon reservation validation, otherwise loop)

If you don't use atomic operations on page->flags, unrelated bits other
than that you're working with can have their updates lost, given that 
in reality page->flags is not protected by the lru_lock.

For example:

/*
 * shrink_list adds the number of reclaimed pages to sc->nr_reclaimed
 */
static int shrink_list(struct list_head *page_list, struct scan_control *sc)
{
...
                BUG_ON(PageActive(page));
...
activate_locked:
                SetPageActive(page);
                pgactivate++;
keep_locked:
                unlock_page(page);
keep:
                list_add(&page->lru, &ret_pages);
                BUG_ON(PageLRU(page));
}

And recently:

#ifdef CONFIG_MIGRATION
static inline void move_to_lru(struct page *page)
{
        list_del(&page->lru);
        if (PageActive(page)) {
                /*
                 * lru_cache_add_active checks that
                 * the PG_active bit is off.
                 */ 
                ClearPageActive(page);
                lru_cache_add_active(page);

Not relying on zone->lru_lock allows interesting optimizations
such as moving active/inactive pgflag bit setting from inside
__pagevec_lru_add/__pagevec_lru_add_active to the caller, and merging
the two.

Comments?


> Signed-off-by: Nick Piggin <npiggin@suse.de>
> 
> Index: linux-2.6/mm/vmscan.c
> ===================================================================
> --- linux-2.6.orig/mm/vmscan.c
> +++ linux-2.6/mm/vmscan.c
> @@ -997,8 +997,9 @@ refill_inactive_zone(struct zone *zone, 
>  		prefetchw_prev_lru_page(page, &l_inactive, flags);
>  		BUG_ON(PageLRU(page));
>  		SetPageLRU(page);
> -		if (!TestClearPageActive(page))
> -			BUG();
> +		BUG_ON(!PageActive(page));
> +		ClearPageActive(page);
> +
>  		list_move(&page->lru, &zone->inactive_list);
>  		pgmoved++;
>  		if (!pagevec_add(&pvec, page)) {
> Index: linux-2.6/mm/swap.c
> ===================================================================
> --- linux-2.6.orig/mm/swap.c
> +++ linux-2.6/mm/swap.c
> @@ -356,8 +356,8 @@ void __pagevec_lru_add_active(struct pag
>  		}
>  		BUG_ON(PageLRU(page));
>  		SetPageLRU(page);
> -		if (TestSetPageActive(page))
> -			BUG();
> +		BUG_ON(PageActive(page));
> +		SetPageActive(page);
>  		add_page_to_active_list(zone, page);
>  	}
>  	if (zone)
> Index: linux-2.6/include/linux/page-flags.h
> ===================================================================
> --- linux-2.6.orig/include/linux/page-flags.h
> +++ linux-2.6/include/linux/page-flags.h
> @@ -251,8 +251,6 @@ extern void __mod_page_state_offset(unsi
>  #define PageActive(page)	test_bit(PG_active, &(page)->flags)
>  #define SetPageActive(page)	set_bit(PG_active, &(page)->flags)
>  #define ClearPageActive(page)	clear_bit(PG_active, &(page)->flags)
> -#define TestClearPageActive(page) test_and_clear_bit(PG_active, &(page)->flags)
> -#define TestSetPageActive(page) test_and_set_bit(PG_active, &(page)->flags)
>  
>  #define PageSlab(page)		test_bit(PG_slab, &(page)->flags)
>  #define SetPageSlab(page)	set_bit(PG_slab, &(page)->flags)
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>