Messages in this thread |  | | Date | Sun, 3 Jul 2022 22:49:56 +0800 | Subject | Re: [RFC PATCH v3 2/3] mm: Add PUD level pagetable account | From | Baolin Wang <> |
| |
On 7/3/2022 10:28 PM, Mike Rapoport wrote: > On Sun, Jul 03, 2022 at 10:06:32PM +0800, Baolin Wang wrote: >> >> >> On 7/3/2022 11:40 AM, Matthew Wilcox wrote: >>> On Fri, Jul 01, 2022 at 04:04:21PM +0800, Baolin Wang wrote: >>>>> Using pgtable_pud_page_ctor() and pgtable_pud_page_dtor() would be >>>>> consistent with what we currently have for PTEs and PMDs. >>>>> >>>>> This applies to all the additions of pgtable_page_dec() and >>>>> pgtable_page_inc(). >>>> >>>> OK. I can add pgtable_pud_page_ctor() and pgtable_pud_page_dtor() helpers to >>>> keep consistent, which are just wrappers of pgtable_page_inc() and >>>> pgtable_page_dec(). >>> >>> I think you misunderstand Mike. >>> >>> Don't add pgtable_page_inc() and pgtable_page_dec(). Just add >>> pgtable_pud_page_ctor() and pgtable_pud_page_dtor(). At least, that >>> was what I said last time you posted these patches. >> >> My concern is that I need another helpers for kernel page table allocation >> helpers, if only adding pgtable_pud_page_ctor() and pgtable_pud_page_dtor() >> like below: >> >> static inline void pgtable_pud_page_ctor(struct page *page) >> { >> __SetPageTable(page); >> inc_lruvec_page_state(page, NR_PAGETABLE); >> } >> >> static inline void pgtable_pud_page_dtor(struct page *page) >> { >> __ClearPageTable(page); >> dec_lruvec_page_state(page, NR_PAGETABLE); >> } >> >> So for kernel pte page table allocation, I need another similar helpers like >> below. However they do the samething with >> pgtable_pud_page_ctor/pgtable_pud_page_dtor, so I am not sure this is good >> for adding these duplicate code. >> >> static inline void pgtable_kernel_pte_page_ctor(struct page *page) >> { >> __SetPageTable(page); >> inc_lruvec_page_state(page, NR_PAGETABLE); >> } >> >> static inline void pgtable_kernel_pte_page_dtor(struct page *page) >> { >> __ClearPageTable(page); >> dec_lruvec_page_state(page, NR_PAGETABLE); >> } >> >> Instead adding a common helpers seems more readable to me, which can also >> simplify original pgtable_pmd_page_dtor()/pgtable_pmd_page_ctor(). Something >> like below. >> >> static inline void pgtable_page_inc(struct page *page) >> { >> __SetPageTable(page); >> inc_lruvec_page_state(page, NR_PAGETABLE); >> } >> >> static inline void pgtable_page_dec(struct page *page) >> { >> __ClearPageTable(page); >> dec_lruvec_page_state(page, NR_PAGETABLE); >> } >> >> static inline void pgtable_pud_page_ctor(struct page *page) >> { >> pgtable_page_inc(page); >> } >> >> static inline void pgtable_pud_page_dtor(struct page *page) >> { >> pgtable_page_dec(page); >> } >> >> For kernel pte page table, we can just use >> pgtable_page_inc/pgtable_page_dec(), or adding >> pgtable_kernel_pte_page_ctor/pgtable_kernel_pte_page_dtor, which just >> wrappers of pgtable_page_inc() and pgtable_page_dec(). >> >> Matthew and Mike, how do you think? Thanks. > > I actually meant to add pgtable_pud_page_ctor/dtor() as a wrapper for the > new helper to keep pud tables allocation consistent with pmd and pte and > as a provision for the time we'll have per-page pud locks. > > For the accounting of the kernel page tables a new helper does make sense > because there are no locks to initialize for the kernel page tables.
Thanks for clarification. That is also my thought.
> > I can't say that I'm happy with the pgtable_page_inc/dec names, though. > > Maybe page_{set,clear}_pgtable()?
Sounds better than pgtable_page_inc/dec() for me. I will use them in next version if no other objections. Thanks.
|  |