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 <[email protected]>
>
> 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 [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]";> [email protected] </a>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
