| Message ID | 20180112143417.16743-8-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 12, 2018 at 03:34:13PM +0100, David Hildenbrand wrote: > We now have the range of free memory, let's initialize the physical > allocator. It is now possible to use alloc_page()/alloc_pages(). Commit message isn't quite right. After this patch it's possible to use malloc and memalign, based on the early-ops, but alloc_pages() requires the freelist to be initialized first by free_pages(), done by setup_vm(). Actually, since we have four states of memory management setup, then I think we either need three setup calls: state 1: no setup done, no call state 2: phys-alloc is set up by the phys_alloc_init() call state 3: alloc_page() is set up, need call that allocates chunk with the phys-allocator for free_pages() (Instead of a chunk, all memory could be given, but then either a new set of early-ops should be provided that are based on alloc_page(), or all early memory allocations must use alloc_page()) state 4: virtual memory is set up by the setup_vm() call Or, simply remove state 2 and its code and alloc-ops, changing all early allocations to use alloc_page(). If each arch provides an mmu_enabled() call, then the alloc_ops->memalign call in memalign could be replaced with if (mmu_enabled()) vm_memalign(...) else /* fallback to alloc_pages() */ I probably should have written all this in a separate thread, because I wouldn't want to hold this series up on a rework of the general memory management framework. But, anyway, thoughts on that? Paolo? Thanks, drew > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > lib/s390x/sclp.c | 12 ++++++++++++ > s390x/Makefile | 3 +++ > 2 files changed, 15 insertions(+) > > diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c > index 199405c..c7471b1 100644 > --- a/lib/s390x/sclp.c > +++ b/lib/s390x/sclp.c > @@ -15,11 +15,21 @@ > #include <asm/arch_def.h> > #include <asm/interrupt.h> > #include "sclp.h" > +#include <alloc_phys.h> > + > +extern unsigned long stacktop; > > static uint64_t storage_increment_size; > static uint64_t max_ram_size; > static uint64_t ram_size; > > +static void mem_init(phys_addr_t mem_end) > +{ > + phys_addr_t freemem_start = (phys_addr_t)&stacktop & PAGE_MASK; > + > + phys_alloc_init(freemem_start, mem_end - freemem_start); > +} > + > void sclp_memory_setup(void) > { > ReadInfo *ri = (void *)_sccb; > @@ -55,4 +65,6 @@ void sclp_memory_setup(void) > break; > ram_size += storage_increment_size; > } > + > + mem_init(ram_size); > } > diff --git a/s390x/Makefile b/s390x/Makefile > index ce63dd1..4198fdc 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -21,6 +21,9 @@ asm-offsets = lib/$(ARCH)/asm-offsets.h > include $(SRCDIR)/scripts/asm-offsets.mak > > cflatobjs += lib/util.o > +cflatobjs += lib/alloc.o > +cflatobjs += lib/alloc_phys.o > +cflatobjs += lib/alloc_page.o > cflatobjs += lib/alloc_phys.o > cflatobjs += lib/s390x/io.o > cflatobjs += lib/s390x/stack.o > -- > 2.14.3 >
On 15.01.2018 11:16, Andrew Jones wrote: > On Fri, Jan 12, 2018 at 03:34:13PM +0100, David Hildenbrand wrote: >> We now have the range of free memory, let's initialize the physical >> allocator. It is now possible to use alloc_page()/alloc_pages(). > > Commit message isn't quite right. After this patch it's possible to > use malloc and memalign, based on the early-ops, but alloc_pages() > requires the freelist to be initialized first by free_pages(), done > by setup_vm(). That's interesting, as I am using alloc_pages() during setup_vm() I haven't noticed it. > > Actually, since we have four states of memory management setup, > then I think we either need three setup calls: > > state 1: no setup done, no call > state 2: phys-alloc is set up by the phys_alloc_init() call > state 3: alloc_page() is set up, need call that allocates chunk with the > phys-allocator for free_pages() > > (Instead of a chunk, all memory could be given, but then either a > new set of early-ops should be provided that are based on > alloc_page(), or all early memory allocations must use alloc_page()) > > state 4: virtual memory is set up by the setup_vm() call > > Or, simply remove state 2 and its code and alloc-ops, changing all early > allocations to use alloc_page(). If each arch provides an mmu_enabled() > call, then the alloc_ops->memalign call in memalign could be replaced > with I think I'd prefer that. Unless somebody needs the flexibility of this extra state. But let's hear what the others say. > > if (mmu_enabled()) > vm_memalign(...) > else > /* fallback to alloc_pages() */ > > I probably should have written all this in a separate thread, because > I wouldn't want to hold this series up on a rework of the general > memory management framework. But, anyway, thoughts on that? Paolo?> > Thanks, > drew >
On 15/01/2018 11:21, David Hildenbrand wrote: >> Commit message isn't quite right. After this patch it's possible to >> use malloc and memalign, based on the early-ops, but alloc_pages() >> requires the freelist to be initialized first by free_pages(), done >> by setup_vm(). > > That's interesting, as I am using alloc_pages() during setup_vm() I > haven't noticed it. You're using it during setup_mmu, not setup_vm. Paolo
On 15.01.2018 11:31, Paolo Bonzini wrote: > On 15/01/2018 11:21, David Hildenbrand wrote: >>> Commit message isn't quite right. After this patch it's possible to >>> use malloc and memalign, based on the early-ops, but alloc_pages() >>> requires the freelist to be initialized first by free_pages(), done >>> by setup_vm(). >> >> That's interesting, as I am using alloc_pages() during setup_vm() I >> haven't noticed it. > > You're using it during setup_mmu, not setup_vm. ... which is only called from setup_vm(). > > Paolo >
On 15/01/2018 11:16, Andrew Jones wrote: > state 2: phys-alloc is set up by the phys_alloc_init() call > > state 3: alloc_page() is set up, need call that allocates chunk with the > phys-allocator for free_pages() > > (Instead of a chunk, all memory could be given, but then either a > new set of early-ops should be provided that are based on > alloc_page(), or all early memory allocations must use alloc_page()) > > state 4: virtual memory is set up by the setup_vm() call > > Or, simply remove state 2 and its code and alloc-ops, changing all early > allocations to use alloc_page(). If each arch provides an mmu_enabled() > call, then the alloc_ops->memalign call in memalign could be replaced > with > > if (mmu_enabled()) > vm_memalign(...) > else > /* fallback to alloc_pages() */ > > I probably should have written all this in a separate thread, because > I wouldn't want to hold this series up on a rework of the general > memory management framework. But, anyway, thoughts on that? Paolo? State 3 should only exist in setup_mmu, so indeed the commit message here is not accurate. setup_vm does the transition from phys_alloc to page_alloc before calling setup_mmu, exactly because setup_mmu already needs to allocate pages: phys_alloc_get_unused(&base, &top); base = (base + PAGE_SIZE - 1) & -PAGE_SIZE; top = top & -PAGE_SIZE; free_pages(phys_to_virt(base), top - base); page_root = setup_mmu(top); alloc_ops = &vmalloc_ops; The reason for state 2 to exist is that right now on x86 it is optional to use the MMU. You could use page-granularity allocation, but that would be a waste of memory if we switch to a GTest-like API that needs to dynamically allocate memory. However, maybe it would make sense to make it internal just like state 3. That would mean enabling the MMU directly where we now call phys_alloc_init. This is independent of course of this s390 series. Paolo
On 15.01.2018 11:16, Andrew Jones wrote: > On Fri, Jan 12, 2018 at 03:34:13PM +0100, David Hildenbrand wrote: >> We now have the range of free memory, let's initialize the physical >> allocator. It is now possible to use alloc_page()/alloc_pages(). Will rephrase to: "It is now possible to use use malloc and memalign, based on the early-ops. alloc_pages() cannot be used yet (as it has to be initialized via free_pages() - e.g. via setup_vm() - first)." Thanks! > > Commit message isn't quite right. After this patch it's possible to > use malloc and memalign, based on the early-ops, but alloc_pages() > requires the freelist to be initialized first by free_pages(), done > by setup_vm(). > > Actually, since we have four states of memory management setup, > then I think we either need three setup calls: > > state 1: no setup done, no call > state 2: phys-alloc is set up by the phys_alloc_init() call > state 3: alloc_page() is set up, need call that allocates chunk with the > phys-allocator for free_pages() > > (Instead of a chunk, all memory could be given, but then either a > new set of early-ops should be provided that are based on > alloc_page(), or all early memory allocations must use alloc_page()) > > state 4: virtual memory is set up by the setup_vm() call > > Or, simply remove state 2 and its code and alloc-ops, changing all early > allocations to use alloc_page(). If each arch provides an mmu_enabled() > call, then the alloc_ops->memalign call in memalign could be replaced > with > > if (mmu_enabled()) > vm_memalign(...) > else > /* fallback to alloc_pages() */ > > I probably should have written all this in a separate thread, because > I wouldn't want to hold this series up on a rework of the general > memory management framework. But, anyway, thoughts on that? Paolo? > > Thanks, > drew
On Mon, Jan 15, 2018 at 11:38:30AM +0100, Paolo Bonzini wrote: > On 15/01/2018 11:16, Andrew Jones wrote: > > state 2: phys-alloc is set up by the phys_alloc_init() call > > > > state 3: alloc_page() is set up, need call that allocates chunk with the > > phys-allocator for free_pages() > > > > (Instead of a chunk, all memory could be given, but then either a > > new set of early-ops should be provided that are based on > > alloc_page(), or all early memory allocations must use alloc_page()) > > > > state 4: virtual memory is set up by the setup_vm() call > > > > Or, simply remove state 2 and its code and alloc-ops, changing all early > > allocations to use alloc_page(). If each arch provides an mmu_enabled() > > call, then the alloc_ops->memalign call in memalign could be replaced > > with > > > > if (mmu_enabled()) > > vm_memalign(...) > > else > > /* fallback to alloc_pages() */ > > > > I probably should have written all this in a separate thread, because > > I wouldn't want to hold this series up on a rework of the general > > memory management framework. But, anyway, thoughts on that? Paolo? > > State 3 should only exist in setup_mmu, so indeed the commit message > here is not accurate. setup_vm does the transition from phys_alloc to > page_alloc before calling setup_mmu, exactly because setup_mmu already > needs to allocate pages: > > phys_alloc_get_unused(&base, &top); > base = (base + PAGE_SIZE - 1) & -PAGE_SIZE; > top = top & -PAGE_SIZE; > free_pages(phys_to_virt(base), top - base); > page_root = setup_mmu(top); > alloc_ops = &vmalloc_ops; > > The reason for state 2 to exist is that right now on x86 it is optional > to use the MMU. You could use page-granularity allocation, but that > would be a waste of memory if we switch to a GTest-like API that needs > to dynamically allocate memory. > > However, maybe it would make sense to make it internal just like state > 3. That would mean enabling the MMU directly where we now call > phys_alloc_init. This is independent of course of this s390 series. > I'm still looking at making the MMU enablement of ARM optional, and, like you say, it already is optional for x86, so I don't think we'd want to reduce the flexibility by making more setup internal. If we need the phys-allocator, then I think introducing an external setup call for state 3, allowing alloc_page() to be used before (without) the MMU being enabled, probably makes the most sense. Thanks, drew
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c index 199405c..c7471b1 100644 --- a/lib/s390x/sclp.c +++ b/lib/s390x/sclp.c @@ -15,11 +15,21 @@ #include <asm/arch_def.h> #include <asm/interrupt.h> #include "sclp.h" +#include <alloc_phys.h> + +extern unsigned long stacktop; static uint64_t storage_increment_size; static uint64_t max_ram_size; static uint64_t ram_size; +static void mem_init(phys_addr_t mem_end) +{ + phys_addr_t freemem_start = (phys_addr_t)&stacktop & PAGE_MASK; + + phys_alloc_init(freemem_start, mem_end - freemem_start); +} + void sclp_memory_setup(void) { ReadInfo *ri = (void *)_sccb; @@ -55,4 +65,6 @@ void sclp_memory_setup(void) break; ram_size += storage_increment_size; } + + mem_init(ram_size); } diff --git a/s390x/Makefile b/s390x/Makefile index ce63dd1..4198fdc 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -21,6 +21,9 @@ asm-offsets = lib/$(ARCH)/asm-offsets.h include $(SRCDIR)/scripts/asm-offsets.mak cflatobjs += lib/util.o +cflatobjs += lib/alloc.o +cflatobjs += lib/alloc_phys.o +cflatobjs += lib/alloc_page.o cflatobjs += lib/alloc_phys.o cflatobjs += lib/s390x/io.o cflatobjs += lib/s390x/stack.o
We now have the range of free memory, let's initialize the physical allocator. It is now possible to use alloc_page()/alloc_pages(). Signed-off-by: David Hildenbrand <david@redhat.com> --- lib/s390x/sclp.c | 12 ++++++++++++ s390x/Makefile | 3 +++ 2 files changed, 15 insertions(+)