[PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS

[PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS

[Anshuman Khandual]

Split ptlocks need not be defined and allocated unless they are being used.
ALLOC_SPLIT_PTLOCKS is inherently dependent on USE_SPLIT_PTE_PTLOCKS. This
just makes it explicit and clear. While here drop the spinlock_t element
from the struct page when USE_SPLIT_PTE_PTLOCKS is not enabled.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
This applies on v5.13-rc2.

Changes in V2:

- Dropped spinlock_t element from struct page

Changes in V1:

https://lore.kernel.org/linux-mm/1620618390-9999-1-git-send-email-anshuman.khandual@arm.com/

 include/linux/mm_types.h      | 2 ++
 include/linux/mm_types_task.h | 5 +++++
 mm/memory.c                   | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 5aacc1c10a45..a0fd851c0a0c 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -152,10 +152,12 @@ struct page {
 				struct mm_struct *pt_mm; /* x86 pgds only */
 				atomic_t pt_frag_refcount; /* powerpc */
 			};
+#if USE_SPLIT_PTE_PTLOCKS
 #if ALLOC_SPLIT_PTLOCKS
 			spinlock_t *ptl;
 #else
 			spinlock_t ptl;
+#endif
 #endif
 		};
 		struct {	/* ZONE_DEVICE pages */
diff --git a/include/linux/mm_types_task.h b/include/linux/mm_types_task.h
index c1bc6731125c..1b222f8039d1 100644
--- a/include/linux/mm_types_task.h
+++ b/include/linux/mm_types_task.h
@@ -22,7 +22,12 @@
 #define USE_SPLIT_PTE_PTLOCKS	(NR_CPUS >= CONFIG_SPLIT_PTLOCK_CPUS)
 #define USE_SPLIT_PMD_PTLOCKS	(USE_SPLIT_PTE_PTLOCKS && \
 		IS_ENABLED(CONFIG_ARCH_ENABLE_SPLIT_PMD_PTLOCK))
+
+#if USE_SPLIT_PTE_PTLOCKS
 #define ALLOC_SPLIT_PTLOCKS	(SPINLOCK_SIZE > BITS_PER_LONG/8)
+#else
+#define ALLOC_SPLIT_PTLOCKS	0
+#endif
 
 /*
  * The per task VMA cache array:
diff --git a/mm/memory.c b/mm/memory.c
index 730daa00952b..9c3b63f11aee 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5250,7 +5250,7 @@ long copy_huge_page_from_user(struct page *dst_page,
 }
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE || CONFIG_HUGETLBFS */
 
-#if USE_SPLIT_PTE_PTLOCKS && ALLOC_SPLIT_PTLOCKS
+#if ALLOC_SPLIT_PTLOCKS
 
 static struct kmem_cache *page_ptl_cachep;
 
-- 
2.20.1

Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS

[Matthew Wilcox]

On Wed, May 19, 2021 at 01:03:06PM +0530, Anshuman Khandual wrote:
> Split ptlocks need not be defined and allocated unless they are being used.
> ALLOC_SPLIT_PTLOCKS is inherently dependent on USE_SPLIT_PTE_PTLOCKS. This
> just makes it explicit and clear. While here drop the spinlock_t element
> from the struct page when USE_SPLIT_PTE_PTLOCKS is not enabled.

I didn't spot this email yesterday.  I'm not a fan.  Isn't struct page
already complicated enough without adding another ifdef to it?  Surely
there's a better way than this.

> +++ b/include/linux/mm_types.h
> @@ -152,10 +152,12 @@ struct page {
>  				struct mm_struct *pt_mm; /* x86 pgds only */
>  				atomic_t pt_frag_refcount; /* powerpc */
>  			};
> +#if USE_SPLIT_PTE_PTLOCKS
>  #if ALLOC_SPLIT_PTLOCKS
>  			spinlock_t *ptl;
>  #else
>  			spinlock_t ptl;
> +#endif
>  #endif
>  		};
>  		struct {	/* ZONE_DEVICE pages */

Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS

[Anshuman Khandual]

On 5/20/21 4:47 PM, Matthew Wilcox wrote:
> On Wed, May 19, 2021 at 01:03:06PM +0530, Anshuman Khandual wrote:
>> Split ptlocks need not be defined and allocated unless they are being used.
>> ALLOC_SPLIT_PTLOCKS is inherently dependent on USE_SPLIT_PTE_PTLOCKS. This
>> just makes it explicit and clear. While here drop the spinlock_t element
>> from the struct page when USE_SPLIT_PTE_PTLOCKS is not enabled.
> 
> I didn't spot this email yesterday.  I'm not a fan.  Isn't struct page
> already complicated enough without adding another ifdef to it?  Surely
> there's a better way than this.

This discussion thread just got dropped off the radar, sorry about it.
None of the spinlock_t elements are required unless split ptlocks are
in use. I understand your concern regarding yet another #ifdef in the
struct page definition. But this change is simple and minimal. Do you
have any other particular alternative in mind which I could explore ?

> 
>> +++ b/include/linux/mm_types.h
>> @@ -152,10 +152,12 @@ struct page {
>>  				struct mm_struct *pt_mm; /* x86 pgds only */
>>  				atomic_t pt_frag_refcount; /* powerpc */
>>  			};
>> +#if USE_SPLIT_PTE_PTLOCKS
>>  #if ALLOC_SPLIT_PTLOCKS
>>  			spinlock_t *ptl;
>>  #else
>>  			spinlock_t ptl;
>> +#endif
>>  #endif
>>  		};
>>  		struct {	/* ZONE_DEVICE pages */

Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS

[Matthew Wilcox]

On Thu, Jul 01, 2021 at 10:51:27AM +0530, Anshuman Khandual wrote:
> 
> 
> On 5/20/21 4:47 PM, Matthew Wilcox wrote:
> > On Wed, May 19, 2021 at 01:03:06PM +0530, Anshuman Khandual wrote:
> >> Split ptlocks need not be defined and allocated unless they are being used.
> >> ALLOC_SPLIT_PTLOCKS is inherently dependent on USE_SPLIT_PTE_PTLOCKS. This
> >> just makes it explicit and clear. While here drop the spinlock_t element
> >> from the struct page when USE_SPLIT_PTE_PTLOCKS is not enabled.
> > 
> > I didn't spot this email yesterday.  I'm not a fan.  Isn't struct page
> > already complicated enough without adding another ifdef to it?  Surely
> > there's a better way than this.
> 
> This discussion thread just got dropped off the radar, sorry about it.
> None of the spinlock_t elements are required unless split ptlocks are
> in use. I understand your concern regarding yet another #ifdef in the
> struct page definition. But this change is simple and minimal. Do you
> have any other particular alternative in mind which I could explore ?

Do nothing?  I don't understand what problem you're trying to solve.

Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS

[Anshuman Khandual]

On 7/1/21 6:27 PM, Matthew Wilcox wrote:
> On Thu, Jul 01, 2021 at 10:51:27AM +0530, Anshuman Khandual wrote:
>>
>>
>> On 5/20/21 4:47 PM, Matthew Wilcox wrote:
>>> On Wed, May 19, 2021 at 01:03:06PM +0530, Anshuman Khandual wrote:
>>>> Split ptlocks need not be defined and allocated unless they are being used.
>>>> ALLOC_SPLIT_PTLOCKS is inherently dependent on USE_SPLIT_PTE_PTLOCKS. This
>>>> just makes it explicit and clear. While here drop the spinlock_t element
>>>> from the struct page when USE_SPLIT_PTE_PTLOCKS is not enabled.
>>>
>>> I didn't spot this email yesterday.  I'm not a fan.  Isn't struct page
>>> already complicated enough without adding another ifdef to it?  Surely
>>> there's a better way than this.
>>
>> This discussion thread just got dropped off the radar, sorry about it.
>> None of the spinlock_t elements are required unless split ptlocks are
>> in use. I understand your concern regarding yet another #ifdef in the
>> struct page definition. But this change is simple and minimal. Do you
>> have any other particular alternative in mind which I could explore ?
> 
> Do nothing?  I don't understand what problem you're trying to solve.

Currently there is an element (spinlock_t ptl) in the struct page for page
table lock. Although a struct page based spinlock is not even required in
case USE_SPLIT_PTE_PTLOCKS evaluates to be false. Is not that something to
be fixed here i.e drop the splinlock_t element if not required ?

The problem is USE_SPLIT_PTE_PTLOCKS and ALLOC_SPLIT_PTLOCKS get evaluated
independently, although they are inherently dependent. ALLOC_SPLIT_PTLOCKS
could just be set to 0, when USE_SPLIT_PTE_PTLOCKS evaluates to be 0. This
patch makes that dependency explicit and also fixes the above situation.

Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS

[Matthew Wilcox]

On Mon, Jul 05, 2021 at 08:57:54AM +0530, Anshuman Khandual wrote:
> 
> On 7/1/21 6:27 PM, Matthew Wilcox wrote:
> > On Thu, Jul 01, 2021 at 10:51:27AM +0530, Anshuman Khandual wrote:
> >>
> >>
> >> On 5/20/21 4:47 PM, Matthew Wilcox wrote:
> >>> On Wed, May 19, 2021 at 01:03:06PM +0530, Anshuman Khandual wrote:
> >>>> Split ptlocks need not be defined and allocated unless they are being used.
> >>>> ALLOC_SPLIT_PTLOCKS is inherently dependent on USE_SPLIT_PTE_PTLOCKS. This
> >>>> just makes it explicit and clear. While here drop the spinlock_t element
> >>>> from the struct page when USE_SPLIT_PTE_PTLOCKS is not enabled.
> >>>
> >>> I didn't spot this email yesterday.  I'm not a fan.  Isn't struct page
> >>> already complicated enough without adding another ifdef to it?  Surely
> >>> there's a better way than this.
> >>
> >> This discussion thread just got dropped off the radar, sorry about it.
> >> None of the spinlock_t elements are required unless split ptlocks are
> >> in use. I understand your concern regarding yet another #ifdef in the
> >> struct page definition. But this change is simple and minimal. Do you
> >> have any other particular alternative in mind which I could explore ?
> > 
> > Do nothing?  I don't understand what problem you're trying to solve.
> 
> Currently there is an element (spinlock_t ptl) in the struct page for page
> table lock. Although a struct page based spinlock is not even required in
> case USE_SPLIT_PTE_PTLOCKS evaluates to be false. Is not that something to
> be fixed here i.e drop the splinlock_t element if not required ?

No?  It doesn't actually cause any problems, does it?

Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS

[Anshuman Khandual]

On 7/5/21 8:58 AM, Matthew Wilcox wrote:
> On Mon, Jul 05, 2021 at 08:57:54AM +0530, Anshuman Khandual wrote:
>>
>> On 7/1/21 6:27 PM, Matthew Wilcox wrote:
>>> On Thu, Jul 01, 2021 at 10:51:27AM +0530, Anshuman Khandual wrote:
>>>>
>>>>
>>>> On 5/20/21 4:47 PM, Matthew Wilcox wrote:
>>>>> On Wed, May 19, 2021 at 01:03:06PM +0530, Anshuman Khandual wrote:
>>>>>> Split ptlocks need not be defined and allocated unless they are being used.
>>>>>> ALLOC_SPLIT_PTLOCKS is inherently dependent on USE_SPLIT_PTE_PTLOCKS. This
>>>>>> just makes it explicit and clear. While here drop the spinlock_t element
>>>>>> from the struct page when USE_SPLIT_PTE_PTLOCKS is not enabled.
>>>>>
>>>>> I didn't spot this email yesterday.  I'm not a fan.  Isn't struct page
>>>>> already complicated enough without adding another ifdef to it?  Surely
>>>>> there's a better way than this.
>>>>
>>>> This discussion thread just got dropped off the radar, sorry about it.
>>>> None of the spinlock_t elements are required unless split ptlocks are
>>>> in use. I understand your concern regarding yet another #ifdef in the
>>>> struct page definition. But this change is simple and minimal. Do you
>>>> have any other particular alternative in mind which I could explore ?
>>>
>>> Do nothing?  I don't understand what problem you're trying to solve.
>>
>> Currently there is an element (spinlock_t ptl) in the struct page for page
>> table lock. Although a struct page based spinlock is not even required in
>> case USE_SPLIT_PTE_PTLOCKS evaluates to be false. Is not that something to
>> be fixed here i.e drop the splinlock_t element if not required ?
> 
> No?  It doesn't actually cause any problems, does it?
> 

No but should an unnecessary element in a struct is dropped only if there
is a reported problem ?

Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS

[Matthew Wilcox]

On Mon, Jul 05, 2021 at 09:09:22AM +0530, Anshuman Khandual wrote:
> 
> 
> On 7/5/21 8:58 AM, Matthew Wilcox wrote:
> > On Mon, Jul 05, 2021 at 08:57:54AM +0530, Anshuman Khandual wrote:
> >>
> >> On 7/1/21 6:27 PM, Matthew Wilcox wrote:
> >>> On Thu, Jul 01, 2021 at 10:51:27AM +0530, Anshuman Khandual wrote:
> >>>>
> >>>>
> >>>> On 5/20/21 4:47 PM, Matthew Wilcox wrote:
> >>>>> On Wed, May 19, 2021 at 01:03:06PM +0530, Anshuman Khandual wrote:
> >>>>>> Split ptlocks need not be defined and allocated unless they are being used.
> >>>>>> ALLOC_SPLIT_PTLOCKS is inherently dependent on USE_SPLIT_PTE_PTLOCKS. This
> >>>>>> just makes it explicit and clear. While here drop the spinlock_t element
> >>>>>> from the struct page when USE_SPLIT_PTE_PTLOCKS is not enabled.
> >>>>>
> >>>>> I didn't spot this email yesterday.  I'm not a fan.  Isn't struct page
> >>>>> already complicated enough without adding another ifdef to it?  Surely
> >>>>> there's a better way than this.
> >>>>
> >>>> This discussion thread just got dropped off the radar, sorry about it.
> >>>> None of the spinlock_t elements are required unless split ptlocks are
> >>>> in use. I understand your concern regarding yet another #ifdef in the
> >>>> struct page definition. But this change is simple and minimal. Do you
> >>>> have any other particular alternative in mind which I could explore ?
> >>>
> >>> Do nothing?  I don't understand what problem you're trying to solve.
> >>
> >> Currently there is an element (spinlock_t ptl) in the struct page for page
> >> table lock. Although a struct page based spinlock is not even required in
> >> case USE_SPLIT_PTE_PTLOCKS evaluates to be false. Is not that something to
> >> be fixed here i.e drop the splinlock_t element if not required ?
> > 
> > No?  It doesn't actually cause any problems, does it?
> 
> No but should an unnecessary element in a struct is dropped only if there
> is a reported problem ?

In this case, yes.  It's not just a struct member; it's a member of a
union in the struct.  You don't save any memory by getting rid of it.
There's no benefit to getting rid of it.