[PATCH] mm/thp: Make ARCH_ENABLE_SPLIT_PMD_PTLOCK dependent on PGTABLE_LEVELS > 2

[PATCH] mm/thp: Make ARCH_ENABLE_SPLIT_PMD_PTLOCK dependent on PGTABLE_LEVELS > 2

[Anshuman Khandual]

ARCH_ENABLE_SPLIT_PMD_PTLOCK is irrelevant unless there are two page table
levels including PMD (also per Documentation/vm/split_page_table_lock.rst).
Make this dependency explicit on remaining platforms i.e x86 and s390 where
ARCH_ENABLE_SPLIT_PMD_PTLOCK is subscribed.

Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: x86@kernel.org
Cc: linux-s390@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/s390/Kconfig | 2 +-
 arch/x86/Kconfig  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index b4c7c34069f8..fcc1ea339a9d 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -62,7 +62,7 @@ config S390
 	select ARCH_BINFMT_ELF_STATE
 	select ARCH_ENABLE_MEMORY_HOTPLUG if SPARSEMEM
 	select ARCH_ENABLE_MEMORY_HOTREMOVE
-	select ARCH_ENABLE_SPLIT_PMD_PTLOCK
+	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
 	select ARCH_HAS_DEBUG_VM_PGTABLE
 	select ARCH_HAS_DEBUG_WX
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0045e1b44190..ec9e9d3d7e3f 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -63,7 +63,7 @@ config X86
 	select ARCH_ENABLE_HUGEPAGE_MIGRATION if X86_64 && HUGETLB_PAGE && MIGRATION
 	select ARCH_ENABLE_MEMORY_HOTPLUG if X86_64 || (X86_32 && HIGHMEM)
 	select ARCH_ENABLE_MEMORY_HOTREMOVE if MEMORY_HOTPLUG
-	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if X86_64 || X86_PAE
+	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if (PGTABLE_LEVELS > 2) && (X86_64 || X86_PAE)
 	select ARCH_ENABLE_THP_MIGRATION if X86_64 && TRANSPARENT_HUGEPAGE
 	select ARCH_HAS_ACPI_TABLE_UPGRADE	if ACPI
 	select ARCH_HAS_CACHE_LINE_SIZE
-- 
2.20.1

Re: [PATCH] mm/thp: Make ARCH_ENABLE_SPLIT_PMD_PTLOCK dependent on PGTABLE_LEVELS > 2

[Peter Zijlstra]

On Mon, May 10, 2021 at 10:05:45AM +0530, Anshuman Khandual wrote:
> -	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if X86_64 || X86_PAE
> +	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if (PGTABLE_LEVELS > 2) && (X86_64 || X86_PAE)

It's still very early on a Monday, but IIRC this new condition is
identical to the pre-existing one.

Re: [PATCH] mm/thp: Make ARCH_ENABLE_SPLIT_PMD_PTLOCK dependent on PGTABLE_LEVELS > 2

[Anshuman Khandual]

On 5/10/21 2:23 PM, Peter Zijlstra wrote:
> On Mon, May 10, 2021 at 10:05:45AM +0530, Anshuman Khandual wrote:
>> -	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if X86_64 || X86_PAE
>> +	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if (PGTABLE_LEVELS > 2) && (X86_64 || X86_PAE)
> 
> It's still very early on a Monday, but IIRC this new condition is
> identical to the pre-existing one.

Did not get it, could you please elaborate ?

Re: [PATCH] mm/thp: Make ARCH_ENABLE_SPLIT_PMD_PTLOCK dependent on PGTABLE_LEVELS > 2

[Oscar Salvador]

On Mon, May 10, 2021 at 03:36:29PM +0530, Anshuman Khandual wrote:
> 
> 
> On 5/10/21 2:23 PM, Peter Zijlstra wrote:
> > On Mon, May 10, 2021 at 10:05:45AM +0530, Anshuman Khandual wrote:
> >> -	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if X86_64 || X86_PAE
> >> +	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if (PGTABLE_LEVELS > 2) && (X86_64 || X86_PAE)
> > 
> > It's still very early on a Monday, but IIRC this new condition is
> > identical to the pre-existing one.
> 
> Did not get it, could you please elaborate ?

When using x86_PAE, you must have more than two pgtable levels, right?
And not speaking of x86_64.

-- 
Oscar Salvador
SUSE L3

Re: [PATCH] mm/thp: Make ARCH_ENABLE_SPLIT_PMD_PTLOCK dependent on PGTABLE_LEVELS > 2

[Anshuman Khandual]

On 5/10/21 3:40 PM, Oscar Salvador wrote:
> On Mon, May 10, 2021 at 03:36:29PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 5/10/21 2:23 PM, Peter Zijlstra wrote:
>>> On Mon, May 10, 2021 at 10:05:45AM +0530, Anshuman Khandual wrote:
>>>> -	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if X86_64 || X86_PAE
>>>> +	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if (PGTABLE_LEVELS > 2) && (X86_64 || X86_PAE)
>>>
>>> It's still very early on a Monday, but IIRC this new condition is
>>> identical to the pre-existing one.
>>
>> Did not get it, could you please elaborate ?
> 
> When using x86_PAE, you must have more than two pgtable levels, right?
> And not speaking of x86_64.

arch/x86/Kconfig..

config PGTABLE_LEVELS
        int
        default 5 if X86_5LEVEL
        default 4 if X86_64
        default 3 if X86_PAE
        default 2

Both X86_PAE and X86_64 will always have page table levels > 2 ? But
regardless, it might be still useful to assert (PGTABLE_LEVELS > 2)
before selecting ARCH_ENABLE_SPLIT_PMD_PTLOCK.

Re: [PATCH] mm/thp: Make ARCH_ENABLE_SPLIT_PMD_PTLOCK dependent on PGTABLE_LEVELS > 2

[Anshuman Khandual]

On 5/10/21 3:46 PM, Anshuman Khandual wrote:
> 
> 
> On 5/10/21 3:40 PM, Oscar Salvador wrote:
>> On Mon, May 10, 2021 at 03:36:29PM +0530, Anshuman Khandual wrote:
>>>
>>>
>>> On 5/10/21 2:23 PM, Peter Zijlstra wrote:
>>>> On Mon, May 10, 2021 at 10:05:45AM +0530, Anshuman Khandual wrote:
>>>>> -	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if X86_64 || X86_PAE
>>>>> +	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if (PGTABLE_LEVELS > 2) && (X86_64 || X86_PAE)
>>>>
>>>> It's still very early on a Monday, but IIRC this new condition is
>>>> identical to the pre-existing one.
>>>
>>> Did not get it, could you please elaborate ?
>>
>> When using x86_PAE, you must have more than two pgtable levels, right?
>> And not speaking of x86_64.
> 
> arch/x86/Kconfig..
> 
> config PGTABLE_LEVELS
>         int
>         default 5 if X86_5LEVEL
>         default 4 if X86_64
>         default 3 if X86_PAE
>         default 2
> 
> Both X86_PAE and X86_64 will always have page table levels > 2 ? But
> regardless, it might be still useful to assert (PGTABLE_LEVELS > 2)
> before selecting ARCH_ENABLE_SPLIT_PMD_PTLOCK.

PGTABLE_LEVELS > 2 is a necessary condition for this PMD split lock
config. The problem is that for arch selectable configs like this,
conditional statements would not work properly when defined along
with the config. Otherwise the following change would have been
sufficient.

diff --git a/mm/Kconfig b/mm/Kconfig
index 02d44e3420f5..5830ea7746b3 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -214,6 +214,7 @@ config SPLIT_PTLOCK_CPUS
 
 config ARCH_ENABLE_SPLIT_PMD_PTLOCK
        bool
+       depends on PGTABLE_LEVELS > 2

Hence this just moves the condition to all subscribing platforms
while making the selection for ARCH_ENABLE_SPLIT_PMD_PTLOCK.

Re: [PATCH] mm/thp: Make ARCH_ENABLE_SPLIT_PMD_PTLOCK dependent on PGTABLE_LEVELS > 2

[Gerald Schaefer]

On Mon, 17 May 2021 09:45:31 +0530
Anshuman Khandual <anshuman.khandual@arm.com> wrote:

> 
> 
> On 5/10/21 10:05 AM, Anshuman Khandual wrote:
> > ARCH_ENABLE_SPLIT_PMD_PTLOCK is irrelevant unless there are two page table
> > levels including PMD (also per Documentation/vm/split_page_table_lock.rst).
> > Make this dependency explicit on remaining platforms i.e x86 and s390 where
> > ARCH_ENABLE_SPLIT_PMD_PTLOCK is subscribed.

For s390, I don't think this makes a lot of sense. We always have 5 levels
defined for PGTABLE_LEVELS, and we would not even compile with any other
value, because of the "#error CONFIG_PGTABLE_LEVELS" in include/linux/pgtable.h.

Our pagetable folding also works a bit different than it does on other archs,
and we would actually have pmd level entries for 2-level pagetables, so it should
all work fine also with PGTABLE_LEVELS == 2 (if it was possible).

In fact, I do not really see why you would need "more than two levels" on any
arch, in order to use split PMD locks. Your description also just says
"irrelevant unless there are two page table levels", and not "more than two
levels", like in Documentation/vm/split_page_table_lock.rst.

Yet, your patch adds checks for "more than", so at least the description
seems a bit misleading. I assume that the "more than" has to do with folded
PMD on a 2-level system, but the way we fold on s390 I do not see why that
should be a problem. Could you please elaborate a bit?

We also have different levels of pagetables for kernel (CONFIG_PGTABLE_LEVELS)
and user processes on s390. The latter can have dynamic levels, currently
starting with 3, but previously we also had 2 levels for compat tasks e.g.
These dynamic levels for user processes are also independent from the
CONFIG_PGTABLE_LEVELS used for the kernel pagetable, while the split PMD lock
of course also affects user process pagetables, so that would be another
reason not to add such a dependency for ARCH_ENABLE_SPLIT_PMD_PTLOCK on s390.

Re: [PATCH] mm/thp: Make ARCH_ENABLE_SPLIT_PMD_PTLOCK dependent on PGTABLE_LEVELS > 2

[Gerald Schaefer]

On Mon, 17 May 2021 16:13:57 +0200
Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote:

> On Mon, 17 May 2021 09:45:31 +0530
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> > 
> > 
> > On 5/10/21 10:05 AM, Anshuman Khandual wrote:  
> > > ARCH_ENABLE_SPLIT_PMD_PTLOCK is irrelevant unless there are two page table
> > > levels including PMD (also per Documentation/vm/split_page_table_lock.rst).
> > > Make this dependency explicit on remaining platforms i.e x86 and s390 where
> > > ARCH_ENABLE_SPLIT_PMD_PTLOCK is subscribed.  
> 
> For s390, I don't think this makes a lot of sense. We always have 5 levels
> defined for PGTABLE_LEVELS, and we would not even compile with any other
> value, because of the "#error CONFIG_PGTABLE_LEVELS" in include/linux/pgtable.h.
> 
> Our pagetable folding also works a bit different than it does on other archs,
> and we would actually have pmd level entries for 2-level pagetables, so it should
> all work fine also with PGTABLE_LEVELS == 2 (if it was possible).
> 
> In fact, I do not really see why you would need "more than two levels" on any
> arch, in order to use split PMD locks. Your description also just says
> "irrelevant unless there are two page table levels", and not "more than two
> levels", like in Documentation/vm/split_page_table_lock.rst.
> 
> Yet, your patch adds checks for "more than", so at least the description
> seems a bit misleading. I assume that the "more than" has to do with folded
> PMD on a 2-level system, but the way we fold on s390 I do not see why that
> should be a problem. Could you please elaborate a bit?
> 
> We also have different levels of pagetables for kernel (CONFIG_PGTABLE_LEVELS)
> and user processes on s390. The latter can have dynamic levels, currently
> starting with 3, but previously we also had 2 levels for compat tasks e.g.
> These dynamic levels for user processes are also independent from the
> CONFIG_PGTABLE_LEVELS used for the kernel pagetable, while the split PMD lock
> of course also affects user process pagetables, so that would be another
> reason not to add such a dependency for ARCH_ENABLE_SPLIT_PMD_PTLOCK on s390.

Ouch, I guess I was a bit confused here. I thought the split PMD lock
was part of the struct page for the 4 KB page where the PMD entry is located,
and therefore, with more than one page, it still would make (a little) sense
to use it also for 2 pagetable levels.

However, pmd_to_page() always returns the struct page of the first page,
so there is only one split PMD lock for the whole thing (4 pages for s390).
Of course that means that with 2 pagetable levels, and only one PMD directory,
the split PMD lock would be equivalent to the global pagetable lock, and
therefore not make any sense.

Maybe you could change the description to also mention "more than two"
levels?

I still do not see a real benefit of the patch, e.g. it does not really
fix any possible misconfiguration, at least on s390. But it certainly is not
wrong, and at least it had the benefit of making me aware again of how split
PMD locks work, so I'll happily add this

Acked-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com> # s390

Re: [PATCH] mm/thp: Make ARCH_ENABLE_SPLIT_PMD_PTLOCK dependent on PGTABLE_LEVELS > 2

[Anshuman Khandual]

On 5/19/21 4:37 PM, Gerald Schaefer wrote:
> On Mon, 17 May 2021 16:13:57 +0200
> Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote:
> 
>> On Mon, 17 May 2021 09:45:31 +0530
>> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>>
>>>
>>>
>>> On 5/10/21 10:05 AM, Anshuman Khandual wrote:  
>>>> ARCH_ENABLE_SPLIT_PMD_PTLOCK is irrelevant unless there are two page table
>>>> levels including PMD (also per Documentation/vm/split_page_table_lock.rst).
>>>> Make this dependency explicit on remaining platforms i.e x86 and s390 where
>>>> ARCH_ENABLE_SPLIT_PMD_PTLOCK is subscribed.  
>>
>> For s390, I don't think this makes a lot of sense. We always have 5 levels
>> defined for PGTABLE_LEVELS, and we would not even compile with any other
>> value, because of the "#error CONFIG_PGTABLE_LEVELS" in include/linux/pgtable.h.
>>
>> Our pagetable folding also works a bit different than it does on other archs,
>> and we would actually have pmd level entries for 2-level pagetables, so it should
>> all work fine also with PGTABLE_LEVELS == 2 (if it was possible).
>>
>> In fact, I do not really see why you would need "more than two levels" on any
>> arch, in order to use split PMD locks. Your description also just says
>> "irrelevant unless there are two page table levels", and not "more than two
>> levels", like in Documentation/vm/split_page_table_lock.rst.
>>
>> Yet, your patch adds checks for "more than", so at least the description
>> seems a bit misleading. I assume that the "more than" has to do with folded
>> PMD on a 2-level system, but the way we fold on s390 I do not see why that
>> should be a problem. Could you please elaborate a bit?
>>
>> We also have different levels of pagetables for kernel (CONFIG_PGTABLE_LEVELS)
>> and user processes on s390. The latter can have dynamic levels, currently
>> starting with 3, but previously we also had 2 levels for compat tasks e.g.
>> These dynamic levels for user processes are also independent from the
>> CONFIG_PGTABLE_LEVELS used for the kernel pagetable, while the split PMD lock
>> of course also affects user process pagetables, so that would be another
>> reason not to add such a dependency for ARCH_ENABLE_SPLIT_PMD_PTLOCK on s390.
> 
> Ouch, I guess I was a bit confused here. I thought the split PMD lock
> was part of the struct page for the 4 KB page where the PMD entry is located,
> and therefore, with more than one page, it still would make (a little) sense
> to use it also for 2 pagetable levels.
> 
> However, pmd_to_page() always returns the struct page of the first page,
> so there is only one split PMD lock for the whole thing (4 pages for s390).
> Of course that means that with 2 pagetable levels, and only one PMD directory,
> the split PMD lock would be equivalent to the global pagetable lock, and
> therefore not make any sense.
> 
> Maybe you could change the description to also mention "more than two"
> levels?

Yes, will change it.

> 
> I still do not see a real benefit of the patch, e.g. it does not really
> fix any possible misconfiguration, at least on s390. But it certainly is not
> wrong, and at least it had the benefit of making me aware again of how split
> PMD locks work, so I'll happily add this

Right, even though it does not change the functionality, the purpose
of this patch is to enforce (and also possibly document) an inherent
assumption which may not hold true on all other platforms like arm64.

> 
> Acked-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com> # s390
> 

Thanks for reviewing.