Re: [PATCH 0/6] Boot-time switching between 4- and 5-level paging for 4.15, Part 1

[Kirill A. Shutemov]

On Tue, Oct 24, 2017 at 02:47:41PM +0200, Ingo Molnar wrote:
> > > > > > > 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?

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