Re: [PATCH 3/3] memblock: cleanup memblock_free interface
On Thu, Sep 23, 2021 at 03:54:46PM +0200, Christophe Leroy wrote: > > Le 23/09/2021 à 14:01, Mike Rapoport a écrit : > > On Thu, Sep 23, 2021 at 11:47:48AM +0200, Christophe Leroy wrote: > > > > > > > > > Le 23/09/2021 à 09:43, Mike Rapoport a écrit : > > > > From: Mike Rapoport > > > > > > > > For ages memblock_free() interface dealt with physical addresses even > > > > despite the existence of memblock_alloc_xx() functions that return a > > > > virtual pointer. > > > > > > > > Introduce memblock_phys_free() for freeing physical ranges and repurpose > > > > memblock_free() to free virtual pointers to make the following pairing > > > > abundantly clear: > > > > > > > > int memblock_phys_free(phys_addr_t base, phys_addr_t size); > > > > phys_addr_t memblock_phys_alloc(phys_addr_t base, phys_addr_t > > > > size); > > > > > > > > void *memblock_alloc(phys_addr_t size, phys_addr_t align); > > > > void memblock_free(void *ptr, size_t size); > > > > > > > > Replace intermediate memblock_free_ptr() with memblock_free() and drop > > > > unnecessary aliases memblock_free_early() and memblock_free_early_nid(). > > > > > > > > Suggested-by: Linus Torvalds > > > > Signed-off-by: Mike Rapoport > > > > --- > > > > > > > diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c > > > > index 1a04e5bdf655..37826d8c4f74 100644 > > > > --- a/arch/s390/kernel/smp.c > > > > +++ b/arch/s390/kernel/smp.c > > > > @@ -723,7 +723,7 @@ void __init smp_save_dump_cpus(void) > > > > /* Get the CPU registers */ > > > > smp_save_cpu_regs(sa, addr, is_boot_cpu, page); > > > > } > > > > - memblock_free(page, PAGE_SIZE); > > > > + memblock_phys_free(page, PAGE_SIZE); > > > > diag_amode31_ops.diag308_reset(); > > > > pcpu_set_smt(0); > > > >} > > > > @@ -880,7 +880,7 @@ void __init smp_detect_cpus(void) > > > > /* Add CPUs present at boot */ > > > > __smp_rescan_cpus(info, true); > > > > - memblock_free_early((unsigned long)info, sizeof(*info)); > > > > + memblock_free(info, sizeof(*info)); > > > >} > > > >/* > > > > > > I'm a bit lost. IIUC memblock_free_early() and memblock_free() where > > > identical. > > > > Yes, they were, but all calls to memblock_free_early() were using > > __pa(vaddr) because they had a virtual address at hand. > > I'm still not following. In the above memblock_free_early() was taking > (unsigned long)info . Was it a bug ? Not really because s390 has pa == va: https://elixir.bootlin.com/linux/latest/source/arch/s390/include/asm/page.h#L169 -- Sincerely yours, Mike. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] memblock: cleanup memblock_free interface
Le 23/09/2021 à 14:01, Mike Rapoport a écrit : On Thu, Sep 23, 2021 at 11:47:48AM +0200, Christophe Leroy wrote: Le 23/09/2021 à 09:43, Mike Rapoport a écrit : From: Mike Rapoport For ages memblock_free() interface dealt with physical addresses even despite the existence of memblock_alloc_xx() functions that return a virtual pointer. Introduce memblock_phys_free() for freeing physical ranges and repurpose memblock_free() to free virtual pointers to make the following pairing abundantly clear: int memblock_phys_free(phys_addr_t base, phys_addr_t size); phys_addr_t memblock_phys_alloc(phys_addr_t base, phys_addr_t size); void *memblock_alloc(phys_addr_t size, phys_addr_t align); void memblock_free(void *ptr, size_t size); Replace intermediate memblock_free_ptr() with memblock_free() and drop unnecessary aliases memblock_free_early() and memblock_free_early_nid(). Suggested-by: Linus Torvalds Signed-off-by: Mike Rapoport --- diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c index 1a04e5bdf655..37826d8c4f74 100644 --- a/arch/s390/kernel/smp.c +++ b/arch/s390/kernel/smp.c @@ -723,7 +723,7 @@ void __init smp_save_dump_cpus(void) /* Get the CPU registers */ smp_save_cpu_regs(sa, addr, is_boot_cpu, page); } - memblock_free(page, PAGE_SIZE); + memblock_phys_free(page, PAGE_SIZE); diag_amode31_ops.diag308_reset(); pcpu_set_smt(0); } @@ -880,7 +880,7 @@ void __init smp_detect_cpus(void) /* Add CPUs present at boot */ __smp_rescan_cpus(info, true); - memblock_free_early((unsigned long)info, sizeof(*info)); + memblock_free(info, sizeof(*info)); } /* I'm a bit lost. IIUC memblock_free_early() and memblock_free() where identical. Yes, they were, but all calls to memblock_free_early() were using __pa(vaddr) because they had a virtual address at hand. I'm still not following. In the above memblock_free_early() was taking (unsigned long)info . Was it a bug ? It looks odd to hide bug fixes in such a big patch, should that bug fix go in patch 2 ? In the first hunk memblock_free() gets replaced by memblock_phys_free() In the second hunk memblock_free_early() gets replaced by memblock_free() In the first hunk the memory is allocated with memblock_phys_alloc() and we have a physical range to free. In the second hunk the memory is allocated with memblock_alloc() and we are freeing a virtual pointer. I think it would be easier to follow if you could split it in several patches: It was an explicit request from Linus to make it a single commit: but the actual commit can and should be just a single commit that just fixes 'memblock_free()' to have sane interfaces. I don't feel strongly about splitting it (except my laziness really objects), but I don't think doing the conversion in several steps worth the churn. The commit is quite big (55 files changed, approx 100 lines modified). If done in the right order the change should be minimal. It is rather not-easy to follow and review when a function that was existing (namely memblock_free() ) disappears and re-appears in the same commit but to do something different. You do: - memblock_free() ==> memblock_phys_free() - memblock_free_ptr() ==> memblock_free() At least you could split in two patches, the advantage would be that between first and second patch memblock() doesn't exist anymore so you can check you really don't have anymore user. - First patch: Create memblock_phys_free() and change all relevant memblock_free() to memblock_phys_free() - Or change memblock_free() to memblock_phys_free() and make memblock_free() an alias of it. - Second patch: Make memblock_free_ptr() become memblock_free() and change all remaining callers to the new semantics (IIUC memblock_free(__pa(ptr)) becomes memblock_free(ptr) and make memblock_free_ptr() an alias of memblock_free() - Fourth patch: Replace and drop memblock_free_ptr() - Fifth patch: Drop memblock_free_early() and memblock_free_early_nid() (All users should have been upgraded to memblock_free_phys() in patch 1 or memblock_free() in patch 2) Christophe ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] memblock: cleanup memblock_free interface
On 9/23/21 9:43 AM, Mike Rapoport wrote: > From: Mike Rapoport > > For ages memblock_free() interface dealt with physical addresses even > despite the existence of memblock_alloc_xx() functions that return a > virtual pointer. > > Introduce memblock_phys_free() for freeing physical ranges and repurpose > memblock_free() to free virtual pointers to make the following pairing > abundantly clear: > > int memblock_phys_free(phys_addr_t base, phys_addr_t size); > phys_addr_t memblock_phys_alloc(phys_addr_t base, phys_addr_t size); > > void *memblock_alloc(phys_addr_t size, phys_addr_t align); > void memblock_free(void *ptr, size_t size); > > Replace intermediate memblock_free_ptr() with memblock_free() and drop > unnecessary aliases memblock_free_early() and memblock_free_early_nid(). > > Suggested-by: Linus Torvalds > Signed-off-by: Mike Rapoport arch/arc part: Reviewed-by: Shahab Vahedi Thanks, Shahab ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] memblock: cleanup memblock_free interface
On Thu, Sep 23, 2021 at 11:47:48AM +0200, Christophe Leroy wrote: > > > Le 23/09/2021 à 09:43, Mike Rapoport a écrit : > > From: Mike Rapoport > > > > For ages memblock_free() interface dealt with physical addresses even > > despite the existence of memblock_alloc_xx() functions that return a > > virtual pointer. > > > > Introduce memblock_phys_free() for freeing physical ranges and repurpose > > memblock_free() to free virtual pointers to make the following pairing > > abundantly clear: > > > > int memblock_phys_free(phys_addr_t base, phys_addr_t size); > > phys_addr_t memblock_phys_alloc(phys_addr_t base, phys_addr_t size); > > > > void *memblock_alloc(phys_addr_t size, phys_addr_t align); > > void memblock_free(void *ptr, size_t size); > > > > Replace intermediate memblock_free_ptr() with memblock_free() and drop > > unnecessary aliases memblock_free_early() and memblock_free_early_nid(). > > > > Suggested-by: Linus Torvalds > > Signed-off-by: Mike Rapoport > > --- > > > diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c > > index 1a04e5bdf655..37826d8c4f74 100644 > > --- a/arch/s390/kernel/smp.c > > +++ b/arch/s390/kernel/smp.c > > @@ -723,7 +723,7 @@ void __init smp_save_dump_cpus(void) > > /* Get the CPU registers */ > > smp_save_cpu_regs(sa, addr, is_boot_cpu, page); > > } > > - memblock_free(page, PAGE_SIZE); > > + memblock_phys_free(page, PAGE_SIZE); > > diag_amode31_ops.diag308_reset(); > > pcpu_set_smt(0); > > } > > @@ -880,7 +880,7 @@ void __init smp_detect_cpus(void) > > /* Add CPUs present at boot */ > > __smp_rescan_cpus(info, true); > > - memblock_free_early((unsigned long)info, sizeof(*info)); > > + memblock_free(info, sizeof(*info)); > > } > > /* > > I'm a bit lost. IIUC memblock_free_early() and memblock_free() where > identical. Yes, they were, but all calls to memblock_free_early() were using __pa(vaddr) because they had a virtual address at hand. > In the first hunk memblock_free() gets replaced by memblock_phys_free() > In the second hunk memblock_free_early() gets replaced by memblock_free() In the first hunk the memory is allocated with memblock_phys_alloc() and we have a physical range to free. In the second hunk the memory is allocated with memblock_alloc() and we are freeing a virtual pointer. > I think it would be easier to follow if you could split it in several > patches: It was an explicit request from Linus to make it a single commit: but the actual commit can and should be just a single commit that just fixes 'memblock_free()' to have sane interfaces. I don't feel strongly about splitting it (except my laziness really objects), but I don't think doing the conversion in several steps worth the churn. > - First patch: Create memblock_phys_free() and change all relevant > memblock_free() to memblock_phys_free() - Or change memblock_free() to > memblock_phys_free() and make memblock_free() an alias of it. > - Second patch: Make memblock_free_ptr() become memblock_free() and change > all remaining callers to the new semantics (IIUC memblock_free(__pa(ptr)) > becomes memblock_free(ptr) and make memblock_free_ptr() an alias of > memblock_free() > - Fourth patch: Replace and drop memblock_free_ptr() > - Fifth patch: Drop memblock_free_early() and memblock_free_early_nid() (All > users should have been upgraded to memblock_free_phys() in patch 1 or > memblock_free() in patch 2) > > Christophe -- Sincerely yours, Mike. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] memblock: cleanup memblock_free interface
Le 23/09/2021 à 09:43, Mike Rapoport a écrit : From: Mike Rapoport For ages memblock_free() interface dealt with physical addresses even despite the existence of memblock_alloc_xx() functions that return a virtual pointer. Introduce memblock_phys_free() for freeing physical ranges and repurpose memblock_free() to free virtual pointers to make the following pairing abundantly clear: int memblock_phys_free(phys_addr_t base, phys_addr_t size); phys_addr_t memblock_phys_alloc(phys_addr_t base, phys_addr_t size); void *memblock_alloc(phys_addr_t size, phys_addr_t align); void memblock_free(void *ptr, size_t size); Replace intermediate memblock_free_ptr() with memblock_free() and drop unnecessary aliases memblock_free_early() and memblock_free_early_nid(). Suggested-by: Linus Torvalds Signed-off-by: Mike Rapoport --- diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c index 1a04e5bdf655..37826d8c4f74 100644 --- a/arch/s390/kernel/smp.c +++ b/arch/s390/kernel/smp.c @@ -723,7 +723,7 @@ void __init smp_save_dump_cpus(void) /* Get the CPU registers */ smp_save_cpu_regs(sa, addr, is_boot_cpu, page); } - memblock_free(page, PAGE_SIZE); + memblock_phys_free(page, PAGE_SIZE); diag_amode31_ops.diag308_reset(); pcpu_set_smt(0); } @@ -880,7 +880,7 @@ void __init smp_detect_cpus(void) /* Add CPUs present at boot */ __smp_rescan_cpus(info, true); - memblock_free_early((unsigned long)info, sizeof(*info)); + memblock_free(info, sizeof(*info)); } /* I'm a bit lost. IIUC memblock_free_early() and memblock_free() where identical. In the first hunk memblock_free() gets replaced by memblock_phys_free() In the second hunk memblock_free_early() gets replaced by memblock_free() I think it would be easier to follow if you could split it in several patches: - First patch: Create memblock_phys_free() and change all relevant memblock_free() to memblock_phys_free() - Or change memblock_free() to memblock_phys_free() and make memblock_free() an alias of it. - Second patch: Make memblock_free_ptr() become memblock_free() and change all remaining callers to the new semantics (IIUC memblock_free(__pa(ptr)) becomes memblock_free(ptr) and make memblock_free_ptr() an alias of memblock_free() - Fourth patch: Replace and drop memblock_free_ptr() - Fifth patch: Drop memblock_free_early() and memblock_free_early_nid() (All users should have been upgraded to memblock_free_phys() in patch 1 or memblock_free() in patch 2) Christophe ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] memblock: cleanup memblock_free interface
On 23.09.21 09:43, Mike Rapoport wrote: From: Mike Rapoport For ages memblock_free() interface dealt with physical addresses even despite the existence of memblock_alloc_xx() functions that return a virtual pointer. Introduce memblock_phys_free() for freeing physical ranges and repurpose memblock_free() to free virtual pointers to make the following pairing abundantly clear: int memblock_phys_free(phys_addr_t base, phys_addr_t size); phys_addr_t memblock_phys_alloc(phys_addr_t base, phys_addr_t size); void *memblock_alloc(phys_addr_t size, phys_addr_t align); void memblock_free(void *ptr, size_t size); Replace intermediate memblock_free_ptr() with memblock_free() and drop unnecessary aliases memblock_free_early() and memblock_free_early_nid(). Suggested-by: Linus Torvalds Signed-off-by: Mike Rapoport arch/x86/xen/ parts: Reviewed-by: Juergen Gross Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
