linux-kernel - Re: [PATCH v2] properly reserve in bootmem the lmb reserved regions that cross NUMA nodes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <1223341431.8157.33.camel@pasglop>
Date:	Tue, 07 Oct 2008 12:03:51 +1100
From:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:	Jon Tollefson <kniht@...ux.vnet.ibm.com>
Cc:	linuxppc-dev <linuxppc-dev@...abs.org>,
	Linux Memory Management List <linux-mm@...ck.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Adam Litke <agl@...ibm.com>,
	Kumar Gala <galak@...nel.crashing.org>,
	Paul Mackerras <paulus@...ba.org>
Subject: Re: [PATCH v2] properly reserve in bootmem the lmb reserved
	regions that cross NUMA nodes

Minor nits ...

One is, you add this helper to mm/page_alloc.c, which means that I'll
need some ack from Hugh or Andrew before I can merge that via the
powerpc tree... Unless there's another user, I'd rather keep the
helper function in powerpc code for now, it can be moved to common
code later if somebody needs something similar.

> +	/* Mark reserved regions */
> +	for (i = 0; i < lmb.reserved.cnt; i++) {
> +		unsigned long physbase = lmb.reserved.region[i].base;
> +		unsigned long size = lmb.reserved.region[i].size;
> +		unsigned long start_pfn = physbase >> PAGE_SHIFT;
> +		unsigned long end_pfn = ((physbase + size - 1) >> PAGE_SHIFT);
> +		struct node_active_region *node_ar;

I'm not too happy wit the fact that something called "end_pfn" is
sometimes inclusive and sometime exclusive.

IE. From your implementation of get_node_active_region() it looks like
early_node_map[i].end_pfn isn't part of the range (exclusive) while
in your loop, the way you define end_pfn to be base + size - 1 means
it's part of the range (inclusive). That subtle distinction makes it
harder to understand the logic and is bug prone.

> +		node_ar = get_node_active_region(start_pfn);
> +		while (start_pfn < end_pfn && node_ar != NULL) {
> +			/*
> +			 * if reserved region extends past active region
> +			 * then trim size to active region
> +			 */
> +			if (end_pfn >= node_ar->end_pfn)

So the above test is correct, but it took me two more brain cells to
figure it out than necessary :-)

> +				size = (node_ar->end_pfn << PAGE_SHIFT)
> +					- (start_pfn << PAGE_SHIFT);
> +			dbg("reserve_bootmem %lx %lx nid=%d\n", physbase, size,
> +				node_ar->nid);
> +			reserve_bootmem_node(NODE_DATA(node_ar->nid), physbase,
> +						size, BOOTMEM_DEFAULT);
> +			/*
> +			 * if reserved region extends past the active region
> +			 * then get next active region that contains
> +			 *        this reserved region
> +			 */
> +			if (end_pfn >= node_ar->end_pfn) {
> +				start_pfn = node_ar->end_pfn;
> +				physbase = start_pfn << PAGE_SHIFT;
> +				node_ar = get_node_active_region(start_pfn);
> +			} else
> +				break;
>  		}
Minor nit but the above would look nicer if you wrote instead

			if (end_pfn < node_ar->end_pfn)
				break;
			start_pfn = ...
 
> +	}
> +
> +	for_each_online_node(nid) {
>  		sparse_memory_present_with_active_regions(nid);
>  	}
>  }

And you can remove the { and } above.

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/