On Tue, Oct 24, 2017 at 02:47:41PM +0200, Ingo Molnar wrote:

quoted

quoted

quoted

quoted

quoted

quoted

Making a variable that 'looks' like a constant macro dynamic in a rare Kconfig 
scenario is asking for trouble.

We expect boot-time page mode switching to be enabled in kernel of next
generation enterprise distros. It shoudn't be that rare.

My point remains even with not-so-rare Kconfig dependency.

I don't follow how introducing new variable that depends on Kconfig option
would help with the situation.

A new, properly named variable or function (max_physmem_bits or 
max_physmem_bits()) that is not all uppercase would make it abundantly clear that 
it is not a constant but a runtime value.

Would we need to rename every uppercase macros that would depend on
max_physmem_bits()? Like MAXMEM.

MAXMEM isn't used in too many places either - what's the total impact of it?

The impact is not very small. The tree of macros dependent on
MAX_PHYSMEM_BITS:

MAX_PHYSMEM_BITS
  MAXMEM
    KEXEC_SOURCE_MEMORY_LIMIT
    KEXEC_DESTINATION_MEMORY_LIMIT
    KEXEC_CONTROL_MEMORY_LIMIT
  SECTIONS_SHIFT
    ZONEID_SHIFT
      ZONEID_PGSHIFT
      ZONEID_MASK

The total number of users of them is not large. It's doable. But I expect
it to be somewhat ugly, since we're partly in generic code and it would
require some kind of compatibility layer for other archtectures.

Do you want me to rename them all?

quoted

quoted

quoted

We would end up with inverse situation: people would use MAX_PHYSMEM_BITS
where the new variable need to be used and we will in the same situation.

It should result in sub-optimal resource allocations worst-case, right?

I don't think it's the worst case.

For instance, virt_addr_valid() depends indirectly on it:

  virt_addr_valid()
    __virt_addr_valid()
      phys_addr_valid()
        boot_cpu_data.x86_phys_bits (initialized with MAX_PHYSMEM_BITS)

virt_addr_valid() is used in things like implementation /dev/kmem.

To me it's far more risky than occasional build breakage for
CONFIG_X86_5LEVEL=y.

So why do we have two variables here, one boot_cpu_data.x86_phys_bits and the 
other MAX_PHYSMEM_BITS - both set once during boot?

I'm trying to find a clean solution for this all - hiding a boot time dependency 
into a constant-looking value doesn't feel clean.

We already have plenty of them: PAGE_OFFSET, IA32_PAGE_OFFSET,
VMALLOC_START, VMEMMAP_START, TASK_SIZE, STACK_TOP, FIXADDR_TOP...

I don't understand why you make this one a special.

-- 
 Kirill A. Shutemov

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