Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1429658 > unrolled thread

[RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into node_page_state

Back to article view | Back to linux.kernel

Contents

  [RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into node_page_state Arnd Bergmann <arnd@arndb.de> - 2016-06-23 12:10 +0200
    Re: [RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into  node_page_state Mel Gorman <mgorman@techsingularity.net> - 2016-06-23 12:50 +0200
      Re: [RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into node_page_state Arnd Bergmann <arnd@arndb.de> - 2016-06-23 15:20 +0200
        Re: [RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into  node_page_state Mel Gorman <mgorman@techsingularity.net> - 2016-06-23 16:00 +0200
          Re: [RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into node_page_state Arnd Bergmann <arnd@arndb.de> - 2016-06-23 18:00 +0200
            Re: [RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into  node_page_state Mel Gorman <mgorman@techsingularity.net> - 2016-06-23 18:40 +0200

#1429658 — [RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into node_page_state

I see some new warnings from a recent mm change:

mm/filemap.c: In function '__delete_from_page_cache':
include/linux/vmstat.h:116:2: error: array subscript is above array bounds [-Werror=array-bounds]
  atomic_long_add(x, &zone->vm_stat[item]);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:116:35: error: array subscript is above array bounds [-Werror=array-bounds]
  atomic_long_add(x, &zone->vm_stat[item]);
                      ~~~~~~~~~~~~~^~~~~~
include/linux/vmstat.h:116:35: error: array subscript is above array bounds [-Werror=array-bounds]
include/linux/vmstat.h:117:2: error: array subscript is above array bounds [-Werror=array-bounds]

Looking deeper into it, I find that we pass the wrong enum
into some functions after the type for the symbol has changed.

This changes the code to use the other function for those that
are using the incorrect type. I've done this blindly just going
by warnings I got from a debug patch I did for this, so it's likely
that some cases are more subtle and need another change, so please
treat this as a bug-report rather than a patch for applying.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: e426f7b4ade5 ("mm: move most file-based accounting to the node")
---
 mm/filemap.c    |  4 ++--
 mm/page_alloc.c | 15 ++++++++-------
 mm/rmap.c       |  4 ++--
 mm/shmem.c      |  4 ++--
 mm/vmscan.c     |  2 +-
 5 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 6cb19e012887..77e902bf04f4 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -218,9 +218,9 @@ void __delete_from_page_cache(struct page *page, void *shadow)
 
 	/* hugetlb pages do not participate in page cache accounting. */
 	if (!PageHuge(page))
-		__mod_zone_page_state(page_zone(page), NR_FILE_PAGES, -nr);
+		__mod_node_page_state(page_zone(page)->zone_pgdat, NR_FILE_PAGES, -nr);
 	if (PageSwapBacked(page)) {
-		__mod_zone_page_state(page_zone(page), NR_SHMEM, -nr);
+		__mod_node_page_state(page_zone(page)->zone_pgdat, NR_SHMEM, -nr);
 		if (PageTransHuge(page))
 			__dec_zone_page_state(page, NR_SHMEM_THPS);
 	} else {
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 23b5044f5ced..d5287011ed27 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3484,9 +3484,10 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
 				unsigned long writeback;
 				unsigned long dirty;
 
-				writeback = zone_page_state_snapshot(zone,
+				writeback = node_page_state_snapshot(zone->zone_pgdat,
 								     NR_WRITEBACK);
-				dirty = zone_page_state_snapshot(zone, NR_FILE_DIRTY);
+				dirty = node_page_state_snapshot(zone->zone_pgdat,
+								 NR_FILE_DIRTY);
 
 				if (2*(writeback + dirty) > reclaimable) {
 					congestion_wait(BLK_RW_ASYNC, HZ/10);
@@ -4396,9 +4397,9 @@ void show_free_areas(unsigned int filter)
 			K(zone->present_pages),
 			K(zone->managed_pages),
 			K(zone_page_state(zone, NR_MLOCK)),
-			K(zone_page_state(zone, NR_FILE_DIRTY)),
-			K(zone_page_state(zone, NR_WRITEBACK)),
-			K(zone_page_state(zone, NR_SHMEM)),
+			K(node_page_state(zone->zone_pgdat, NR_FILE_DIRTY)),
+			K(node_page_state(zone, NR_WRITEBACK)),
+			K(node_page_state(zone, NR_SHMEM)),
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 			K(zone_page_state(zone, NR_SHMEM_THPS) * HPAGE_PMD_NR),
 			K(zone_page_state(zone, NR_SHMEM_PMDMAPPED)
@@ -4410,12 +4411,12 @@ void show_free_areas(unsigned int filter)
 			zone_page_state(zone, NR_KERNEL_STACK) *
 				THREAD_SIZE / 1024,
 			K(zone_page_state(zone, NR_PAGETABLE)),
-			K(zone_page_state(zone, NR_UNSTABLE_NFS)),
+			K(node_page_state(zone, NR_UNSTABLE_NFS)),
 			K(zone_page_state(zone, NR_BOUNCE)),
 			K(free_pcp),
 			K(this_cpu_read(zone->pageset->pcp.count)),
 			K(zone_page_state(zone, NR_FREE_CMA_PAGES)),
-			K(zone_page_state(zone, NR_WRITEBACK_TEMP)),
+			K(node_page_state(zone, NR_WRITEBACK_TEMP)),
 			K(node_page_state(zone->zone_pgdat, NR_PAGES_SCANNED)));
 		printk("lowmem_reserve[]:");
 		for (i = 0; i < MAX_NR_ZONES; i++)
diff --git a/mm/rmap.c b/mm/rmap.c
index 4deff963ea8a..898b2b7806ca 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1296,7 +1296,7 @@ void page_add_file_rmap(struct page *page, bool compound)
 		if (!atomic_inc_and_test(&page->_mapcount))
 			goto out;
 	}
-	__mod_zone_page_state(page_zone(page), NR_FILE_MAPPED, nr);
+	__mod_node_page_state(page_zone(page)->zone_pgdat, NR_FILE_MAPPED, nr);
 	mem_cgroup_inc_page_stat(page, MEM_CGROUP_STAT_FILE_MAPPED);
 out:
 	unlock_page_memcg(page);
@@ -1336,7 +1336,7 @@ static void page_remove_file_rmap(struct page *page, bool compound)
 	 * these counters are not modified in interrupt context, and
 	 * pte lock(a spinlock) is held, which implies preemption disabled.
 	 */
-	__mod_zone_page_state(page_zone(page), NR_FILE_MAPPED, -nr);
+	__mod_node_page_state(page_zone(page)->zone_pgdat, NR_FILE_MAPPED, -nr);
 	mem_cgroup_dec_page_stat(page, MEM_CGROUP_STAT_FILE_MAPPED);
 
 	if (unlikely(PageMlocked(page)))
diff --git a/mm/shmem.c b/mm/shmem.c
index e5c50fb0d4a4..99dcb8e5642d 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -576,8 +576,8 @@ static int shmem_add_to_page_cache(struct page *page,
 		mapping->nrpages += nr;
 		if (PageTransHuge(page))
 			__inc_zone_page_state(page, NR_SHMEM_THPS);
-		__mod_zone_page_state(page_zone(page), NR_FILE_PAGES, nr);
-		__mod_zone_page_state(page_zone(page), NR_SHMEM, nr);
+		__mod_node_page_state(page_zone(page)->zone_pgdat, NR_FILE_PAGES, nr);
+		__mod_node_page_state(page_zone(page)->zone_pgdat, NR_SHMEM, nr);
 		spin_unlock_irq(&mapping->tree_lock);
 	} else {
 		page->mapping = NULL;
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 07e17dac1793..4702069cc80b 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2079,7 +2079,7 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
 		int z;
 		unsigned long total_high_wmark = 0;
 
-		pgdatfree = sum_zone_node_page_state(pgdat->node_id, NR_FREE_PAGES);
+		pgdatfree = global_page_state(NR_FREE_PAGES);
 		pgdatfile = node_page_state(pgdat, NR_ACTIVE_FILE) +
 			   node_page_state(pgdat, NR_INACTIVE_FILE);
 
-- 
2.9.0

[toc] | [next] | [standalone]

#1429673 — Re: [RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into node_page_state

On Thu, Jun 23, 2016 at 12:05:17PM +0200, Arnd Bergmann wrote:
> I see some new warnings from a recent mm change:
> 
> mm/filemap.c: In function '__delete_from_page_cache':
> include/linux/vmstat.h:116:2: error: array subscript is above array bounds [-Werror=array-bounds]
>   atomic_long_add(x, &zone->vm_stat[item]);
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/vmstat.h:116:35: error: array subscript is above array bounds [-Werror=array-bounds]
>   atomic_long_add(x, &zone->vm_stat[item]);
>                       ~~~~~~~~~~~~~^~~~~~
> include/linux/vmstat.h:116:35: error: array subscript is above array bounds [-Werror=array-bounds]
> include/linux/vmstat.h:117:2: error: array subscript is above array bounds [-Werror=array-bounds]
> 
> Looking deeper into it, I find that we pass the wrong enum
> into some functions after the type for the symbol has changed.
> 
> This changes the code to use the other function for those that
> are using the incorrect type. I've done this blindly just going
> by warnings I got from a debug patch I did for this, so it's likely
> that some cases are more subtle and need another change, so please
> treat this as a bug-report rather than a patch for applying.
> 

I have an alternative fix for this in a private tree. For now, I've asked
Andrew to withdraw the series entirely as there are non-trivial collisions
with OOM detection rework and huge page support for tmpfs.  It'll be easier
and safer to resolve this outside of mmotm as it'll require a full round
of testing which takes 3-4 days.

Thanks.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1429818

On Thursday, June 23, 2016 11:41:24 AM CEST Mel Gorman wrote:
> On Thu, Jun 23, 2016 at 12:05:17PM +0200, Arnd Bergmann wrote:
> > I see some new warnings from a recent mm change:
> > 
> > mm/filemap.c: In function '__delete_from_page_cache':
> > include/linux/vmstat.h:116:2: error: array subscript is above array bounds [-Werror=array-bounds]
> >   atomic_long_add(x, &zone->vm_stat[item]);
> >   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/vmstat.h:116:35: error: array subscript is above array bounds [-Werror=array-bounds]
> >   atomic_long_add(x, &zone->vm_stat[item]);
> >                       ~~~~~~~~~~~~~^~~~~~
> > include/linux/vmstat.h:116:35: error: array subscript is above array bounds [-Werror=array-bounds]
> > include/linux/vmstat.h:117:2: error: array subscript is above array bounds [-Werror=array-bounds]
> > 
> > Looking deeper into it, I find that we pass the wrong enum
> > into some functions after the type for the symbol has changed.
> > 
> > This changes the code to use the other function for those that
> > are using the incorrect type. I've done this blindly just going
> > by warnings I got from a debug patch I did for this, so it's likely
> > that some cases are more subtle and need another change, so please
> > treat this as a bug-report rather than a patch for applying.
> > 
> 
> I have an alternative fix for this in a private tree. For now, I've asked
> Andrew to withdraw the series entirely as there are non-trivial collisions
> with OOM detection rework and huge page support for tmpfs.  It'll be easier
> and safer to resolve this outside of mmotm as it'll require a full round
> of testing which takes 3-4 days.

Ok. I've done a new version of my debug patch now, will follow up here
so you can do some testing on top of that as well if you like. We probably
don't want to apply my patch for the type checking, but you might find it
useful for your own testing.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1429859 — Re: [RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into node_page_state

On Thu, Jun 23, 2016 at 03:17:43PM +0200, Arnd Bergmann wrote:
> > I have an alternative fix for this in a private tree. For now, I've asked
> > Andrew to withdraw the series entirely as there are non-trivial collisions
> > with OOM detection rework and huge page support for tmpfs.  It'll be easier
> > and safer to resolve this outside of mmotm as it'll require a full round
> > of testing which takes 3-4 days.
> 
> Ok. I've done a new version of my debug patch now, will follow up here
> so you can do some testing on top of that as well if you like. We probably
> don't want to apply my patch for the type checking, but you might find it
> useful for your own testing.
> 

It is useful. After fixing up a bunch of problems manually, it
identified two more errors. I probably won't merge it but I'll hang on
to it during development.

Thanks!

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1429944

On Thursday, June 23, 2016 2:51:11 PM CEST Mel Gorman wrote:
> On Thu, Jun 23, 2016 at 03:17:43PM +0200, Arnd Bergmann wrote:
> > > I have an alternative fix for this in a private tree. For now, I've asked
> > > Andrew to withdraw the series entirely as there are non-trivial collisions
> > > with OOM detection rework and huge page support for tmpfs.  It'll be easier
> > > and safer to resolve this outside of mmotm as it'll require a full round
> > > of testing which takes 3-4 days.
> > 
> > Ok. I've done a new version of my debug patch now, will follow up here
> > so you can do some testing on top of that as well if you like. We probably
> > don't want to apply my patch for the type checking, but you might find it
> > useful for your own testing.
> > 
> 
> It is useful. After fixing up a bunch of problems manually, it
> identified two more errors. I probably won't merge it but I'll hang on
> to it during development.

I'm glad it helps. On my randconfig build machine, I've also now run
into yet another finding that I originally didn't catch, not sure if you
found this one already:

In file included from ../include/linux/mm.h:999:0,
                 from ../include/linux/highmem.h:7,
                 from ../drivers/staging/lustre/lustre/osc/../../include/linux/libcfs/linux/libcfs.h:46,
                 from ../drivers/staging/lustre/lustre/osc/../../include/linux/libcfs/libcfs.h:36,
                 from ../drivers/staging/lustre/lustre/osc/osc_cl_internal.h:45,
                 from ../drivers/staging/lustre/lustre/osc/osc_cache.c:40:
../drivers/staging/lustre/lustre/osc/osc_cache.c: In function 'osc_dec_unstable_pages':
../include/linux/vmstat.h:247:42: error: comparison between 'enum node_stat_item' and 'enum zone_stat_item' [-Werror=enum-compare]
  dec_zone_page_state_check(page, ((item) == (enum zone_stat_item )0) ? (item) : (item))
                                          ^
../drivers/staging/lustre/lustre/osc/osc_cache.c:1867:3: note: in expansion of macro 'dec_zone_page_state'
   dec_zone_page_state(desc->bd_iov[i].kiov_page, NR_UNSTABLE_NFS);
   ^~~~~~~~~~~~~~~~~~~
../drivers/staging/lustre/lustre/osc/osc_cache.c: In function 'osc_inc_unstable_pages':
../include/linux/vmstat.h:245:42: error: comparison between 'enum node_stat_item' and 'enum zone_stat_item' [-Werror=enum-compare]
  inc_zone_page_state_check(page, ((item) == (enum zone_stat_item )0) ? (item) : (item))
                                          ^
../drivers/staging/lustre/lustre/osc/osc_cache.c:1901:3: note: in expansion of macro 'inc_zone_page_state'
   inc_zone_page_state(desc->bd_iov[i].kiov_page, NR_UNSTABLE_NFS);
   ^~~~~~~~~~~~~~~~~~~

	Arnd

[toc] | [prev] | [next] | [standalone]

#1429969 — Re: [RFC, DEBUGGING 1/2] mm: pass NR_FILE_PAGES/NR_SHMEM into node_page_state

On Thu, Jun 23, 2016 at 05:56:57PM +0200, Arnd Bergmann wrote:
> On Thursday, June 23, 2016 2:51:11 PM CEST Mel Gorman wrote:
> > On Thu, Jun 23, 2016 at 03:17:43PM +0200, Arnd Bergmann wrote:
> > > > I have an alternative fix for this in a private tree. For now, I've asked
> > > > Andrew to withdraw the series entirely as there are non-trivial collisions
> > > > with OOM detection rework and huge page support for tmpfs.  It'll be easier
> > > > and safer to resolve this outside of mmotm as it'll require a full round
> > > > of testing which takes 3-4 days.
> > > 
> > > Ok. I've done a new version of my debug patch now, will follow up here
> > > so you can do some testing on top of that as well if you like. We probably
> > > don't want to apply my patch for the type checking, but you might find it
> > > useful for your own testing.
> > > 
> > 
> > It is useful. After fixing up a bunch of problems manually, it
> > identified two more errors. I probably won't merge it but I'll hang on
> > to it during development.
> 
> I'm glad it helps. On my randconfig build machine, I've also now run
> into yet another finding that I originally didn't catch, not sure if you
> found this one already:
> 

It's corrected in my current working tree. Thanks for continuing to
check.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web