mm: rename confusing function names

PATCH In review Zhang Yanfei 5 participants 11 messages Feb 5, 2013 – Feb 8, 2013

Subsystems (16)

Networking Memory management VFS netfilter +12 more subsystems

lore

Share & tools

lore

Zhang Yanfei

author

2013-02-05 17:10 UTC #0 lore raw

cover mm: rename confusing function names

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

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

Zhang Yanfei

author

2013-02-05 17:11 UTC #1 lore raw

PATCH 1/3 mm: rename nr_free_zone_pages to nr_free_zone_high_pages

From: Zhang Yanfei <[email protected]>

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

Signed-off-by Zhang Yanfei <[email protected]>

---
 mm/page_alloc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

mm/page_alloc.c +3 -3

index df2022f..4aea19e 100644
@@ -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)

Signature · 7 lines

1.7.1

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

Zhang Yanfei

author

2013-02-05 17:13 UTC #2 lore raw

PATCH 2/3 mm: rename nr_free_buffer_pages to nr_free_buffer_high_pages

In reply to Zhang Yanfei

From: Zhang Yanfei <[email protected]>

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 <[email protected]>

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

arch/ia64/mm/contig.c +2 -1

index 1516d1d..a2f45ce 100644
@@ -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());
 }

arch/ia64/mm/discontig.c +2 -1

index c641333..cc55453 100644
@@ -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());
 }
 
 /**

drivers/mmc/card/mmc_test.c +2 -2

index 759714e..8271d4d 100644
@@ -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)

fs/buffer.c +1 -1

index 7a75c3e..c1bcb6d 100644
@@ -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);
 }

fs/nfsd/nfs4state.c +1 -1

index ac8ed96..cddd447 100644
@@ -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)

fs/nfsd/nfssvc.c +1 -1

index cee62ab..2820409 100644
@@ -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);

include/linux/swap.h +1 -1

index 68df9c1..0df8905 100644
@@ -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 */

mm/huge_memory.c +1 -1

index b5783d8..c483e20 100644
@@ -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)

mm/page-writeback.c +1 -1

index 0713bfb..246d0bd 100644
@@ -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"

mm/page_alloc.c +5 -4

index 4aea19e..a021d91 100644
@@ -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)

net/9p/trans_virtio.c +1 -1

index fd05c81..da5b0bc 100644
@@ -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);

net/ipv4/tcp.c +2 -2

index 2aa69c8..44a6ace 100644
@@ -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);

net/ipv4/udp.c +1 -1

index 1f4d405..cd61b9f 100644
@@ -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;

net/netfilter/ipvs/ip_vs_ctl.c +1 -1

index ec664cb..a53cfb3 100644
@@ -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;

net/sctp/protocol.c +1 -1

index f898b1c..8c135af 100644
@@ -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;

Signature · 7 lines

1.7.1

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

Zhang Yanfei

author

2013-02-05 17:14 UTC #3 lore raw

PATCH 3/3 mm: rename nr_free_pagecache_pages to nr_free_pagecache_high_pages

In reply to Zhang Yanfei

From: Zhang Yanfei <[email protected]>

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 <[email protected]>

---
 include/linux/swap.h |    2 +-
 mm/memory_hotplug.c  |    4 ++--
 mm/page_alloc.c      |    7 ++++---
 3 files changed, 7 insertions(+), 6 deletions(-)

include/linux/swap.h +1 -1

index 0df8905..9a8ab19 100644
@@ -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)

mm/memory_hotplug.c +2 -2

index d04ed87..6e482c7 100644
@@ -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);

mm/page_alloc.c +4 -3

index a021d91..6e0d91a 100644
@@ -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

Signature · 7 lines

1.7.1

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

Johannes Weiner

reviewer

2013-02-05 19:26 UTC #4 lore raw

In reply to Zhang Yanfei

Quoted 11 lines · 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

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.

Andrew Morton

maintainer

2013-02-05 22:13 UTC #5 lore raw

Quoted 13 lines · On Tue, 5 Feb 2013 14:26:40 -0500 Johannes Weiner <[email protected]> wrote:

Quoted 11 lines · 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

Quoted 8 lines

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.

Zhang Yanfei

2013-02-06 01:10 UTC #6 lore raw

Quoted 22 lines · Andrew Morton

Quoted 19 lines · On Tue, 5 Feb 2013 14:26:40 -0500 Johannes Weiner <[email protected]> wrote:
Quoted 11 lines · 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
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.

Quoted 8 lines

Quoted 2 lines
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?

Quoted 9 lines

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

Andrew Morton

maintainer

2013-02-06 01:20 UTC #7 lore raw

Quoted 3 lines · On Wed, 06 Feb 2013 09:06:05 +0800 Zhang Yanfei <[email protected]>…

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?

Zhang Yanfei

2013-02-06 01:35 UTC #8 lore raw

Quoted 10 lines · Andrew Morton

Quoted 3 lines · On Wed, 06 Feb 2013 09:06:05 +0800 Zhang Yanfei <[email protected]>…
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".

Quoted 7 lines

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.

Quoted 12 lines

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

Quoted 3 lines

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

Andrew Morton

maintainer

2013-02-06 01:54 UTC #9 lore raw

Quoted 12 lines · On Wed, 06 Feb 2013 09:34:16 +0800 Zhang Yanfei <[email protected]>…

Quoted 12 lines
hm,
static unsigned int nr_free_zone_pages(int offset)
{
	...
	unsigned int sum = 0;
	...
	return sum;
}

Quoted 6 lines

Quoted 3 lines
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.

Robin Holt

2013-02-08 13:34 UTC #10 lore raw

In reply to Zhang Yanfei

Quoted 8 lines

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

Quoted 6 lines

Quoted 3 lines
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