* [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS
@ 2021-05-19 7:33 Anshuman Khandual
2021-05-20 11:17 ` Matthew Wilcox
0 siblings, 1 reply; 8+ messages in thread
From: Anshuman Khandual @ 2021-05-19 7:33 UTC (permalink / raw)
To: linux-mm, akpm
Cc: Anshuman Khandual, Matthew Wilcox, Vlastimil Babka, Randy Dunlap,
linux-kernel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS
2021-05-19 7:33 [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS Anshuman Khandual
@ 2021-05-20 11:17 ` Matthew Wilcox
2021-07-01 5:21 ` Anshuman Khandual
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Wilcox @ 2021-05-20 11:17 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, akpm, Vlastimil Babka, Randy Dunlap, linux-kernel
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 */
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS
2021-05-20 11:17 ` Matthew Wilcox
@ 2021-07-01 5:21 ` Anshuman Khandual
2021-07-01 12:57 ` Matthew Wilcox
0 siblings, 1 reply; 8+ messages in thread
From: Anshuman Khandual @ 2021-07-01 5:21 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-mm, akpm, Vlastimil Babka, Randy Dunlap, linux-kernel
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 */
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS
2021-07-01 5:21 ` Anshuman Khandual
@ 2021-07-01 12:57 ` Matthew Wilcox
2021-07-05 3:27 ` Anshuman Khandual
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Wilcox @ 2021-07-01 12:57 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, akpm, Vlastimil Babka, Randy Dunlap, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS
2021-07-01 12:57 ` Matthew Wilcox
@ 2021-07-05 3:27 ` Anshuman Khandual
2021-07-05 3:28 ` Matthew Wilcox
0 siblings, 1 reply; 8+ messages in thread
From: Anshuman Khandual @ 2021-07-05 3:27 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-mm, akpm, Vlastimil Babka, Randy Dunlap, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS
2021-07-05 3:27 ` Anshuman Khandual
@ 2021-07-05 3:28 ` Matthew Wilcox
2021-07-05 3:39 ` Anshuman Khandual
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Wilcox @ 2021-07-05 3:28 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, akpm, Vlastimil Babka, Randy Dunlap, linux-kernel
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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS
2021-07-05 3:28 ` Matthew Wilcox
@ 2021-07-05 3:39 ` Anshuman Khandual
2021-07-05 11:30 ` Matthew Wilcox
0 siblings, 1 reply; 8+ messages in thread
From: Anshuman Khandual @ 2021-07-05 3:39 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-mm, akpm, Vlastimil Babka, Randy Dunlap, linux-kernel
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 ?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS
2021-07-05 3:39 ` Anshuman Khandual
@ 2021-07-05 11:30 ` Matthew Wilcox
0 siblings, 0 replies; 8+ messages in thread
From: Matthew Wilcox @ 2021-07-05 11:30 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, akpm, Vlastimil Babka, Randy Dunlap, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-05 11:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-19 7:33 [PATCH V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS Anshuman Khandual
2021-05-20 11:17 ` Matthew Wilcox
2021-07-01 5:21 ` Anshuman Khandual
2021-07-01 12:57 ` Matthew Wilcox
2021-07-05 3:27 ` Anshuman Khandual
2021-07-05 3:28 ` Matthew Wilcox
2021-07-05 3:39 ` Anshuman Khandual
2021-07-05 11:30 ` Matthew Wilcox
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.