Re: [PATCH 4/7] ppc64 - Specify amount of kernel memory at boot time

[Dave Hansen <haveblue@us.ibm.com>]

On Wed, 2006-02-22 at 16:43 +0000, Mel Gorman wrote:
> Is this a bit clearer? It's built and boot tested on one ppc64 machine. I 
> am having trouble finding a ppc64 machine that *has* memory holes to be 
> 100% sure it's ok.

Yeah, it looks that way.  If you need a machine, see Mike Kravetz.  I
think he was working on a way to automate creating memory holes.

> diff -rup -X /usr/src/patchset-0.5/bin//dontdiff linux-2.6.16-rc3-mm1-103_x86coremem/arch/powerpc/mm/numa.c linux-2.6.16-rc3-mm1-104_ppc64coremem/arch/powerpc/mm/numa.c
> --- linux-2.6.16-rc3-mm1-103_x86coremem/arch/powerpc/mm/numa.c	2006-02-16 09:50:42.000000000 +0000
> +++ linux-2.6.16-rc3-mm1-104_ppc64coremem/arch/powerpc/mm/numa.c	2006-02-22 16:07:35.000000000 +0000
> @@ -17,10 +17,12 @@
>   #include <linux/nodemask.h>
>   #include <linux/cpu.h>
>   #include <linux/notifier.h>
> +#include <linux/sort.h>
>   #include <asm/sparsemem.h>
>   #include <asm/lmb.h>
>   #include <asm/system.h>
>   #include <asm/smp.h>
> +#include <asm/machdep.h>

Is the email spacing getting screwed up here?

> +/* Initialise the size of each zone in a node */
> +void __init zone_sizes_init(unsigned int nid,
> +		unsigned long kernelcore_pages,
> +		unsigned long *zones_size)

Minor nit territory: set_zone_sizes(), maybe?

> +{
> +	unsigned int i;
> +	unsigned long pages_present = 0;

pages_present_in_node?

> +	/* Get the number of present pages in the node */
> +	for (i = 0; init_node_data[i].end_pfn; i++) {
> +		if (init_node_data[i].nid != nid)
> +			continue;
> +
> +		pages_present += init_node_data[i].end_pfn -
> +			init_node_data[i].start_pfn;
> +	}
> +
> +	if (kernelcore_pages && kernelcore_pages < pages_present) {
> +		zones_size[ZONE_DMA] = kernelcore_pages;
> +		zones_size[ZONE_EASYRCLM] = pages_present - kernelcore_pages;
> +	} else {
> +		zones_size[ZONE_DMA] = pages_present;
> +		zones_size[ZONE_EASYRCLM] = 0;
> +	}
> +}

I think there are a couple of buglets here.  I think the
kernelcore_pages is going to be applied per-zone, right?

Also, how do we want to distribute kernelcore memory over each node?
The way it is coded up for now, it will all be sliced out of the first
node.  I'm not sure that's a good thing.

My inclination would be to completely separate out the ZONE_EASYRCLM
into separate code.  It makes it easier to set whatever policy you want
in one place.  Just a suggestion.

> +void __init get_zholes_size(unsigned int nid, unsigned long *zones_size,
> +		unsigned long *zholes_size) {

nid_zholes_size()?  I'm not too sure about this one.  Just promise me
you'll think about it a bit more. ;)

> +	unsigned int i = 0;
> +	unsigned int start_easyrclm_pfn;
> +	unsigned long last_end_pfn, first;
> +
> +	/* Find where the PFN of the end of DMA is */
> +	unsigned long pages_count = zones_size[ZONE_DMA];

<tangent> This (virtually) proves that zones_size[] needs to get a
different name.  Perhaps we need to make it more like the zone structure
itself and go to spanned and present pages? </tangent>

> +	for (i = 0; init_node_data[i].end_pfn; i++) {
> +		unsigned long segment_size;
> +		if (init_node_data[i].nid != nid)
> +			continue;
> +
> +		/*
> +		 * Check if the end of ZONE_DMA is in this segment of the
> +		 * init_node_data
> +		 */
> +		segment_size = init_node_data[i].end_pfn -
> +			init_node_data[i].start_pfn;

"segment" is probably a bad term to use here, especially on ppc.

One other thing, I want to _know_ that variables being compared are in
the same units.  When one is called "pages_" something and the other is
something "_size", I don't _know_.  

> +
> +	/* Walk the map again and get the size of the holes */
> +	first = 1;
> +	zholes_size[ZONE_DMA] = 0;
> +	zholes_size[ZONE_EASYRCLM] = 0;
> +	for (i = 1; init_node_data[i].end_pfn; i++) {
> +		unsigned long hole_size;
> +		if (init_node_data[i].nid != nid)
> +			continue;
> +
> +		if (first) {
> +			last_end_pfn = init_node_data[i].end_pfn;
> +			first = 0;
> +			continue;
> +		}
> +
> +		/* Hole found */
> +		hole_size = init_node_data[i].start_pfn - last_end_pfn;
> +		if (init_node_data[i].start_pfn < start_easyrclm_pfn) {
> +			zholes_size[ZONE_DMA] += hole_size;
> +		} else {
> +			zholes_size[ZONE_EASYRCLM] += hole_size;
> +		}
> +		last_end_pfn = init_node_data[i].end_pfn;
> +	}
> +}

I'd probably put this loop in another function.  It is pretty
self-contained, no?

-- Dave

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