[PATCH] x86: Drop redundant memory-block sizing code

[PATCH] x86: Drop redundant memory-block sizing code

[Daniel J Blueman]

Drop the unused code from selecting a fixed memory block size of 2GB
on large-memory x86-64 systems.

Signed-off-by: Daniel J Blueman <daniel@numascale.com>
---
 arch/x86/mm/init_64.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index ebca30f..09c0567 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1243,28 +1243,12 @@ int in_gate_area_no_mm(unsigned long addr)
 
 static unsigned long probe_memory_block_size(void)
 {
-	/* start from 2g */
-	unsigned long bz = 1UL<<31;
-
 	if (totalram_pages >= (64ULL << (30 - PAGE_SHIFT))) {
 		pr_info("Using 2GB memory block size for large-memory system\n");
 		return 2UL * 1024 * 1024 * 1024;
 	}
 
-	/* less than 64g installed */
-	if ((max_pfn << PAGE_SHIFT) < (16UL << 32))
-		return MIN_MEMORY_BLOCK_SIZE;
-
-	/* get the tail size */
-	while (bz > MIN_MEMORY_BLOCK_SIZE) {
-		if (!((max_pfn << PAGE_SHIFT) & (bz - 1)))
-			break;
-		bz >>= 1;
-	}
-
-	printk(KERN_DEBUG "memory block size : %ldMB\n", bz >> 20);
-
-	return bz;
+	return MIN_MEMORY_BLOCK_SIZE;
 }
 
 static unsigned long memory_block_size_probed;
-- 
1.9.1

Re: [PATCH] x86: Drop redundant memory-block sizing code

[Borislav Petkov]

On Thu, Nov 06, 2014 at 12:50:14PM +0800, Daniel J Blueman wrote:
> Drop the unused code from selecting a fixed memory block size of 2GB
> on large-memory x86-64 systems.
> 
> Signed-off-by: Daniel J Blueman <daniel@numascale.com>

This commit message is seriously lacking an explanation why? Why is it
unused, why is it ok on systems with mem < 64g, what is the problem it
solves, ...

Just ask yourself this when you write commit messages: would anyone else
be able to understand what this commit was improving when anyone reads
that commit message months, maybe years from now.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: Drop redundant memory-block sizing code

[Daniel J Blueman]

On 11/06/2014 05:40 PM, Borislav Petkov wrote:
> On Thu, Nov 06, 2014 at 12:50:14PM +0800, Daniel J Blueman wrote:
>> Drop the unused code from selecting a fixed memory block size of 2GB
>> on large-memory x86-64 systems.
>>
>> Signed-off-by: Daniel J Blueman <daniel@numascale.com>
>
> This commit message is seriously lacking an explanation why? Why is it
> unused, why is it ok on systems with mem < 64g, what is the problem it
> solves, ...
>
> Just ask yourself this when you write commit messages: would anyone else
> be able to understand what this commit was improving when anyone reads
> that commit message months, maybe years from now.

Yes, true. I am incorrectly assuming that someone is looking at the 
patch in-context, but perhaps better to write assuming the code isn't 
seen (or at least understood).

How is this?

As the first check for 64GB or larger memory returns a 2GB memory block 
size in that case, the following check for less than 64GB will always 
evaluate true, leading to unreachable code.

Remove the second and unnecessary condition and the code in the 
remainder of the function, as it therefore can't be reached.

Thanks,
   Daniel
-- 
Daniel J Blueman
Principal Software Engineer, Numascale

Re: [PATCH] x86: Drop redundant memory-block sizing code

[Borislav Petkov]

On Thu, Nov 06, 2014 at 06:33:40PM +0800, Daniel J Blueman wrote:
> As the first check for 64GB or larger memory returns a 2GB memory block size
> in that case, the following check for less than 64GB will always evaluate
> true, leading to unreachable code.

I'm reading this as this code is never running on systems < 64GB. Why is
that so?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: Drop redundant memory-block sizing code

[Daniel J Blueman]

On 11/06/2014 06:40 PM, Borislav Petkov wrote:
> On Thu, Nov 06, 2014 at 06:33:40PM +0800, Daniel J Blueman wrote:
>> As the first check for 64GB or larger memory returns a 2GB memory block size
>> in that case, the following check for less than 64GB will always evaluate
>> true, leading to unreachable code.
>
> I'm reading this as this code is never running on systems < 64GB. Why is
> that so?

Let me clarify that "Leading to" didn't mean "executing":

"As the first check for 64GB or larger memory returns a 2GB memory block 
size in that case, the following check for less than 64GB will always 
evaluate true and return MIN_MEMORY_BLOCK_SIZE, leading to unreachable code.

Remove the second and unnecessary condition and the code in the 
remainder of the function, as it therefore can't be reached."

Sheesh. Even Shakespeare would have trouble writing a exemplary changelog.

Thanks,
   Daniel
-- 
Daniel J Blueman
Principal Software Engineer, Numascale

Re: [PATCH] x86: Drop redundant memory-block sizing code

[Borislav Petkov]

On Thu, Nov 06, 2014 at 07:10:45PM +0800, Daniel J Blueman wrote:
> "As the first check for 64GB or larger memory returns a 2GB memory
> block size in that case, the following check for less than 64GB will
> always

Right, but why isn't there a simple else? Instead, the >64GB case is
looking at totalram_pages but the so-called else case is looking at
max_pfn. Why, what's the difference?

My purely hypothetical suspicion is this thing used to handle some
special case with memory holes where totalram_pages was still < 64GB but
max_pfn was above. I'm looking at this memory block size approximation
downwards which supposedly used to do something at some point, right?

Now, when you remove this, it doesn't do so anymore, potentially
breaking some machines.

Or is this simply unfortunate coding and totalram_pages and max_pfn are
equivalent?

Questions over questions... Maybe it is time for some git log
archeology...

:-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: Drop redundant memory-block sizing code

[Daniel J Blueman]

On 11/06/2014 07:56 PM, Borislav Petkov wrote:
> On Thu, Nov 06, 2014 at 07:10:45PM +0800, Daniel J Blueman wrote:
>> "As the first check for 64GB or larger memory returns a 2GB memory
>> block size in that case, the following check for less than 64GB will
>> always
>
> Right, but why isn't there a simple else? Instead, the >64GB case is
> looking at totalram_pages but the so-called else case is looking at
> max_pfn. Why, what's the difference?
>
> My purely hypothetical suspicion is this thing used to handle some
> special case with memory holes where totalram_pages was still < 64GB but
> max_pfn was above. I'm looking at this memory block size approximation
> downwards which supposedly used to do something at some point, right?
>
> Now, when you remove this, it doesn't do so anymore, potentially
> breaking some machines.
>
> Or is this simply unfortunate coding and totalram_pages and max_pfn are
> equivalent?
>
> Questions over questions... Maybe it is time for some git log
> archeology...

Yes, totalram_pages doesn't count the MMIO hole, whereas max_pfn does.

I've made NumaConnect firmware changes that will guarantee max_pfn is 
always aligned to at least 2GB, so 
bdee237c0343a5d1a6cf72c7ea68e88338b26e08 "x86: mm: Use 2GB memory block 
size on large-memory x86-64 systems" can be dropped and Yinghai's 
approach will give 2GB memory blocks on our systems.

Thanks,
   Daniel
-- 
Daniel J Blueman
Principal Software Engineer, Numascale

Re: [PATCH] x86: Drop redundant memory-block sizing code

[Borislav Petkov]

On Mon, Nov 10, 2014 at 05:03:16PM +0800, Daniel J Blueman wrote:
> Yes, totalram_pages doesn't count the MMIO hole, whereas max_pfn does.
> 
> I've made NumaConnect firmware changes that will guarantee max_pfn is always
> aligned to at least 2GB, so bdee237c0343a5d1a6cf72c7ea68e88338b26e08 "x86:
> mm: Use 2GB memory block size on large-memory x86-64 systems" can be dropped
> and Yinghai's approach will give 2GB memory blocks on our systems.

What about the rest of the systems? I.e., the !numascale ones. This is
generic code which needs to support all, not only your flavour.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--