| Message ID | 1415249414-20888-1-git-send-email-daniel@numascale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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.
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
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?
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
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... :-)
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
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.
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;
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(-)