| Message ID | 20230627031431.29653-1-vishal.moola@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Split ptdesc from struct page | expand |
On Mon, 26 Jun 2023, Vishal Moola (Oracle) wrote: > The MM subsystem is trying to shrink struct page. This patchset > introduces a memory descriptor for page table tracking - struct ptdesc. ... > 39 files changed, 686 insertions(+), 455 deletions(-) I don't see the point of this patchset: to me it is just obfuscation of the present-day tight relationship between page table and struct page. Matthew already explained: > The intent is to get ptdescs to be dynamically allocated at some point > in the ~2-3 years out future when we have finished the folio project ... So in a kindly mood, I'd say that this patchset is ahead of its time. But I can certainly adapt to it, if everyone else sees some point to it. Hugh
On 27.06.23 06:44, Hugh Dickins wrote: > On Mon, 26 Jun 2023, Vishal Moola (Oracle) wrote: > >> The MM subsystem is trying to shrink struct page. This patchset >> introduces a memory descriptor for page table tracking - struct ptdesc. > ... >> 39 files changed, 686 insertions(+), 455 deletions(-) > > I don't see the point of this patchset: to me it is just obfuscation of > the present-day tight relationship between page table and struct page. > > Matthew already explained: > >> The intent is to get ptdescs to be dynamically allocated at some point >> in the ~2-3 years out future when we have finished the folio project ... > > So in a kindly mood, I'd say that this patchset is ahead of its time. > But I can certainly adapt to it, if everyone else sees some point to it. I share your thoughts, that code churn which will help eventually in the far, far future (not wanting to sound too pessimistic, but it's not going to be there tomorrow ;) ). However, if it's just the same as the other conversions we already did (e.g., struct slab), then I guess there is no reason to stop now -- the obfuscation already happened. ... or is there a difference regarding this conversion and the previous ones?
On Mon, Jun 26, 2023 at 09:44:08PM -0700, Hugh Dickins wrote: > On Mon, 26 Jun 2023, Vishal Moola (Oracle) wrote: > > > The MM subsystem is trying to shrink struct page. This patchset > > introduces a memory descriptor for page table tracking - struct ptdesc. > ... > > 39 files changed, 686 insertions(+), 455 deletions(-) > > I don't see the point of this patchset: to me it is just obfuscation of > the present-day tight relationship between page table and struct page. > > Matthew already explained: > > > The intent is to get ptdescs to be dynamically allocated at some point > > in the ~2-3 years out future when we have finished the folio project ... > > So in a kindly mood, I'd say that this patchset is ahead of its time. > But I can certainly adapt to it, if everyone else sees some point to it. If you think this patchset is ahead of its time, we can certainly put it on hold. We're certainly prepared to redo it to be merged after your current patch series. I think you can see the advantage of the destination, so I don't think you're against that. Are you opposed to the sequencing of the work to get us there? I'd be happy to discuss another way to do it. For example, we could dynamically allocate ptdescs right now. We'd get the benefit of having an arbitrary amount of space in the ptdesc, although not the benefit of a smaller memmap until everything else is also dynamically allocated.
On Tue, 27 Jun 2023, David Hildenbrand wrote: > On 27.06.23 06:44, Hugh Dickins wrote: > > On Mon, 26 Jun 2023, Vishal Moola (Oracle) wrote: > > > >> The MM subsystem is trying to shrink struct page. This patchset > >> introduces a memory descriptor for page table tracking - struct ptdesc. > > ... > >> 39 files changed, 686 insertions(+), 455 deletions(-) > > > > I don't see the point of this patchset: to me it is just obfuscation of > > the present-day tight relationship between page table and struct page. > > > > Matthew already explained: > > > >> The intent is to get ptdescs to be dynamically allocated at some point > >> in the ~2-3 years out future when we have finished the folio project ... > > > > So in a kindly mood, I'd say that this patchset is ahead of its time. > > But I can certainly adapt to it, if everyone else sees some point to it. > > I share your thoughts, that code churn which will help eventually in the far, > far future (not wanting to sound too pessimistic, but it's not going to be > there tomorrow ;) ). > > However, if it's just the same as the other conversions we already did (e.g., > struct slab), then I guess there is no reason to stop now -- the obfuscation > already happened. > > ... or is there a difference regarding this conversion and the previous ones? I was aware of the struct slab thing, didn't see much point there myself either; but it was welcomed by Vlastimil, and barely affected outside of slab allocators, so I had no reason to object. You think that if a little unnecessary churn (a *lot* of churn if you include folios, which did save some repeated calls to compound_head()) has already occurred, that's a good precedent for allowing more and more? My opinion happens to differ on that. Hugh
On Tue, 27 Jun 2023, Matthew Wilcox wrote: > On Mon, Jun 26, 2023 at 09:44:08PM -0700, Hugh Dickins wrote: > > On Mon, 26 Jun 2023, Vishal Moola (Oracle) wrote: > > > > > The MM subsystem is trying to shrink struct page. This patchset > > > introduces a memory descriptor for page table tracking - struct ptdesc. > > ... > > > 39 files changed, 686 insertions(+), 455 deletions(-) > > > > I don't see the point of this patchset: to me it is just obfuscation of > > the present-day tight relationship between page table and struct page. > > > > Matthew already explained: > > > > > The intent is to get ptdescs to be dynamically allocated at some point > > > in the ~2-3 years out future when we have finished the folio project ... > > > > So in a kindly mood, I'd say that this patchset is ahead of its time. > > But I can certainly adapt to it, if everyone else sees some point to it. > > If you think this patchset is ahead of its time, we can certainly put > it on hold. We're certainly prepared to redo it to be merged after your > current patch series. Thank you, but I can adapt. That was not my point: I'm claiming this patchset is ~2-3 years ahead of its time. > > I think you can see the advantage of the destination, so I don't think > you're against that. Maybe - I have some scepticism, but I'll be happy for that to be dissolved. > Are you opposed to the sequencing of the work to > get us there? I'd be happy to discuss another way to do it. Yes, I'm opposed to churn for no benefit. > > For example, we could dynamically allocate ptdescs right now. We'd get > the benefit of having an arbitrary amount of space in the ptdesc, > although not the benefit of a smaller memmap until everything else is > also dynamically allocated. That sounded much better, at first: churn serving good purpose. But now I suspect you're offering to dynamically allocate a ptdesc, in addition to the struct page of the page table(s) itself, which will be wasted: more memory consumption to no advantage. If that's so, no thanks. Hugh
On 27.06.23 22:13, Hugh Dickins wrote: > On Tue, 27 Jun 2023, David Hildenbrand wrote: >> On 27.06.23 06:44, Hugh Dickins wrote: >>> On Mon, 26 Jun 2023, Vishal Moola (Oracle) wrote: >>> >>>> The MM subsystem is trying to shrink struct page. This patchset >>>> introduces a memory descriptor for page table tracking - struct ptdesc. >>> ... >>>> 39 files changed, 686 insertions(+), 455 deletions(-) >>> >>> I don't see the point of this patchset: to me it is just obfuscation of >>> the present-day tight relationship between page table and struct page. >>> >>> Matthew already explained: >>> >>>> The intent is to get ptdescs to be dynamically allocated at some point >>>> in the ~2-3 years out future when we have finished the folio project ... >>> >>> So in a kindly mood, I'd say that this patchset is ahead of its time. >>> But I can certainly adapt to it, if everyone else sees some point to it. >> >> I share your thoughts, that code churn which will help eventually in the far, >> far future (not wanting to sound too pessimistic, but it's not going to be >> there tomorrow ;) ). >> >> However, if it's just the same as the other conversions we already did (e.g., >> struct slab), then I guess there is no reason to stop now -- the obfuscation >> already happened. >> >> ... or is there a difference regarding this conversion and the previous ones? > > I was aware of the struct slab thing, didn't see much point there myself > either; but it was welcomed by Vlastimil, and barely affected outside of > slab allocators, so I had no reason to object. > > You think that if a little unnecessary churn (a *lot* of churn if you > include folios, which did save some repeated calls to compound_head()) > has already occurred, that's a good precedent for allowing more and more? Well, if you phrase it like that ... no, I'm not in favor of unnecessary churn at all. Yes, folios were/are a lot of churn (IMHO not unnecessary, though). I'm not a friend of these "overlays"; it all only really makes sense to me once we actually allocate the descriptors dynamically. Maybe some of the existing/ongoing conversions were different (that's why I was asking for the difference, as you said the "struct slab" thing was well received). If they are primarily only unnecessary churn for now (and unclear when/how it will become useful), I share your opinion.
On Wed, Jun 28, 2023 at 09:41:18AM +0200, David Hildenbrand wrote: > I'm not a friend of these "overlays"; it all only really makes sense to me > once we actually allocate the descriptors dynamically. Maybe some of the > existing/ongoing conversions were different (that's why I was asking for the > difference, as you said the "struct slab" thing was well received). > > If they are primarily only unnecessary churn for now (and unclear when/how > it will become useful), I share your opinion. One of the reasons for doing these conversions "early" is that it helps people who work on this code know what fields they can actually use in their memory descriptor. We have a _lot_ of historical baggage with people just using random bits in struct page for their own purposes without necessarily considering the effects on the rest of the system. By creating specific types for each user of struct page, we can see what's actually going on. Before the ptdesc conversion started, I could not have told you which bits in struct page were used by the s390 code. I knew they were playing some fun games with the refcount (it's even documented in the s390 code!) but I didn't know they were using ... whetever it is; page->private to point to the kvm private data? So maybe it is harder for MM developers right now to see what fields in memdesc A overlap with which fields in memdesc B. That _ought_ not to be a concern! We document which fields are available in each memdesc, and have various assertions to trip when people make things not line up any more. There can still be problems, of course; we haven't set the assertions quite tightly enough in some cases. People are going to keep adding crap to struct page, and they're going to keep misusing the crap that's in struct page. That has to stop.
The MM subsystem is trying to shrink struct page. This patchset introduces a memory descriptor for page table tracking - struct ptdesc. This patchset introduces ptdesc, splits ptdesc from struct page, and converts many callers of page table constructor/destructors to use ptdescs. Ptdesc is a foundation to further standardize page tables, and eventually allow for dynamic allocation of page tables independent of struct page. However, the use of pages for page table tracking is quite deeply ingrained and varied across archictectures, so there is still a lot of work to be done before that can happen. This is rebased on next-20230626. There is a minor conflict with patch 24 and the mm-unstable tree in arch/m68k/mm/motorola.c - The end result of applying the patch should be the same. v6: Fix compiler warnings/errors v5: More Acked-bys :) Cleanup some documentation wording and formatting Add pt_rcu_head to ptdesc Add memcg to ptdesc (and align it with struct page) Ensure all get_free_page() callers prohibit HIGHMEM for 32 bit support. Renamed folio_{set, clear}_table() to folio_{set, clear}_pgtable() Removed pagetable_clear() as it is not necessary right now Dropped s390 _refcount to _pt_frag_refcount conversion Vishal Moola (Oracle) (33): mm: Add PAGE_TYPE_OP folio functions s390: Use _pt_s390_gaddr for gmap address tracking pgtable: Create struct ptdesc mm: add utility functions for ptdesc mm: Convert pmd_pgtable_page() to pmd_ptdesc() mm: Convert ptlock_alloc() to use ptdescs mm: Convert ptlock_ptr() to use ptdescs mm: Convert pmd_ptlock_init() to use ptdescs mm: Convert ptlock_init() to use ptdescs mm: Convert pmd_ptlock_free() to use ptdescs mm: Convert ptlock_free() to use ptdescs mm: Create ptdesc equivalents for pgtable_{pte,pmd}_page_{ctor,dtor} powerpc: Convert various functions to use ptdescs x86: Convert various functions to use ptdescs s390: Convert various gmap functions to use ptdescs s390: Convert various pgalloc functions to use ptdescs mm: Remove page table members from struct page pgalloc: Convert various functions to use ptdescs arm: Convert various functions to use ptdescs arm64: Convert various functions to use ptdescs csky: Convert __pte_free_tlb() to use ptdescs hexagon: Convert __pte_free_tlb() to use ptdescs loongarch: Convert various functions to use ptdescs m68k: Convert various functions to use ptdescs mips: Convert various functions to use ptdescs nios2: Convert __pte_free_tlb() to use ptdescs openrisc: Convert __pte_free_tlb() to use ptdescs riscv: Convert alloc_{pmd, pte}_late() to use ptdescs sh: Convert pte_free_tlb() to use ptdescs sparc64: Convert various functions to use ptdescs sparc: Convert pgtable_pte_page_{ctor, dtor}() to ptdesc equivalents um: Convert {pmd, pte}_free_tlb() to use ptdescs mm: Remove pgtable_{pmd, pte}_page_{ctor, dtor}() wrappers Documentation/mm/split_page_table_lock.rst | 12 +- .../zh_CN/mm/split_page_table_lock.rst | 14 +- arch/arm/include/asm/tlb.h | 12 +- arch/arm/mm/mmu.c | 7 +- arch/arm64/include/asm/tlb.h | 14 +- arch/arm64/mm/mmu.c | 7 +- arch/csky/include/asm/pgalloc.h | 4 +- arch/hexagon/include/asm/pgalloc.h | 8 +- arch/loongarch/include/asm/pgalloc.h | 27 ++- arch/loongarch/mm/pgtable.c | 7 +- arch/m68k/include/asm/mcf_pgalloc.h | 47 ++-- arch/m68k/include/asm/sun3_pgalloc.h | 8 +- arch/m68k/mm/motorola.c | 4 +- arch/mips/include/asm/pgalloc.h | 32 +-- arch/mips/mm/pgtable.c | 8 +- arch/nios2/include/asm/pgalloc.h | 8 +- arch/openrisc/include/asm/pgalloc.h | 8 +- arch/powerpc/mm/book3s64/mmu_context.c | 10 +- arch/powerpc/mm/book3s64/pgtable.c | 32 +-- arch/powerpc/mm/pgtable-frag.c | 46 ++-- arch/riscv/include/asm/pgalloc.h | 8 +- arch/riscv/mm/init.c | 16 +- arch/s390/include/asm/pgalloc.h | 4 +- arch/s390/include/asm/tlb.h | 4 +- arch/s390/mm/gmap.c | 207 ++++++++++-------- arch/s390/mm/pgalloc.c | 108 ++++----- arch/sh/include/asm/pgalloc.h | 9 +- arch/sparc/mm/init_64.c | 17 +- arch/sparc/mm/srmmu.c | 5 +- arch/um/include/asm/pgalloc.h | 18 +- arch/x86/mm/pgtable.c | 47 ++-- arch/x86/xen/mmu_pv.c | 2 +- include/asm-generic/pgalloc.h | 88 +++++--- include/asm-generic/tlb.h | 11 + include/linux/mm.h | 153 +++++++++---- include/linux/mm_types.h | 14 -- include/linux/page-flags.h | 30 ++- include/linux/pgtable.h | 77 +++++++ mm/memory.c | 8 +- 39 files changed, 686 insertions(+), 455 deletions(-)