meminfo returns inaccurate NR_FILE_PAGES

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

Right. Sigh. Does this fix it?

Fix NR_FILE_PAGES and NR_ANON_PAGES accounting.

Index: linux-2.6.21-rc6/mm/migrate.c
===================================================================
@@ -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 [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>

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

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

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.

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
===================================================================
@@ -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 [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>

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

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

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

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.

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

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
===================================================================
@@ -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 [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>

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

I will make sure it gets applied.

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