[PATCH 0/3] mm: rename confusing function names

[PATCH 0/3] mm: rename confusing function names

[Zhang Yanfei]

Function nr_free_zone_pages, nr_free_buffer_pages and nr_free_pagecache_pages
are horribly badly named, they count present_pages - pages_high within zones
instead of free pages, so why not rename them to reasonable names, not cofusing
people.

patch2 and patch3 are based on patch1. So please apply patch1 first.

Zhang Yanfei (3):
  mm: rename nr_free_zone_pages to nr_free_zone_high_pages
  mm: rename nr_free_buffer_pages to nr_free_buffer_high_pages
  mm: rename nr_free_pagecache_pages to nr_free_pagecache_high_pages

 arch/ia64/mm/contig.c          |    3 ++-
 arch/ia64/mm/discontig.c       |    3 ++-
 drivers/mmc/card/mmc_test.c    |    4 ++--
 fs/buffer.c                    |    2 +-
 fs/nfsd/nfs4state.c            |    2 +-
 fs/nfsd/nfssvc.c               |    2 +-
 include/linux/swap.h           |    4 ++--
 mm/huge_memory.c               |    2 +-
 mm/memory_hotplug.c            |    4 ++--
 mm/page-writeback.c            |    2 +-
 mm/page_alloc.c                |   22 ++++++++++++----------
 net/9p/trans_virtio.c          |    2 +-
 net/ipv4/tcp.c                 |    4 ++--
 net/ipv4/udp.c                 |    2 +-
 net/netfilter/ipvs/ip_vs_ctl.c |    2 +-
 net/sctp/protocol.c            |    2 +-
 16 files changed, 33 insertions(+), 29 deletions(-)

[PATCH 1/3] mm: rename nr_free_zone_pages to nr_free_zone_high_pages

[Zhang Yanfei]

From: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>

This function actually counts present_pages - pages_high, so rename
it to a reasonable name.

Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 mm/page_alloc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index df2022f..4aea19e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2785,7 +2785,7 @@ void free_pages_exact(void *virt, size_t size)
 }
 EXPORT_SYMBOL(free_pages_exact);
 
-static unsigned int nr_free_zone_pages(int offset)
+static unsigned int nr_free_zone_high_pages(int offset)
 {
 	struct zoneref *z;
 	struct zone *zone;
@@ -2810,7 +2810,7 @@ static unsigned int nr_free_zone_pages(int offset)
  */
 unsigned int nr_free_buffer_pages(void)
 {
-	return nr_free_zone_pages(gfp_zone(GFP_USER));
+	return nr_free_zone_high_pages(gfp_zone(GFP_USER));
 }
 EXPORT_SYMBOL_GPL(nr_free_buffer_pages);
 
@@ -2819,7 +2819,7 @@ EXPORT_SYMBOL_GPL(nr_free_buffer_pages);
  */
 unsigned int nr_free_pagecache_pages(void)
 {
-	return nr_free_zone_pages(gfp_zone(GFP_HIGHUSER_MOVABLE));
+	return nr_free_zone_high_pages(gfp_zone(GFP_HIGHUSER_MOVABLE));
 }
 
 static inline void show_node(struct zone *zone)
-- 
1.7.1

[PATCH 2/3] mm: rename nr_free_buffer_pages to nr_free_buffer_high_pages

[Zhang Yanfei]

From: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>

This function actually counts RAM pages that are above high watermark within
ZONE_DMA and ZONE_NORMAL, so rename it to a reasonable name.

Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 arch/ia64/mm/contig.c          |    3 ++-
 arch/ia64/mm/discontig.c       |    3 ++-
 drivers/mmc/card/mmc_test.c    |    4 ++--
 fs/buffer.c                    |    2 +-
 fs/nfsd/nfs4state.c            |    2 +-
 fs/nfsd/nfssvc.c               |    2 +-
 include/linux/swap.h           |    2 +-
 mm/huge_memory.c               |    2 +-
 mm/page-writeback.c            |    2 +-
 mm/page_alloc.c                |    9 +++++----
 net/9p/trans_virtio.c          |    2 +-
 net/ipv4/tcp.c                 |    4 ++--
 net/ipv4/udp.c                 |    2 +-
 net/netfilter/ipvs/ip_vs_ctl.c |    2 +-
 net/sctp/protocol.c            |    2 +-
 15 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/arch/ia64/mm/contig.c b/arch/ia64/mm/contig.c
index 1516d1d..a2f45ce 100644
--- a/arch/ia64/mm/contig.c
+++ b/arch/ia64/mm/contig.c
@@ -93,7 +93,8 @@ void show_mem(unsigned int filter)
 	printk(KERN_INFO "%d pages swap cached\n", total_cached);
 	printk(KERN_INFO "Total of %ld pages in page table cache\n",
 	       quicklist_total_size());
-	printk(KERN_INFO "%d free buffer pages\n", nr_free_buffer_pages());
+	printk(KERN_INFO "%d free buffer pages above high watermark\n",
+	       nr_free_buffer_high_pages());
 }
 
 
diff --git a/arch/ia64/mm/discontig.c b/arch/ia64/mm/discontig.c
index c641333..cc55453 100644
--- a/arch/ia64/mm/discontig.c
+++ b/arch/ia64/mm/discontig.c
@@ -666,7 +666,8 @@ void show_mem(unsigned int filter)
 	printk(KERN_INFO "%d pages swap cached\n", total_cached);
 	printk(KERN_INFO "Total of %ld pages in page table cache\n",
 	       quicklist_total_size());
-	printk(KERN_INFO "%d free buffer pages\n", nr_free_buffer_pages());
+	printk(KERN_INFO "%d free buffer pages above high watermark\n",
+	       nr_free_buffer_pages());
 }
 
 /**
diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
index 759714e..8271d4d 100644
--- a/drivers/mmc/card/mmc_test.c
+++ b/drivers/mmc/card/mmc_test.c
@@ -16,7 +16,7 @@
 #include <linux/slab.h>
 
 #include <linux/scatterlist.h>
-#include <linux/swap.h>		/* For nr_free_buffer_pages() */
+#include <linux/swap.h>		/* For nr_free_buffer_high_pages() */
 #include <linux/list.h>
 
 #include <linux/debugfs.h>
@@ -323,7 +323,7 @@ static struct mmc_test_mem *mmc_test_alloc_mem(unsigned long min_sz,
 	unsigned long min_page_cnt = DIV_ROUND_UP(min_sz, PAGE_SIZE);
 	unsigned long max_seg_page_cnt = DIV_ROUND_UP(max_seg_sz, PAGE_SIZE);
 	unsigned long page_cnt = 0;
-	unsigned long limit = nr_free_buffer_pages() >> 4;
+	unsigned long limit = nr_free_buffer_high_pages() >> 4;
 	struct mmc_test_mem *mem;
 
 	if (max_page_cnt > limit)
diff --git a/fs/buffer.c b/fs/buffer.c
index 7a75c3e..c1bcb6d 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -3354,7 +3354,7 @@ void __init buffer_init(void)
 	/*
 	 * Limit the bh occupancy to 10% of ZONE_NORMAL
 	 */
-	nrpages = (nr_free_buffer_pages() * 10) / 100;
+	nrpages = (nr_free_buffer_high_pages() * 10) / 100;
 	max_buffer_heads = nrpages * (PAGE_SIZE / sizeof(struct buffer_head));
 	hotcpu_notifier(buffer_cpu_notify, 0);
 }
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ac8ed96..cddd447 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4813,7 +4813,7 @@ set_max_delegations(void)
 	 * is for a different inode), a delegation could take about 1.5K,
 	 * giving a worst case usage of about 6% of memory.
 	 */
-	max_delegations = nr_free_buffer_pages() >> (20 - 2 - PAGE_SHIFT);
+	max_delegations = nr_free_buffer_high_pages() >> (20 - 2 - PAGE_SHIFT);
 }
 
 static int nfs4_state_create_net(struct net *net)
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index cee62ab..2820409 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -338,7 +338,7 @@ void nfsd_reset_versions(void)
 static void set_max_drc(void)
 {
 	#define NFSD_DRC_SIZE_SHIFT	10
-	nfsd_drc_max_mem = (nr_free_buffer_pages()
+	nfsd_drc_max_mem = (nr_free_buffer_high_pages()
 					>> NFSD_DRC_SIZE_SHIFT) * PAGE_SIZE;
 	nfsd_drc_mem_used = 0;
 	spin_lock_init(&nfsd_drc_lock);
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 68df9c1..0df8905 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -216,7 +216,7 @@ struct swap_list_t {
 extern unsigned long totalram_pages;
 extern unsigned long totalreserve_pages;
 extern unsigned long dirty_balance_reserve;
-extern unsigned int nr_free_buffer_pages(void);
+extern unsigned int nr_free_buffer_high_pages(void);
 extern unsigned int nr_free_pagecache_pages(void);
 
 /* Definition of global_page_state not available yet */
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index b5783d8..c483e20 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -127,7 +127,7 @@ static int set_recommended_min_free_kbytes(void)
 
 	/* don't ever allow to reserve more than 5% of the lowmem */
 	recommended_min = min(recommended_min,
-			      (unsigned long) nr_free_buffer_pages() / 20);
+			      (unsigned long) nr_free_buffer_high_pages() / 20);
 	recommended_min <<= (PAGE_SHIFT-10);
 
 	if (recommended_min > min_free_kbytes)
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 0713bfb..246d0bd 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -1640,7 +1640,7 @@ static struct notifier_block __cpuinitdata ratelimit_nb = {
  *
  * We used to scale dirty pages according to how total memory
  * related to pages that could be allocated for buffers (by
- * comparing nr_free_buffer_pages() to vm_total_pages.
+ * comparing nr_free_buffer_high_pages() to vm_total_pages.
  *
  * However, that was when we used "dirty_ratio" to scale with
  * all memory, and we don't do that any more. "dirty_ratio"
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4aea19e..a021d91 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2806,13 +2806,14 @@ static unsigned int nr_free_zone_high_pages(int offset)
 }
 
 /*
- * Amount of free RAM allocatable within ZONE_DMA and ZONE_NORMAL
+ * Amount of free RAM allocatable that is above high watermark
+ * within ZONE_DMA and ZONE_NORMAL
  */
-unsigned int nr_free_buffer_pages(void)
+unsigned int nr_free_buffer_high_pages(void)
 {
 	return nr_free_zone_high_pages(gfp_zone(GFP_USER));
 }
-EXPORT_SYMBOL_GPL(nr_free_buffer_pages);
+EXPORT_SYMBOL_GPL(nr_free_buffer_high_pages);
 
 /*
  * Amount of free RAM allocatable within all zones
@@ -5351,7 +5352,7 @@ int __meminit init_per_zone_wmark_min(void)
 {
 	unsigned long lowmem_kbytes;
 
-	lowmem_kbytes = nr_free_buffer_pages() * (PAGE_SIZE >> 10);
+	lowmem_kbytes = nr_free_buffer_high_pages() * (PAGE_SIZE >> 10);
 
 	min_free_kbytes = int_sqrt(lowmem_kbytes * 16);
 	if (min_free_kbytes < 128)
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index fd05c81..da5b0bc 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -542,7 +542,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 	init_waitqueue_head(chan->vc_wq);
 	chan->ring_bufs_avail = 1;
 	/* Ceiling limit to avoid denial of service attacks */
-	chan->p9_max_pages = nr_free_buffer_pages()/4;
+	chan->p9_max_pages = nr_free_buffer_high_pages() / 4;
 
 	mutex_lock(&virtio_9p_lock);
 	list_add_tail(&chan->chan_list, &virtio_chan_list);
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 2aa69c8..44a6ace 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3565,7 +3565,7 @@ __setup("thash_entries=", set_thash_entries);
 
 void tcp_init_mem(struct net *net)
 {
-	unsigned long limit = nr_free_buffer_pages() / 8;
+	unsigned long limit = nr_free_buffer_high_pages() / 8;
 	limit = max(limit, 128UL);
 	net->ipv4.sysctl_tcp_mem[0] = limit / 4 * 3;
 	net->ipv4.sysctl_tcp_mem[1] = limit;
@@ -3635,7 +3635,7 @@ void __init tcp_init(void)
 
 	tcp_init_mem(&init_net);
 	/* Set per-socket limits to no more than 1/128 the pressure threshold */
-	limit = nr_free_buffer_pages() << (PAGE_SHIFT - 7);
+	limit = nr_free_buffer_high_pages() << (PAGE_SHIFT - 7);
 	max_wshare = min(4UL*1024*1024, limit);
 	max_rshare = min(6UL*1024*1024, limit);
 
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 1f4d405..cd61b9f 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2235,7 +2235,7 @@ void __init udp_init(void)
 	unsigned long limit;
 
 	udp_table_init(&udp_table, "UDP");
-	limit = nr_free_buffer_pages() / 8;
+	limit = nr_free_buffer_high_pages() / 8;
 	limit = max(limit, 128UL);
 	sysctl_udp_mem[0] = limit / 4 * 3;
 	sysctl_udp_mem[1] = limit;
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index ec664cb..a53cfb3 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -3723,7 +3723,7 @@ static int __net_init ip_vs_control_net_init_sysctl(struct net *net)
 	tbl[idx++].data = &ipvs->sysctl_sync_ver;
 	ipvs->sysctl_sync_ports = 1;
 	tbl[idx++].data = &ipvs->sysctl_sync_ports;
-	ipvs->sysctl_sync_qlen_max = nr_free_buffer_pages() / 32;
+	ipvs->sysctl_sync_qlen_max = nr_free_buffer_high_pages() / 32;
 	tbl[idx++].data = &ipvs->sysctl_sync_qlen_max;
 	ipvs->sysctl_sync_sock_size = 0;
 	tbl[idx++].data = &ipvs->sysctl_sync_sock_size;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index f898b1c..8c135af 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1354,7 +1354,7 @@ SCTP_STATIC __init int sctp_init(void)
 	/* Initialize handle used for association ids. */
 	idr_init(&sctp_assocs_id);
 
-	limit = nr_free_buffer_pages() / 8;
+	limit = nr_free_buffer_high_pages() / 8;
 	limit = max(limit, 128UL);
 	sysctl_sctp_mem[0] = limit / 4 * 3;
 	sysctl_sctp_mem[1] = limit;
-- 
1.7.1

[PATCH 3/3] mm: rename nr_free_pagecache_pages to nr_free_pagecache_high_pages

[Zhang Yanfei]

From: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>

This function actually counts RAM pages that are above high watermark within
all zones, so rename it to a reasonable name.

Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
 include/linux/swap.h |    2 +-
 mm/memory_hotplug.c  |    4 ++--
 mm/page_alloc.c      |    7 ++++---
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 0df8905..9a8ab19 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -217,7 +217,7 @@ extern unsigned long totalram_pages;
 extern unsigned long totalreserve_pages;
 extern unsigned long dirty_balance_reserve;
 extern unsigned int nr_free_buffer_high_pages(void);
-extern unsigned int nr_free_pagecache_pages(void);
+extern unsigned int nr_free_pagecache_high_pages(void);
 
 /* Definition of global_page_state not available yet */
 #define nr_free_pages() global_page_state(NR_FREE_PAGES)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index d04ed87..6e482c7 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -777,7 +777,7 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 	if (onlined_pages)
 		kswapd_run(zone_to_nid(zone));
 
-	vm_total_pages = nr_free_pagecache_pages();
+	vm_total_pages = nr_free_pagecache_high_pages();
 
 	writeback_set_ratelimit();
 
@@ -1356,7 +1356,7 @@ repeat:
 	if (arg.status_change_nid >= 0)
 		kswapd_stop(node);
 
-	vm_total_pages = nr_free_pagecache_pages();
+	vm_total_pages = nr_free_pagecache_high_pages();
 	writeback_set_ratelimit();
 
 	memory_notify(MEM_OFFLINE, &arg);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a021d91..6e0d91a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2816,9 +2816,10 @@ unsigned int nr_free_buffer_high_pages(void)
 EXPORT_SYMBOL_GPL(nr_free_buffer_high_pages);
 
 /*
- * Amount of free RAM allocatable within all zones
+ * Amount of free RAM allocatable that is above high watermark
+ * within all zones
  */
-unsigned int nr_free_pagecache_pages(void)
+unsigned int nr_free_pagecache_high_pages(void)
 {
 	return nr_free_zone_high_pages(gfp_zone(GFP_HIGHUSER_MOVABLE));
 }
@@ -3649,7 +3650,7 @@ void __ref build_all_zonelists(pg_data_t *pgdat, struct zone *zone)
 		stop_machine(__build_all_zonelists, pgdat, NULL);
 		/* cpuset refresh routine should be here */
 	}
-	vm_total_pages = nr_free_pagecache_pages();
+	vm_total_pages = nr_free_pagecache_high_pages();
 	/*
 	 * Disable grouping by mobility if the number of pages in the
 	 * system is too low to allow the mechanism to work. It would be
-- 
1.7.1

Re: [PATCH 0/3] mm: rename confusing function names

[Johannes Weiner]

On Wed, Feb 06, 2013 at 01:09:55AM +0800, Zhang Yanfei wrote:
> Function nr_free_zone_pages, nr_free_buffer_pages and nr_free_pagecache_pages
> are horribly badly named, they count present_pages - pages_high within zones
> instead of free pages, so why not rename them to reasonable names, not cofusing
> people.
> 
> patch2 and patch3 are based on patch1. So please apply patch1 first.
> 
> Zhang Yanfei (3):
>   mm: rename nr_free_zone_pages to nr_free_zone_high_pages
>   mm: rename nr_free_buffer_pages to nr_free_buffer_high_pages
>   mm: rename nr_free_pagecache_pages to nr_free_pagecache_high_pages

I don't feel that this is an improvement.

As you said, the "free" is already misleading, because those pages
might all be allocated.  "High" makes me think not just of highmem,
but drug abuse in general.

nr_available_*_pages?  I don't know, but if we go through with all
that churn, it had better improve something.

Re: [PATCH 0/3] mm: rename confusing function names

[Andrew Morton]

On Tue, 5 Feb 2013 14:26:40 -0500
Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Wed, Feb 06, 2013 at 01:09:55AM +0800, Zhang Yanfei wrote:
> > Function nr_free_zone_pages, nr_free_buffer_pages and nr_free_pagecache_pages
> > are horribly badly named, they count present_pages - pages_high within zones
> > instead of free pages, so why not rename them to reasonable names, not cofusing
> > people.
> > 
> > patch2 and patch3 are based on patch1. So please apply patch1 first.
> > 
> > Zhang Yanfei (3):
> >   mm: rename nr_free_zone_pages to nr_free_zone_high_pages
> >   mm: rename nr_free_buffer_pages to nr_free_buffer_high_pages
> >   mm: rename nr_free_pagecache_pages to nr_free_pagecache_high_pages
> 
> I don't feel that this is an improvement.
> 
> As you said, the "free" is already misleading, because those pages
> might all be allocated.  "High" makes me think not just of highmem,
> but drug abuse in general.
> 
> nr_available_*_pages?  I don't know, but if we go through with all
> that churn, it had better improve something.

Yes, those names are ghastly.

Here's an idea: accurately document the functions with code comments. 
Once this is done, that documentation may well suggest a good name ;)


While we're there, please note that nr_free_buffer_pages() has a *lot*
of callers.  Generally it's code which is trying to work out what is an
appropriate size for preallocated caching space, lookup tables, etc. 

That's a rather hopeless objective, given memory hotplug, mlock, etc. 
But please do take a look at *why* these callers are calling
nr_free_buffer_pages() and let's ensure that both the implementation
and name are appropriate to their requirements.

Re: [PATCH 0/3] mm: rename confusing function names

[Zhang Yanfei]

于 2013年02月06日 06:13, Andrew Morton 写道:
> On Tue, 5 Feb 2013 14:26:40 -0500
> Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
>> On Wed, Feb 06, 2013 at 01:09:55AM +0800, Zhang Yanfei wrote:
>>> Function nr_free_zone_pages, nr_free_buffer_pages and nr_free_pagecache_pages
>>> are horribly badly named, they count present_pages - pages_high within zones
>>> instead of free pages, so why not rename them to reasonable names, not cofusing
>>> people.
>>>
>>> patch2 and patch3 are based on patch1. So please apply patch1 first.
>>>
>>> Zhang Yanfei (3):
>>>   mm: rename nr_free_zone_pages to nr_free_zone_high_pages
>>>   mm: rename nr_free_buffer_pages to nr_free_buffer_high_pages
>>>   mm: rename nr_free_pagecache_pages to nr_free_pagecache_high_pages
>>
>> I don't feel that this is an improvement.
>>
>> As you said, the "free" is already misleading, because those pages
>> might all be allocated.  "High" makes me think not just of highmem,
>> but drug abuse in general.
>>
>> nr_available_*_pages?  I don't know, but if we go through with all
>> that churn, it had better improve something.
> 
> Yes, those names are ghastly.
> 
> Here's an idea: accurately document the functions with code comments. 
> Once this is done, that documentation may well suggest a good name ;)
> 

As Johannes said, free is already misleading, so I think we should
rename "free" at first. to "available"? I think it is ok.

"high" here means those pages are above high watermark of a zone,
not highmem or something else.

So could I rename the functions to the names like
nr_available_buffer_high_pages
And accurately document them with code comments just as you suggested.

is this ok?

> 
> While we're there, please note that nr_free_buffer_pages() has a *lot*
> of callers.  Generally it's code which is trying to work out what is an
> appropriate size for preallocated caching space, lookup tables, etc. 
> 
> That's a rather hopeless objective, given memory hotplug, mlock, etc. 
> But please do take a look at *why* these callers are calling
> nr_free_buffer_pages() and let's ensure that both the implementation
> and name are appropriate to their requirements.

Yeah, it does have a lot callers and I think some of the callers are
misusing the function from the comments. They always want to call the
function to get lowmem pages.

Thanks
Zhang Yanfei

Re: [PATCH 0/3] mm: rename confusing function names

[Andrew Morton]

On Wed, 06 Feb 2013 09:06:05 +0800
Zhang Yanfei <zhangyanfei@cn.fujitsu.com> wrote:

> So could I rename the functions to the names like
> nr_available_buffer_high_pages
> And accurately document them with code comments just as you suggested.

gee.  "available" implies "available for you to allocate".  It has the
same problem as "free".

And "buffer" shouldn't be there - that's a reflection of the fact
that buffer_head payloads are not allocated from highmem.  An archaic
irrelevant thing.

Seriously, first let's write down the descriptions of what these
functions *do*.  Then choose nice names which abbreviate that.



hm,

static unsigned int nr_free_zone_pages(int offset)
{
	...
	unsigned int sum = 0;
	...
	return sum;
}

How long will it be until these things start exploding from
sums-of-zones which exceed 16TB?  

Re: [PATCH 0/3] mm: rename confusing function names

[Zhang Yanfei]

于 2013年02月06日 09:20, Andrew Morton 写道:
> On Wed, 06 Feb 2013 09:06:05 +0800
> Zhang Yanfei <zhangyanfei@cn.fujitsu.com> wrote:
> 
>> So could I rename the functions to the names like
>> nr_available_buffer_high_pages
>> And accurately document them with code comments just as you suggested.
> 
> gee.  "available" implies "available for you to allocate".  It has the
> same problem as "free".
> 
> And "buffer" shouldn't be there - that's a reflection of the fact
> that buffer_head payloads are not allocated from highmem.  An archaic
> irrelevant thing.
> 
> Seriously, first let's write down the descriptions of what these
> functions *do*.  Then choose nice names which abbreviate that.
> 

OK, I will try to do this.

> 
> 
> hm,
> 
> static unsigned int nr_free_zone_pages(int offset)
> {
> 	...
> 	unsigned int sum = 0;
> 	...
> 	return sum;
> }
> 
> How long will it be until these things start exploding from
> sums-of-zones which exceed 16TB?  
> 

You mean overflow? Hmm.. it might happens. Change the sum to
unsigned long is ok?

Thanks
Zhang Yanfei

Re: [PATCH 0/3] mm: rename confusing function names

[Andrew Morton]

On Wed, 06 Feb 2013 09:34:16 +0800 Zhang Yanfei <zhangyanfei@cn.fujitsu.com> wrote:

> > 
> > 
> > hm,
> > 
> > static unsigned int nr_free_zone_pages(int offset)
> > {
> > 	...
> > 	unsigned int sum = 0;
> > 	...
> > 	return sum;
> > }
> > 
> > How long will it be until these things start exploding from
> > sums-of-zones which exceed 16TB?  
> > 
> 
> You mean overflow? Hmm.. it might happens. Change the sum to
> unsigned long is ok?

The sum, the return value.  And in the case of nr_free_buffer_pages(),
the signedness of the return value (sheesh).  Then review and if
necessary fix up all the callsites.  That's all a separate exercise.

Re: [PATCH 0/3] mm: rename confusing function names

[Robin Holt]

> > static unsigned int nr_free_zone_pages(int offset)
> > {
> > 	...
> > 	unsigned int sum = 0;
> > 	...
> > 	return sum;
> > }
> > 
> > How long will it be until these things start exploding from
> > sums-of-zones which exceed 16TB?  
> > 
> 
> You mean overflow? Hmm.. it might happens. Change the sum to
> unsigned long is ok?

We are in the process right now of building a 32TB machine.  Let me make
a note about this right away.  Thankfully, the memory will be spread
over 256 zones so it should not impact us right away.

Thanks,
Robin