[RFC] Reverting NUMA-affine page table allocation

[RFC] Reverting NUMA-affine page table allocation

[Tejun Heo]

Hello,

I've been looking through NUMA-affine page table allocation code and
the proposed changes, and there currently are the a couple of
problems.

1. Holes or misaligned nodes will force use of smaller sized mappings.
   Patches to fix the problem have been posted by Yinghai[1].

2. find_early_table_space() always calculates the amount of the needed
   space from 0 to the specified @end.  As nodes are registered, each
   node would try to allocate accumulative amount of space for page
   table.  This probably wouldn't cause any actual problem (may affect
   emulated configurations a bit tho).

IMHO, it would be better to avoid adding fixes for #1 and #2 at this
stage as we're very close to the next merge window and this is
(somewhat unnecessarily) delicate piece of code.  Also, I do think
that the NUMA affine page table allocation is generally overdone given
its limited usefulness when 1GiB mapping is available.

I'd like to revert NUMA-affine page table allocation for now and come
back to it in the next devel cycle.  Thanks to the memblock top-down
change, the RED-PEN condition (page table ending up in DMA memory)
doesn't exist with or without NUMA affine allocation and the only
downside of reverting would be page tables allocated in foreign nodes
on machines which don't support 1GiB mapping.

What do you think?

Thanks.

-- 
tejun

[1] http://thread.gmane.org/gmane.linux.kernel/1104672

Re: [RFC] Reverting NUMA-affine page table allocation

[Tejun Heo]

On Sat, Feb 26, 2011 at 03:37:30PM +0100, Tejun Heo wrote:
> I'd like to revert NUMA-affine page table allocation for now and come
> back to it in the next devel cycle.  Thanks to the memblock top-down
> change, the RED-PEN condition (page table ending up in DMA memory)
> doesn't exist with or without NUMA affine allocation and the only
> downside of reverting would be page tables allocated in foreign nodes
> on machines which don't support 1GiB mapping.
> 
> What do you think?

If there's no objection, I'll revert it later today and ask Ingo to
pull it.

Thanks.

-- 
tejun

Re: [RFC] Reverting NUMA-affine page table allocation

[Yinghai Lu]

On Sat, Feb 26, 2011 at 6:37 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> I've been looking through NUMA-affine page table allocation code and
> the proposed changes, and there currently are the a couple of
> problems.
>
> 1. Holes or misaligned nodes will force use of smaller sized mappings.
>   Patches to fix the problem have been posted by Yinghai[1].

	git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-2.6-yinghai.git
memblock

>
> 2. find_early_table_space() always calculates the amount of the needed
>   space from 0 to the specified @end.  As nodes are registered, each
>   node would try to allocate accumulative amount of space for page
>   table.  This probably wouldn't cause any actual problem (may affect
>   emulated configurations a bit tho).

interesting.

Yinghai

Re: [RFC] Reverting NUMA-affine page table allocation

[Tejun Heo]

Hey, Yinghai.

On Tue, Mar 01, 2011 at 02:41:55PM -0800, Yinghai Lu wrote:
> > 2. find_early_table_space() always calculates the amount of the needed
> >   space from 0 to the specified @end.  As nodes are registered, each
> >   node would try to allocate accumulative amount of space for page
> >   table.  This probably wouldn't cause any actual problem (may affect
> >   emulated configurations a bit tho).
> 
> interesting.

Yeah, it's a pretty interesting piece of code in need of some cleanup.

I'm reverting the NUMA-affine page table allocation for now.  I wish
something like the following happens during the next devel cycle.

* General cleanup of page table allocation code.  I'm not talking
  about major rewrite or anything major.  Just making the code sane(r)
  so that they take parameters instead of doing subtle stuff with
  internal #ifdef's.  And hopefully proper documentation.

* Redo the NUMA-affine page table allocation in simpler way using
  cleaned up allocation functions.  As suggested before, IMO, just
  taking top and bottom of nodes is enough.  Again, hopefully, with
  enough documentation on both what and why.

If you're interested in doing the above, please go ahead and let me
know.

Thank you.

-- 
tejun

Re: [RFC] Reverting NUMA-affine page table allocation

[Yinghai Lu]

On 03/02/2011 08:19 AM, Tejun Heo wrote:
> > Hey, Yinghai.
> >
> > On Tue, Mar 01, 2011 at 02:41:55PM -0800, Yinghai Lu wrote:
>>> >>> 2. find_early_table_space() always calculates the amount of the needed
>>> >>>   space from 0 to the specified @end.  As nodes are registered, each
>>> >>>   node would try to allocate accumulative amount of space for page
>>> >>>   table.  This probably wouldn't cause any actual problem (may affect
>>> >>>   emulated configurations a bit tho).
>> >>
>> >> interesting.
> >
> > Yeah, it's a pretty interesting piece of code in need of some cleanup.
No, i mean second one is not causing any problem.

that only take end is intentionally. that will make sure we can get
space for page table even in extreme case.

> >
> > I'm reverting the NUMA-affine page table allocation for now.  I wish
> > something like the following happens during the next devel cycle.
> >
> > * General cleanup of page table allocation code.  I'm not talking
> >   about major rewrite or anything major.  Just making the code sane(r)
> >   so that they take parameters instead of doing subtle stuff with
> >   internal #ifdef's.  And hopefully proper documentation.
> >
> > * Redo the NUMA-affine page table allocation in simpler way using
> >   cleaned up allocation functions.  As suggested before, IMO, just
> >   taking top and bottom of nodes is enough.  Again, hopefully, with
> >   enough documentation on both what and why.
> >
> > If you're interested in doing the above, please go ahead and let me
> > know.

No. please don't revert it.

Ingo, please get patches that will align to 1G ... from

	git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-2.6-yinghai.git
memblock

Thanks

Yinghai

Re: [RFC] Reverting NUMA-affine page table allocation

[Tejun Heo]

Hello,

On Wed, Mar 02, 2011 at 08:47:54AM -0800, Yinghai Lu wrote:
> > > Yeah, it's a pretty interesting piece of code in need of some cleanup.
> No, i mean second one is not causing any problem.
> 
> that only take end is intentionally. that will make sure we can get
> space for page table even in extreme case.

Hmmm... it seems like it only took @end because it always allocated
the whole page table in one go.  Always from the bottom to top.  Am I
missing something?

> > > If you're interested in doing the above, please go ahead and let me
> > > know.
> 
> No. please don't revert it.
> 
> Ingo, please get patches that will align to 1G ... from
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-2.6-yinghai.git memblock

If you were gonna do that, you could have just said that you objected
to the reverting.  The message was Request for Comments - soliciting
responses.

As I wrote multiple times, I think the code as implemented is a bit
heavy handed for the problem.  And a bigger problem for me is that it
kind of just piles on the existing messiness and worsens it.  I really
hope we wouldn't be doing that anymore.

I tried to clean up the page table allocation code but the necessary
changes felt a bit too large at this stage, so IMO that's best left to
the next cycle.

To me, it seems complicated for not good enough reasons.  I'll defer
the decision to x86 maintainers.  Ingo, hpa, Thomas, what do you guys
think?

Thanks.

-- 
tejun

Re: [RFC] Reverting NUMA-affine page table allocation

[Ingo Molnar]

* Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> On Wed, Mar 02, 2011 at 08:47:54AM -0800, Yinghai Lu wrote:
> > > > Yeah, it's a pretty interesting piece of code in need of some cleanup.
> > No, i mean second one is not causing any problem.
> > 
> > that only take end is intentionally. that will make sure we can get
> > space for page table even in extreme case.
> 
> Hmmm... it seems like it only took @end because it always allocated
> the whole page table in one go.  Always from the bottom to top.  Am I
> missing something?
> 
> > > > If you're interested in doing the above, please go ahead and let me
> > > > know.
> > 
> > No. please don't revert it.
> > 
> > Ingo, please get patches that will align to 1G ... from
> > 
> > 	git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-2.6-yinghai.git memblock
> 
> If you were gonna do that, you could have just said that you objected
> to the reverting.  The message was Request for Comments - soliciting
> responses.
> 
> As I wrote multiple times, I think the code as implemented is a bit
> heavy handed for the problem.  And a bigger problem for me is that it
> kind of just piles on the existing messiness and worsens it.  I really
> hope we wouldn't be doing that anymore.
> 
> I tried to clean up the page table allocation code but the necessary
> changes felt a bit too large at this stage, so IMO that's best left to
> the next cycle.

Do you plan to implement it more cleanly?

> To me, it seems complicated for not good enough reasons.  I'll defer
> the decision to x86 maintainers.  Ingo, hpa, Thomas, what do you guys
> think?

Would be nice to see an actual patch that does the revert.

Thanks,

	Ingo

Re: [RFC] Reverting NUMA-affine page table allocation

[H. Peter Anvin]

On 03/02/2011 10:08 AM, Ingo Molnar wrote:
> 
>> To me, it seems complicated for not good enough reasons.  I'll defer
>> the decision to x86 maintainers.  Ingo, hpa, Thomas, what do you guys
>> think?
> 
> Would be nice to see an actual patch that does the revert.
> 

I also would like to point out that we are at -rc7.  The time to test
new things is quickly running out, so a more conservative option for
this upcoming merge window is starting to look good.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [RFC] Reverting NUMA-affine page table allocation

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 03/02/2011 10:08 AM, Ingo Molnar wrote:
> > 
> >> To me, it seems complicated for not good enough reasons.  I'll defer
> >> the decision to x86 maintainers.  Ingo, hpa, Thomas, what do you guys
> >> think?
> > 
> > Would be nice to see an actual patch that does the revert.
> > 
> 
> I also would like to point out that we are at -rc7.  The time to test
> new things is quickly running out, so a more conservative option for
> this upcoming merge window is starting to look good.

Hm, i thought the revert was for tip:x86/mm, not x86/urgent. The development window 
for 2.6.39 is closing down.

A revert (without an actual bug that it fixes) for -rc8 is definitely out of 
question.

Thanks,

	Ingo

Re: [RFC] Reverting NUMA-affine page table allocation

[Tejun Heo]

On Wed, Mar 02, 2011 at 07:19:41PM +0100, Ingo Molnar wrote:
> Hm, i thought the revert was for tip:x86/mm, not x86/urgent. The development window 
> for 2.6.39 is closing down.
> 
> A revert (without an actual bug that it fixes) for -rc8 is definitely out of 
> question.

It is for x86/mm and it does address the unnecessarily smaller mapping
problem, which Yinghai's further patches address too.  Those patches
are what triggered me to ask for a revert because they kinda show how
ugly the underlying code is and add on top of it.

Thanks.

-- 
tejun

Re: [RFC] Reverting NUMA-affine page table allocation

[Ingo Molnar]

* Tejun Heo <tj@kernel.org> wrote:

> On Wed, Mar 02, 2011 at 07:19:41PM +0100, Ingo Molnar wrote:
> > Hm, i thought the revert was for tip:x86/mm, not x86/urgent. The development window 
> > for 2.6.39 is closing down.
> > 
> > A revert (without an actual bug that it fixes) for -rc8 is definitely out of 
> > question.
> 
> It is for x86/mm and it does address the unnecessarily smaller mapping
> problem, which Yinghai's further patches address too.  Those patches
> are what triggered me to ask for a revert because they kinda show how
> ugly the underlying code is and add on top of it.

Yeah - i agree with you and hpa that deferring this to .40 is the most sensible 
approach.

The splitting up into small regions side-effect was rather nasty to begin with - it 
would have taken who knows how many cycles for someone to notice that based on raw 
analysis of the pagetables ...

We need to slow down here for .39.

Thanks,

	Ingo

Re: [RFC] Reverting NUMA-affine page table allocation

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * H. Peter Anvin <hpa@zytor.com> wrote:
> 
> > On 03/02/2011 10:08 AM, Ingo Molnar wrote:
> > > 
> > >> To me, it seems complicated for not good enough reasons.  I'll defer
> > >> the decision to x86 maintainers.  Ingo, hpa, Thomas, what do you guys
> > >> think?
> > > 
> > > Would be nice to see an actual patch that does the revert.
> > > 
> > 
> > I also would like to point out that we are at -rc7.  The time to test
> > new things is quickly running out, so a more conservative option for
> > this upcoming merge window is starting to look good.
> 
> Hm, i thought the revert was for tip:x86/mm, not x86/urgent. [...]

On a second reading you suggested the same, so please ignore my mail! :-)

Thanks,

	Ingo

Re: [RFC] Reverting NUMA-affine page table allocation

[Tejun Heo]

Hello,

On Wed, Mar 02, 2011 at 07:08:27PM +0100, Ingo Molnar wrote:
> > I tried to clean up the page table allocation code but the necessary
> > changes felt a bit too large at this stage, so IMO that's best left to
> > the next cycle.
> 
> Do you plan to implement it more cleanly?

Yeah, that's the plan.  I want the page table allocation code cleaned
up before doing this.

I also want to take a dumber/simpler approach at the expense of some
disadvantage to machines with interleaved NUMA nodes which can't do
1GiB mappings.  If this scenario is a real concern, which I'm doubtful
about but then again it could be, we can do the callback walking thing
but I'd at least want to know that that's an actual concern we need to
address.

> > To me, it seems complicated for not good enough reasons.  I'll defer
> > the decision to x86 maintainers.  Ingo, hpa, Thomas, what do you guys
> > think?
> 
> Would be nice to see an actual patch that does the revert.

Here it is.

Thanks.

diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
index 97e6007..bce688d 100644
--- a/arch/x86/include/asm/page_types.h
+++ b/arch/x86/include/asm/page_types.h
@@ -54,8 +54,6 @@ static inline phys_addr_t get_max_mapped(void)
 extern unsigned long init_memory_mapping(unsigned long start,
 					 unsigned long end);
 
-void init_memory_mapping_high(void);
-
 extern void initmem_init(void);
 extern void free_initmem(void);
 
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 46e684f..c3a606c 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -963,6 +963,14 @@ void __init setup_arch(char **cmdline_p)
 	max_low_pfn_mapped = init_memory_mapping(0, max_low_pfn<<PAGE_SHIFT);
 	max_pfn_mapped = max_low_pfn_mapped;
 
+#ifdef CONFIG_X86_64
+	if (max_pfn > max_low_pfn) {
+		max_pfn_mapped = init_memory_mapping(1UL<<32,
+						     max_pfn<<PAGE_SHIFT);
+		/* can we preseve max_low_pfn ?*/
+		max_low_pfn = max_pfn;
+	}
+#endif
 	memblock.current_limit = get_max_mapped();
 
 	/*
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 470cc47..c8813aa 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -606,63 +606,9 @@ kernel_physical_mapping_init(unsigned long start,
 void __init initmem_init(void)
 {
 	memblock_x86_register_active_regions(0, 0, max_pfn);
-	init_memory_mapping_high();
 }
 #endif
 
-struct mapping_work_data {
-	unsigned long start;
-	unsigned long end;
-	unsigned long pfn_mapped;
-};
-
-static int __init_refok
-mapping_work_fn(unsigned long start_pfn, unsigned long end_pfn, void *datax)
-{
-	struct mapping_work_data *data = datax;
-	unsigned long pfn_mapped;
-	unsigned long final_start, final_end;
-
-	final_start = max_t(unsigned long, start_pfn<<PAGE_SHIFT, data->start);
-	final_end = min_t(unsigned long, end_pfn<<PAGE_SHIFT, data->end);
-
-	if (final_end <= final_start)
-		return 0;
-
-	pfn_mapped = init_memory_mapping(final_start, final_end);
-
-	if (pfn_mapped > data->pfn_mapped)
-		data->pfn_mapped = pfn_mapped;
-
-	return 0;
-}
-
-static unsigned long __init_refok
-init_memory_mapping_active_regions(unsigned long start, unsigned long end)
-{
-	struct mapping_work_data data;
-
-	data.start = start;
-	data.end = end;
-	data.pfn_mapped = 0;
-
-	work_with_active_regions(MAX_NUMNODES, mapping_work_fn, &data);
-
-	return data.pfn_mapped;
-}
-
-void __init_refok init_memory_mapping_high(void)
-{
-	if (max_pfn > max_low_pfn) {
-		max_pfn_mapped = init_memory_mapping_active_regions(1UL<<32,
-							 max_pfn<<PAGE_SHIFT);
-		/* can we preserve max_low_pfn ? */
-		max_low_pfn = max_pfn;
-
-		memblock.current_limit = get_max_mapped();
-	}
-}
-
 void __init paging_init(void)
 {
 	unsigned long max_zone_pfns[MAX_NR_ZONES];
diff --git a/arch/x86/mm/numa_64.c b/arch/x86/mm/numa_64.c
index 74064e8..86491ba 100644
--- a/arch/x86/mm/numa_64.c
+++ b/arch/x86/mm/numa_64.c
@@ -543,8 +543,6 @@ static int __init numa_register_memblks(struct numa_meminfo *mi)
 	if (!numa_meminfo_cover_memory(mi))
 		return -EINVAL;
 
-	init_memory_mapping_high();
-
 	/* Finally register nodes. */
 	for_each_node_mask(nid, node_possible_map) {
 		u64 start = (u64)max_pfn << PAGE_SHIFT;

-- 
tejun