| Message ID | 20241219164425.2277022-2-kevin.brodsky@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Account page tables at all levels | expand |
On Thu, Dec 19, 2024 at 04:44:16PM +0000, Kevin Brodsky wrote: > Besides the ptlock management at PTE/PMD level, all the > pagetable_*_[cd]tor have the same implementation. Introduce common > helpers for all levels to reduce the duplication. Uff, I forgot to Cc you on the discussion here, sorry!: https://lkml.kernel.org/r/cover.1734526570.git.zhengqi.arch@bytedance.com we now have two series doing more or less overlapping things :/ You can in fact trivially merge the all the implementations -- the apparent non-common bit (ptlock_free) is a no-op for all those other levels because they'll be having ptdesc->lock == NULL.
Hi Peter, Qi, On 19/12/2024 18:19, Peter Zijlstra wrote: > On Thu, Dec 19, 2024 at 04:44:16PM +0000, Kevin Brodsky wrote: >> Besides the ptlock management at PTE/PMD level, all the >> pagetable_*_[cd]tor have the same implementation. Introduce common >> helpers for all levels to reduce the duplication. > Uff, I forgot to Cc you on the discussion here, sorry!: > > https://lkml.kernel.org/r/cover.1734526570.git.zhengqi.arch@bytedance.com > > we now have two series doing more or less overlapping things :/ > > You can in fact trivially merge the all the implementations -- the > apparent non-common bit (ptlock_free) is a no-op for all those other > levels because they'll be having ptdesc->lock == NULL. Ah that is good to know, thanks for letting me know about that and Qi's series! Fortunately there isn't that much overlap between our series - I think we can easily sort this out. Qi, shall we collaborate to make our series complementary? I believe my series covers patch 2 and 4 of your series, but it goes further by covering all levels and all architectures, and patches introducing ctor/dtor are already split as Alexander suggested on your series. So my suggestion would be: * Remove patch 1 in my series - I'd just introduce pagetable_{p4d,pgd}_[cd]tor with the same implementation as pagetable_pud_[cd]tor. * Remove patch 2 and 4 from your series and rebase it on mine. Let me know if that makes sense, if so I'll post a v2. Cheers, - Kevin
Hi Kevin, On 2024/12/20 18:49, Kevin Brodsky wrote: > Hi Peter, Qi, > > On 19/12/2024 18:19, Peter Zijlstra wrote: >> On Thu, Dec 19, 2024 at 04:44:16PM +0000, Kevin Brodsky wrote: >>> Besides the ptlock management at PTE/PMD level, all the >>> pagetable_*_[cd]tor have the same implementation. Introduce common >>> helpers for all levels to reduce the duplication. >> Uff, I forgot to Cc you on the discussion here, sorry!: >> >> https://lkml.kernel.org/r/cover.1734526570.git.zhengqi.arch@bytedance.com >> >> we now have two series doing more or less overlapping things :/ >> >> You can in fact trivially merge the all the implementations -- the >> apparent non-common bit (ptlock_free) is a no-op for all those other >> levels because they'll be having ptdesc->lock == NULL. > > Ah that is good to know, thanks for letting me know about that and Qi's > series! Fortunately there isn't that much overlap between our series - I > think we can easily sort this out. > > Qi, shall we collaborate to make our series complementary? I believe my > series covers patch 2 and 4 of your series, but it goes further by > covering all levels and all architectures, and patches introducing > ctor/dtor are already split as Alexander suggested on your series. So my > suggestion would be: > > * Remove patch 1 in my series - I'd just introduce > pagetable_{p4d,pgd}_[cd]tor with the same implementation as > pagetable_pud_[cd]tor. > * Remove patch 2 and 4 from your series and rebase it on mine. I quickly went through your patch series. It looks like my patch 2 and your patch 6 are duplicated, so you want me to remove my patch 2. But I think you may not be able to simple let arm64, riscv and x86 to use generic p4d_{alloc_one,free}(). Because even if CONFIG_PGTABLE_LEVELS > 4, the pgtable_l5_enabled() may not be true. For example, in arm64: #if CONFIG_PGTABLE_LEVELS > 4 static __always_inline bool pgtable_l5_enabled(void) { if (!alternative_has_cap_likely(ARM64_ALWAYS_BOOT)) return vabits_actual == VA_BITS; return alternative_has_cap_unlikely(ARM64_HAS_VA52); } Did I miss something? My patch series is not only for cleanup, but also for fixes of UAF issue [1], so is it possible to rebase your patch series onto mine? I can post v3 ASAP. [1]. https://lore.kernel.org/all/67548279.050a0220.a30f1.015b.GAE@google.com/ Thanks! > > Let me know if that makes sense, if so I'll post a v2. > > Cheers, > - Kevin
On 20/12/2024 12:46, Qi Zheng wrote: > Hi Kevin, > > On 2024/12/20 18:49, Kevin Brodsky wrote: >> [...] >> >> Qi, shall we collaborate to make our series complementary? I believe my >> series covers patch 2 and 4 of your series, but it goes further by >> covering all levels and all architectures, and patches introducing >> ctor/dtor are already split as Alexander suggested on your series. So my >> suggestion would be: >> >> * Remove patch 1 in my series - I'd just introduce >> pagetable_{p4d,pgd}_[cd]tor with the same implementation as >> pagetable_pud_[cd]tor. >> * Remove patch 2 and 4 from your series and rebase it on mine. > > I quickly went through your patch series. It looks like my patch 2 and > your patch 6 are duplicated, so you want me to remove my patch 2. > > But I think you may not be able to simple let arm64, riscv and x86 to > use generic p4d_{alloc_one,free}(). Because even if > CONFIG_PGTABLE_LEVELS > 4, the pgtable_l5_enabled() may not be true. > > For example, in arm64: > > #if CONFIG_PGTABLE_LEVELS > 4 > > static __always_inline bool pgtable_l5_enabled(void) > { > if (!alternative_has_cap_likely(ARM64_ALWAYS_BOOT)) > return vabits_actual == VA_BITS; > return alternative_has_cap_unlikely(ARM64_HAS_VA52); > } Correct. That's why the implementation of p4d_free() I introduce in patch 6 checks mm_p4d_folded(), which is implemented as !pgtable_l5_enabled() on those architectures (see last paragraph in commit message). In fact it turns out Alexander suggested exactly this approach [2]. > > Did I miss something? > > My patch series is not only for cleanup, but also for fixes of > UAF issue [1], so is it possible to rebase your patch series onto > mine? I can post v3 ASAP. I see, yours should be merged first then. The issue is that yours would depend on some of the patches in mine, not the other way round. My suggestion would then be for you to take patch 5, 6 and 7 from my series, as they match Alexander's suggestions (and patch 5 is I think a useful simplification), and replace patch 2 in your series with those. I would then rebase my series on top and adapt it accordingly. Does that sound reasonable? - Kevin [2] https://lore.kernel.org/all/Z2RKpdv7pL34MIEt@tuxmaker.boeblingen.de.ibm.com/
On 2024/12/20 21:50, Kevin Brodsky wrote: > On 20/12/2024 12:46, Qi Zheng wrote: >> Hi Kevin, >> >> On 2024/12/20 18:49, Kevin Brodsky wrote: >>> [...] >>> >>> Qi, shall we collaborate to make our series complementary? I believe my >>> series covers patch 2 and 4 of your series, but it goes further by >>> covering all levels and all architectures, and patches introducing >>> ctor/dtor are already split as Alexander suggested on your series. So my >>> suggestion would be: >>> >>> * Remove patch 1 in my series - I'd just introduce >>> pagetable_{p4d,pgd}_[cd]tor with the same implementation as >>> pagetable_pud_[cd]tor. >>> * Remove patch 2 and 4 from your series and rebase it on mine. >> >> I quickly went through your patch series. It looks like my patch 2 and >> your patch 6 are duplicated, so you want me to remove my patch 2. >> >> But I think you may not be able to simple let arm64, riscv and x86 to >> use generic p4d_{alloc_one,free}(). Because even if >> CONFIG_PGTABLE_LEVELS > 4, the pgtable_l5_enabled() may not be true. >> >> For example, in arm64: >> >> #if CONFIG_PGTABLE_LEVELS > 4 >> >> static __always_inline bool pgtable_l5_enabled(void) >> { >> if (!alternative_has_cap_likely(ARM64_ALWAYS_BOOT)) >> return vabits_actual == VA_BITS; >> return alternative_has_cap_unlikely(ARM64_HAS_VA52); >> } > > Correct. That's why the implementation of p4d_free() I introduce in > patch 6 checks mm_p4d_folded(), which is implemented as > !pgtable_l5_enabled() on those architectures (see last paragraph in > commit message). In fact it turns out Alexander suggested exactly this > approach [2]. OK, I see. > >> >> Did I miss something? >> >> My patch series is not only for cleanup, but also for fixes of >> UAF issue [1], so is it possible to rebase your patch series onto >> mine? I can post v3 ASAP. > > I see, yours should be merged first then. The issue is that yours would > depend on some of the patches in mine, not the other way round. > > My suggestion would then be for you to take patch 5, 6 and 7 from my > series, as they match Alexander's suggestions (and patch 5 is I think a > useful simplification), and replace patch 2 in your series with those. I > would then rebase my series on top and adapt it accordingly. Does that > sound reasonable? Sounds good. But maybe just patch 5 and 6. Because I actually did the work of your patch 7 in my patch 2 and 4. So, is it okay to do something like the following? 1. I separate the ctor()/dtor() part from my patch 2, and then replace the rest with your patch 6. 2. take your patch 5 form your series If it's ok, I will post the v3 next Monday. ;) Thanks! > > - Kevin > > [2] > https://lore.kernel.org/all/Z2RKpdv7pL34MIEt@tuxmaker.boeblingen.de.ibm.com/ >
On 20/12/2024 15:16, Qi Zheng wrote: >>> >>> Did I miss something? >>> >>> My patch series is not only for cleanup, but also for fixes of >>> UAF issue [1], so is it possible to rebase your patch series onto >>> mine? I can post v3 ASAP. >> >> I see, yours should be merged first then. The issue is that yours would >> depend on some of the patches in mine, not the other way round. >> >> My suggestion would then be for you to take patch 5, 6 and 7 from my >> series, as they match Alexander's suggestions (and patch 5 is I think a >> useful simplification), and replace patch 2 in your series with those. I >> would then rebase my series on top and adapt it accordingly. Does that >> sound reasonable? > > Sounds good. But maybe just patch 5 and 6. Because I actually did > the work of your patch 7 in my patch 2 and 4. Yes that's fair! You'd have to do adapt my patch 7 to make it fit in your series so I agree it makes more sense this way. > > So, is it okay to do something like the following? > > 1. I separate the ctor()/dtor() part from my patch 2, and then replace > the rest with your patch 6. > 2. take your patch 5 form your series Sounds good to me! IIUC Dave Hansen gave his Acked-by for the x86 part of patch 6 [1], would make sense to add it when you post your v3. > > If it's ok, I will post the v3 next Monday. ;) Perfect. I'm going offline tonight, when I come back in the new year I'll review your v3 series and post a new version of this one. Cheers, - Kevin [1] https://lore.kernel.org/linux-mm/a7398426-56d1-40b4-a1c9-40ae8c8a4b4b@intel.com/
On 2024/12/20 22:28, Kevin Brodsky wrote: > On 20/12/2024 15:16, Qi Zheng wrote: >>>> >>>> Did I miss something? >>>> >>>> My patch series is not only for cleanup, but also for fixes of >>>> UAF issue [1], so is it possible to rebase your patch series onto >>>> mine? I can post v3 ASAP. >>> >>> I see, yours should be merged first then. The issue is that yours would >>> depend on some of the patches in mine, not the other way round. >>> >>> My suggestion would then be for you to take patch 5, 6 and 7 from my >>> series, as they match Alexander's suggestions (and patch 5 is I think a >>> useful simplification), and replace patch 2 in your series with those. I >>> would then rebase my series on top and adapt it accordingly. Does that >>> sound reasonable? >> >> Sounds good. But maybe just patch 5 and 6. Because I actually did >> the work of your patch 7 in my patch 2 and 4. > > Yes that's fair! You'd have to do adapt my patch 7 to make it fit in > your series so I agree it makes more sense this way. Thanks! > >> >> So, is it okay to do something like the following? >> >> 1. I separate the ctor()/dtor() part from my patch 2, and then replace >> the rest with your patch 6. >> 2. take your patch 5 form your series > > Sounds good to me! > > IIUC Dave Hansen gave his Acked-by for the x86 part of patch 6 [1], > would make sense to add it when you post your v3. OK, will add it! > >> >> If it's ok, I will post the v3 next Monday. ;) > > Perfect. I'm going offline tonight, when I come back in the new year > I'll review your v3 series and post a new version of this one. Thank you very much! And Happy New Year! > > Cheers, > - Kevin > > [1] > https://lore.kernel.org/linux-mm/a7398426-56d1-40b4-a1c9-40ae8c8a4b4b@intel.com/
diff --git a/include/linux/mm.h b/include/linux/mm.h index c39c4945946c..a5b482362792 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2915,6 +2915,22 @@ static inline void pagetable_free(struct ptdesc *pt) __free_pages(page, compound_order(page)); } +static inline void __pagetable_ctor(struct ptdesc *ptdesc) +{ + struct folio *folio = ptdesc_folio(ptdesc); + + __folio_set_pgtable(folio); + lruvec_stat_add_folio(folio, NR_PAGETABLE); +} + +static inline void __pagetable_dtor(struct ptdesc *ptdesc) +{ + struct folio *folio = ptdesc_folio(ptdesc); + + __folio_clear_pgtable(folio); + lruvec_stat_sub_folio(folio, NR_PAGETABLE); +} + #if defined(CONFIG_SPLIT_PTE_PTLOCKS) #if ALLOC_SPLIT_PTLOCKS void __init ptlock_cache_init(void); @@ -2992,22 +3008,16 @@ static inline void ptlock_free(struct ptdesc *ptdesc) {} static inline bool pagetable_pte_ctor(struct ptdesc *ptdesc) { - struct folio *folio = ptdesc_folio(ptdesc); - if (!ptlock_init(ptdesc)) return false; - __folio_set_pgtable(folio); - lruvec_stat_add_folio(folio, NR_PAGETABLE); + __pagetable_ctor(ptdesc); return true; } static inline void pagetable_pte_dtor(struct ptdesc *ptdesc) { - struct folio *folio = ptdesc_folio(ptdesc); - ptlock_free(ptdesc); - __folio_clear_pgtable(folio); - lruvec_stat_sub_folio(folio, NR_PAGETABLE); + __pagetable_dtor(ptdesc); } pte_t *__pte_offset_map(pmd_t *pmd, unsigned long addr, pmd_t *pmdvalp); @@ -3110,22 +3120,16 @@ static inline spinlock_t *pmd_lock(struct mm_struct *mm, pmd_t *pmd) static inline bool pagetable_pmd_ctor(struct ptdesc *ptdesc) { - struct folio *folio = ptdesc_folio(ptdesc); - if (!pmd_ptlock_init(ptdesc)) return false; - __folio_set_pgtable(folio); - lruvec_stat_add_folio(folio, NR_PAGETABLE); + __pagetable_ctor(ptdesc); return true; } static inline void pagetable_pmd_dtor(struct ptdesc *ptdesc) { - struct folio *folio = ptdesc_folio(ptdesc); - pmd_ptlock_free(ptdesc); - __folio_clear_pgtable(folio); - lruvec_stat_sub_folio(folio, NR_PAGETABLE); + __pagetable_dtor(ptdesc); } /* @@ -3149,18 +3153,12 @@ static inline spinlock_t *pud_lock(struct mm_struct *mm, pud_t *pud) static inline void pagetable_pud_ctor(struct ptdesc *ptdesc) { - struct folio *folio = ptdesc_folio(ptdesc); - - __folio_set_pgtable(folio); - lruvec_stat_add_folio(folio, NR_PAGETABLE); + __pagetable_ctor(ptdesc); } static inline void pagetable_pud_dtor(struct ptdesc *ptdesc) { - struct folio *folio = ptdesc_folio(ptdesc); - - __folio_clear_pgtable(folio); - lruvec_stat_sub_folio(folio, NR_PAGETABLE); + __pagetable_dtor(ptdesc); } extern void __init pagecache_init(void);
Besides the ptlock management at PTE/PMD level, all the pagetable_*_[cd]tor have the same implementation. Introduce common helpers for all levels to reduce the duplication. Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com> --- include/linux/mm.h | 46 ++++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 24 deletions(-)