Message ID | 1621409586-5555-1-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [V2] mm/thp: Make ALLOC_SPLIT_PTLOCKS dependent on USE_SPLIT_PTE_PTLOCKS | expand |
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 */
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 */
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.
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.
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?
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 ?
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.
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;