meminfo returns inaccurate NR_FILE_PAGES

meminfo returns inaccurate NR_FILE_PAGES

[Ethan Solomita]

Node 2 MemTotal:        64640 kB
Node 2 MemFree:         59816 kB
Node 2 MemUsed:          4824 kB
Node 2 Active:              0 kB
Node 2 Inactive:            8 kB
Node 2 HighTotal:           0 kB
Node 2 HighFree:            0 kB
Node 2 LowTotal:        64640 kB
Node 2 LowFree:         59816 kB
Node 2 Dirty:               0 kB
Node 2 Writeback:           0 kB
Node 2 FilePages:       62040 kB
Node 2 Mapped:              8 kB
Node 2 AnonPages:           0 kB
Node 2 PageTables:          0 kB
Node 2 NFS_Unstable:        0 kB
Node 2 Bounce:              0 kB
Node 2 Slab:             4696 kB
Node 2 HugePages_Total:     0
Node 2 HugePages_Free:      0

   
    Note that File Pages is 62040kB when MemUsed is only 4824kB. We do 
__(dec|inc)_zone_page_state(page, NR_FILE_PAGES) whenever doing a 
radix_tree_(delete|insert) from/to mapping->page_tree. Except we missed one:

migrate.c:migrate_page_move_mapping()

    Here we replace the page* in the radix tree, but we don't dec on the 
old page and add on the new. Bug fix -- add:

__dec_zone_page_state(page, NR_FILE_PAGES)
__inc_zone_page_state(newpage, NR_FILE_PAGES)

    into migrate_page_move_mapping() immediately after writing to 
radix_pointer.

    If I get agreement that this is a bug I'll write up a patch.
    -- Ethan

--
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>

Re: meminfo returns inaccurate NR_FILE_PAGES

[Christoph Lameter]

On Tue, 17 Apr 2007, Ethan Solomita wrote:

>      Note that File Pages is 62040kB when MemUsed is only 4824kB. We do
> __(dec|inc)_zone_page_state(page, NR_FILE_PAGES) whenever doing a
> radix_tree_(delete|insert) from/to mapping->page_tree. Except we missed one:

Right. Sigh. Does this fix it?

Fix NR_FILE_PAGES and NR_ANON_PAGES accounting.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6.21-rc6/mm/migrate.c
===================================================================
--- linux-2.6.21-rc6.orig/mm/migrate.c	2007-04-17 14:15:45.000000000 -0700
+++ linux-2.6.21-rc6/mm/migrate.c	2007-04-17 14:34:09.000000000 -0700
@@ -579,9 +579,21 @@ static int move_to_new_page(struct page 
 	else
 		rc = fallback_migrate_page(mapping, newpage, page);
 
-	if (!rc)
+	if (!rc) {
+		/*
+		 * If moved to a different zone then also account
+		 * the page for that zone. Other VM counters will be
+		 * taken care of when we establish references to the
+		 * new page and drop references to the old page.
+		 */
+		if (page_zone(newpage) != page_zone(page)) {
+			int counter = PageAnon(page) ? NR_ANON_PAGES : NR_FILE_PAGES;
+
+			dec_zone_page_state(page, counter);
+			inc_zone_page_state(newpage, counter);
+		}
 		remove_migration_ptes(page, newpage);
-	else
+	} else
 		newpage->mapping = NULL;
 
 	unlock_page(newpage);

--
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>

Re: meminfo returns inaccurate NR_FILE_PAGES

[Ethan Solomita]

Christoph Lameter wrote:
> On Tue, 17 Apr 2007, Ethan Solomita wrote:
>
>   
>>      Note that File Pages is 62040kB when MemUsed is only 4824kB. We do
>> __(dec|inc)_zone_page_state(page, NR_FILE_PAGES) whenever doing a
>> radix_tree_(delete|insert) from/to mapping->page_tree. Except we missed one:
>>     
>
> Right. Sigh. Does this fix it?
>
> Fix NR_FILE_PAGES and NR_ANON_PAGES accounting.
>   

    I don't think that there's a problem with NR_ANON_PAGES. 
unmap_and_move(), the caller of move_to_new_page(), calls try_to_unmap() 
which calls try_to_unmap_anon() which calls try_to_unmap_one() which 
calls page_remove_rmap() which in turn makes the call to 
__dec_zone_page_state. i.e. the rmap() code is handling NR_ANON_PAGES 
and NR_FILE_MAPPED pages correctly. It's just the NR_FILE_PAGES which 
are tied to the mapping's page tree, where the problem lies.
    -- Ethan

--
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>

Re: meminfo returns inaccurate NR_FILE_PAGES

[Christoph Lameter]

On Tue, 17 Apr 2007, Ethan Solomita wrote:

> > Fix NR_FILE_PAGES and NR_ANON_PAGES accounting.
> >   
> 
>    I don't think that there's a problem with NR_ANON_PAGES. unmap_and_move(),
> the caller of move_to_new_page(), calls try_to_unmap() which calls
> try_to_unmap_anon() which calls try_to_unmap_one() which calls
> page_remove_rmap() which in turn makes the call to __dec_zone_page_state. i.e.
> the rmap() code is handling NR_ANON_PAGES and NR_FILE_MAPPED pages correctly.

Hmmmm...... Ok I see that NR_ANON_PAGES is decremented. But where does 
NR_ANON_PAGES get incremented for the new zone? Ahh in page_add_anon_rmap. 
So that is fine the same way as NR_FILE_MAPPED.

> It's just the NR_FILE_PAGES which are tied to the mapping's page tree, where
> the problem lies.

Ah. I see.

However, anonymous pages may also have a mapping (swap). So we need to 
check first that it is not an anonymous page and then eventually shift 
the count between zones.

Do you think this is right?


Index: linux-2.6.21-rc6/mm/migrate.c
===================================================================
--- linux-2.6.21-rc6.orig/mm/migrate.c	2007-04-17 17:01:58.000000000 -0700
+++ linux-2.6.21-rc6/mm/migrate.c	2007-04-17 22:08:22.000000000 -0700
@@ -333,6 +333,17 @@ static int migrate_page_move_mapping(str
 	 */
 	__put_page(page);
 
+	/*
+	 * If moved to a different zone then also account
+	 * the page for that zone. Other VM counters will be
+	 * taken care of when we establish references to the
+	 * new page and drop references to the old page.
+	 */
+	if (page_zone(newpage) != page_zone(page) && !PageAnon(page)) {
+		__dec_zone_page_state(page, NR_FILE_PAGES);
+		__inc_zone_page_state(newpage, NR_FILE_PAGES);
+	}
+
 	write_unlock_irq(&mapping->tree_lock);
 
 	return 0;

--
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>

Re: meminfo returns inaccurate NR_FILE_PAGES

[Ethan Solomita]

Christoph Lameter wrote:
>> It's just the NR_FILE_PAGES which are tied to the mapping's page tree, where
>> the problem lies.
>>     
>
> Ah. I see.
>
> However, anonymous pages may also have a mapping (swap). So we need to 
> check first that it is not an anonymous page and then eventually shift 
> the count between zones.
>   

    Anonymous pages have a value in mapping, but it's not a struct 
address_space, it's a struct vm_area_struct (+1). The NR_FILE_PAGES 
count is incremented and decremented only when something is added to or 
removed from an address_space's page_table as pointed to by a mapping. 
This is only done in filemap.c, except for this one example in migrate.c 
that changes the radix table's page pointer in place. I think that all 
that is needed is an extra set of lines in migrate_page_move_mapping() 
after modifying *radix_pointer to call __dec on the old page and __inc 
on the new. You can check the zones first if you'd like to save effort, 
although I'm not sure it's a big deal since the __dec and __inc 
functions are only modifying per-cpu accumulation variables.
    -- Ethan

--
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>

Re: meminfo returns inaccurate NR_FILE_PAGES

[Christoph Lameter]

On Tue, 17 Apr 2007, Ethan Solomita wrote:

>    Anonymous pages have a value in mapping, but it's not a struct
> address_space, it's a struct vm_area_struct (+1). The NR_FILE_PAGES count is

Wrong. Anonymous pages can be a part of swap space which is an 
address_space.

from include/linux/mm.h

extern struct address_space swapper_space;
static inline struct address_space *page_mapping(struct page *page)
{
        struct address_space *mapping = page->mapping;

        if (unlikely(PageSwapCache(page)))
                mapping = &swapper_space;
        else if (unlikely((unsigned long)mapping & PAGE_MAPPING_ANON))
                mapping = NULL;
        return mapping;
}

> of lines in migrate_page_move_mapping() after modifying *radix_pointer to call
> __dec on the old page and __inc on the new. You can check the zones first if
> you'd like to save effort, although I'm not sure it's a big deal since the
> __dec and __inc functions are only modifying per-cpu accumulation variables.

Ok. That is what the patch does. So please test the patch and get back 
to me.

--
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>

Re: meminfo returns inaccurate NR_FILE_PAGES

[Ethan Solomita]

Christoph Lameter wrote:
> On Tue, 17 Apr 2007, Ethan Solomita wrote:
>
>   
>>    Anonymous pages have a value in mapping, but it's not a struct
>> address_space, it's a struct vm_area_struct (+1). The NR_FILE_PAGES count is
>>     
>
> Wrong. Anonymous pages can be a part of swap space which is an 
> address_space.
>
> from include/linux/mm.h
>
> extern struct address_space swapper_space;
> static inline struct address_space *page_mapping(struct page *page)
> {
>         struct address_space *mapping = page->mapping;
>
>         if (unlikely(PageSwapCache(page)))
>                 mapping = &swapper_space;
>         else if (unlikely((unsigned long)mapping & PAGE_MAPPING_ANON))
>                 mapping = NULL;
>         return mapping;
> }
>   

    While you're busy correcting me, look in swap_state.c at 
__add_to_swap_cache(). Note how, when it inserts a page into 
swapper_space.page_tree, it then does an 
__inc_zone_page_state(NR_FILE_PAGES). Going back to my initial email 
reporting the bug you'll see that I make it clear: whenever a page is 
inserted into a mapping's page_tree we increment NR_FILE_PAGES.

    My comment above was meant to refer to anonymous mappings ala 
PAGE_MAPPING_ANON.

>> of lines in migrate_page_move_mapping() after modifying *radix_pointer to call
>> __dec on the old page and __inc on the new. You can check the zones first if
>> you'd like to save effort, although I'm not sure it's a big deal since the
>> __dec and __inc functions are only modifying per-cpu accumulation variables.
>>     
>
> Ok. That is what the patch does. So please test the patch and get back 
> to me.
>   

    I'll test it when it works, i.e. when you remove the check for 
PAGE_ANON.  There is a one-to-one correspondence -- except in migrate.c 
-- of adding/removing a page from *ANY* page_tree and inc/dec'ing 
NR_FILE_PAGES. There's no reason for migrate to make an exception and 
check for PAGE_ANON.
    -- Ethan

--
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>

Re: meminfo returns inaccurate NR_FILE_PAGES

[Christoph Lameter]

On Tue, 17 Apr 2007, Ethan Solomita wrote:

>    While you're busy correcting me, look in swap_state.c at
> __add_to_swap_cache(). Note how, when it inserts a page into
> swapper_space.page_tree, it then does an __inc_zone_page_state(NR_FILE_PAGES).

Correct. So a page is accounted for both as anonymous and a file pages. 
That is surprising. So this patch should indeed work. Added some comments
to clarify the situation.

Index: linux-2.6.21-rc6/mm/migrate.c
===================================================================
--- linux-2.6.21-rc6.orig/mm/migrate.c	2007-04-17 22:10:33.000000000 -0700
+++ linux-2.6.21-rc6/mm/migrate.c	2007-04-18 12:34:19.000000000 -0700
@@ -297,7 +297,7 @@ static int migrate_page_move_mapping(str
 	void **pslot;
 
 	if (!mapping) {
-		/* Anonymous page */
+		/* Anonymous page without mapping */
 		if (page_count(page) != 1)
 			return -EAGAIN;
 		return 0;
@@ -333,6 +333,19 @@ static int migrate_page_move_mapping(str
 	 */
 	__put_page(page);
 
+	/*
+	 * If moved to a different zone then also account
+	 * the page for that zone. Other VM counters will be
+	 * taken care of when we establish references to the
+	 * new page and drop references to the old page.
+	 *
+	 * Note that anonymous pages are accounted for
+	 * via NR_FILE_PAGES and NR_ANON_PAGES if they
+	 * are mapped to swap space.
+	 */
+	__dec_zone_page_state(page, NR_FILE_PAGES);
+	__inc_zone_page_state(newpage, NR_FILE_PAGES);
+
 	write_unlock_irq(&mapping->tree_lock);
 
 	return 0;

--
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>

Re: meminfo returns inaccurate NR_FILE_PAGES

[Ethan Solomita]

Christoph Lameter wrote:
> On Tue, 17 Apr 2007, Ethan Solomita wrote:
>
>   
>>    While you're busy correcting me, look in swap_state.c at
>> __add_to_swap_cache(). Note how, when it inserts a page into
>> swapper_space.page_tree, it then does an __inc_zone_page_state(NR_FILE_PAGES).
>>     
>
> Correct. So a page is accounted for both as anonymous and a file pages. 
> That is surprising. So this patch should indeed work. Added some comments
> to clarify the situation.
>   

    Given that it's exactly what I suggested in my first post, clearly I 
second your patch.
    -- Ethan

--
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>

Re: meminfo returns inaccurate NR_FILE_PAGES

[Christoph Lameter]

On Wed, 18 Apr 2007, Ethan Solomita wrote:

>    Given that it's exactly what I suggested in my first post, clearly I second
> your patch.

I will make sure it gets applied.

--
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>

Re: meminfo returns inaccurate NR_FILE_PAGES

[Ethan Solomita]

Christoph Lameter wrote:
> Fix NR_FILE_PAGES and NR_ANON_PAGES accounting.
>   

    One other thing -- I think you're confusing NR_FILE_PAGES with 
NR_FILE_MAPPED. Either NR_FILE_MAPPED or NR_ANON_PAGES is set in rmap.c 
depending upon the whether the page is anon. NR_FILE_PAGES is set in 
filemap.c in the page cache functions.
    -- Ethan

--
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>