Messages in this thread | | | Date | Fri, 6 Jun 2008 18:04:34 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH -mm 05/25] define page_file_cache() function |
| |
On Fri, 06 Jun 2008 16:28:43 -0400 Rik van Riel <riel@redhat.com> wrote:
> From: Rik van Riel <riel@redhat.com> > > Define page_file_cache() function to answer the question: > is page backed by a file? > > Originally part of Rik van Riel's split-lru patch. Extracted > to make available for other, independent reclaim patches. > > Moved inline function to linux/mm_inline.h where it will > be needed by subsequent "split LRU" and "noreclaim" patches. > > Unfortunately this needs to use a page flag, since the > PG_swapbacked state needs to be preserved all the way > to the point where the page is last removed from the > LRU. Trying to derive the status from other info in > the page resulted in wrong VM statistics in earlier > split VM patchsets. >
argh. How many are left?
> +#ifndef LINUX_MM_INLINE_H > +#define LINUX_MM_INLINE_H > + > +/** > + * page_file_cache - should the page be on a file LRU or anon LRU? > + * @page: the page to test > + * > + * Returns !0 if @page is page cache page backed by a regular filesystem, > + * or 0 if @page is anonymous, tmpfs or otherwise ram or swap backed. > + * > + * We would like to get this info without a page flag, but the state > + * needs to survive until the page is last deleted from the LRU, which > + * could be as far down as __page_cache_release. > + */ > +static inline int page_file_cache(struct page *page) > +{ > + if (PageSwapBacked(page)) > + return 0; > + > + /* The page is page cache backed by a normal filesystem. */ > + return 2;
2?
Maybe bool would suit here.
Maybe a better name would be page_is_file_cache(). The gnu (gcc?) convention of putting _p at the end of predicate functions' names makes heaps of sense.
This function doesn't do enough stuff to do that which it says it does. There must be a whole pile of preconditions which the caller must evaluate before this function can be usefully used. I mean, it would be a bug to pass an anonymous page or a slab page or whatever into here?
> +} > + > > ... > > --- linux-2.6.26-rc2-mm1.orig/include/linux/page-flags.h 2008-05-23 14:21:21.000000000 -0400 > +++ linux-2.6.26-rc2-mm1/include/linux/page-flags.h 2008-05-23 14:21:34.000000000 -0400 > @@ -93,6 +93,7 @@ enum pageflags { > PG_mappedtodisk, /* Has blocks allocated on-disk */ > PG_reclaim, /* To be reclaimed asap */ > PG_buddy, /* Page is free, on buddy lists */ > + PG_swapbacked, /* Page is backed by RAM/swap */ > #ifdef CONFIG_IA64_UNCACHED_ALLOCATOR > PG_uncached, /* Page has been mapped as uncached */ > #endif > @@ -160,6 +161,7 @@ PAGEFLAG(Pinned, owner_priv_1) TESTSCFLA > PAGEFLAG(Reserved, reserved) __CLEARPAGEFLAG(Reserved, reserved) > PAGEFLAG(Private, private) __CLEARPAGEFLAG(Private, private) > __SETPAGEFLAG(Private, private) > +PAGEFLAG(SwapBacked, swapbacked) __CLEARPAGEFLAG(SwapBacked, swapbacked)
Those __ClearPageFoo() functions scare my pants into the next suburb. They can cause such horridly subtle bugs if misused. Every single callsite should have special attention and careful justification in comments, IMO.
> /* > * Only test-and-set exist for PG_writeback. The unconditional operators are > Index: linux-2.6.26-rc2-mm1/mm/memory.c > =================================================================== > --- linux-2.6.26-rc2-mm1.orig/mm/memory.c 2008-05-23 14:21:21.000000000 -0400 > +++ linux-2.6.26-rc2-mm1/mm/memory.c 2008-05-23 14:21:34.000000000 -0400 > @@ -1765,6 +1765,7 @@ gotten: > ptep_clear_flush(vma, address, page_table); > set_pte_at(mm, address, page_table, entry); > update_mmu_cache(vma, address, entry); > + SetPageSwapBacked(new_page); > lru_cache_add_active(new_page); > page_add_new_anon_rmap(new_page, vma, address); > > @@ -2233,6 +2234,7 @@ static int do_anonymous_page(struct mm_s > if (!pte_none(*page_table)) > goto release; > inc_mm_counter(mm, anon_rss); > + SetPageSwapBacked(page); > lru_cache_add_active(page); > page_add_new_anon_rmap(page, vma, address); > set_pte_at(mm, address, page_table, entry); > @@ -2374,6 +2376,7 @@ static int __do_fault(struct mm_struct * > set_pte_at(mm, address, page_table, entry); > if (anon) { > inc_mm_counter(mm, anon_rss); > + SetPageSwapBacked(page); > lru_cache_add_active(page); > page_add_new_anon_rmap(page, vma, address);
OK, someone lost their tab key and it wasn't you.
<does git-blame>
<blames Nick>
> } else { > > ... > > @@ -261,6 +261,7 @@ static void bad_page(struct page *page) > 1 << PG_slab | > 1 << PG_swapcache | > 1 << PG_writeback | > + 1 << PG_swapbacked | > 1 << PG_buddy ); > set_page_count(page, 0); > reset_page_mapcount(page); > @@ -494,6 +495,8 @@ static inline int free_pages_check(struc > bad_page(page); > if (PageDirty(page)) > __ClearPageDirty(page); > + if (PageSwapBacked(page)) > + __ClearPageSwapBacked(page);
OK, that one isn't so scary.
> /* > * For now, we report if PG_reserved was found set, but do not > * clear it, and do not free the page. But we shall soon need > @@ -644,6 +647,7 @@ static int prep_new_page(struct page *pa > 1 << PG_swapcache | > 1 << PG_writeback | > 1 << PG_reserved | > + 1 << PG_swapbacked | > 1 << PG_buddy )))) > bad_page(page);
| |