[PATCH 0/3] memblock: cleanup memblock_free interface

Mike Rapoport posted 3 patches 3 years, 2 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
arch/alpha/kernel/core_irongate.c         |  2 +-
arch/arc/mm/init.c                        |  2 +-
arch/arm/mach-hisi/platmcpm.c             |  2 +-
arch/arm/mm/init.c                        |  2 +-
arch/arm64/mm/mmu.c                       |  4 ++--
arch/mips/mm/init.c                       |  2 +-
arch/mips/sgi-ip30/ip30-setup.c           |  6 +++---
arch/powerpc/kernel/dt_cpu_ftrs.c         |  2 +-
arch/powerpc/kernel/paca.c                |  4 ++--
arch/powerpc/kernel/setup-common.c        |  2 +-
arch/powerpc/kernel/setup_64.c            |  2 +-
arch/powerpc/platforms/powernv/pci-ioda.c |  2 +-
arch/powerpc/platforms/pseries/svm.c      |  4 +---
arch/riscv/kernel/setup.c                 |  4 ++--
arch/s390/kernel/setup.c                  |  8 ++++----
arch/s390/kernel/smp.c                    |  4 ++--
arch/s390/kernel/uv.c                     |  2 +-
arch/s390/mm/kasan_init.c                 |  2 +-
arch/sh/boards/mach-ap325rxa/setup.c      |  2 +-
arch/sh/boards/mach-ecovec24/setup.c      |  4 ++--
arch/sh/boards/mach-kfr2r09/setup.c       |  2 +-
arch/sh/boards/mach-migor/setup.c         |  2 +-
arch/sh/boards/mach-se/7724/setup.c       |  4 ++--
arch/sparc/kernel/smp_64.c                |  2 +-
arch/um/kernel/mem.c                      |  2 +-
arch/x86/kernel/setup.c                   |  4 ++--
arch/x86/kernel/setup_percpu.c            |  2 +-
arch/x86/mm/init.c                        |  2 +-
arch/x86/mm/kasan_init_64.c               |  4 ++--
arch/x86/mm/numa.c                        |  2 +-
arch/x86/mm/numa_emulation.c              |  2 +-
arch/x86/xen/mmu_pv.c                     |  6 +++---
arch/x86/xen/p2m.c                        |  2 +-
arch/x86/xen/setup.c                      |  6 +++---
drivers/base/arch_numa.c                  | 10 ++++------
drivers/firmware/efi/memmap.c             |  2 +-
drivers/macintosh/smu.c                   |  2 +-
drivers/of/kexec.c                        |  2 +-
drivers/of/of_reserved_mem.c              |  4 ++--
drivers/s390/char/sclp_early.c            |  2 +-
drivers/usb/early/xhci-dbc.c              | 10 +++++-----
drivers/xen/swiotlb-xen.c                 |  2 +-
include/linux/memblock.h                  | 16 ++--------------
init/initramfs.c                          |  2 +-
init/main.c                               |  2 +-
kernel/dma/swiotlb.c                      |  2 +-
kernel/printk/printk.c                    |  4 ++--
lib/bootconfig.c                          |  2 +-
lib/cpumask.c                             |  2 +-
mm/cma.c                                  |  2 +-
mm/memblock.c                             | 20 ++++++++++----------
mm/memory_hotplug.c                       |  2 +-
mm/percpu.c                               |  8 ++++----
mm/sparse.c                               |  2 +-
tools/bootconfig/include/linux/memblock.h |  2 +-
55 files changed, 94 insertions(+), 110 deletions(-)
[PATCH 0/3] memblock: cleanup memblock_free interface
Posted by Mike Rapoport 3 years, 2 months ago
From: Mike Rapoport <rppt@linux.ibm.com>

Hi,

Following the discussion on [1] this is the fix for memblock freeing APIs
mismatch. 

The first patch is a cleanup of numa_distance allocation in arch_numa I've
spotted during the conversion.
The second patch is a fix for Xen memory freeing on some of the error
paths.

The core change is in the third patch that makes memblock_free() a
counterpart of memblock_alloc() and adds memblock_phys_alloc() to be a
counterpart of memblock_phys_alloc().

Since scripts/get_maintainer.pl returned more than 100 addresses I've
trimmed the distribution list only to the relevant lists.

[1] https://lore.kernel.org/all/CAHk-=wj9k4LZTz+svCxLYs5Y1=+yKrbAUArH1+ghyG3OLd8VVg@mail.gmail.com

Mike Rapoport (3):
  arch_numa: simplify numa_distance allocation
  xen/x86: free_p2m_page: use memblock_free_ptr() to free a virtual pointer
  memblock: cleanup memblock_free interface

 arch/alpha/kernel/core_irongate.c         |  2 +-
 arch/arc/mm/init.c                        |  2 +-
 arch/arm/mach-hisi/platmcpm.c             |  2 +-
 arch/arm/mm/init.c                        |  2 +-
 arch/arm64/mm/mmu.c                       |  4 ++--
 arch/mips/mm/init.c                       |  2 +-
 arch/mips/sgi-ip30/ip30-setup.c           |  6 +++---
 arch/powerpc/kernel/dt_cpu_ftrs.c         |  2 +-
 arch/powerpc/kernel/paca.c                |  4 ++--
 arch/powerpc/kernel/setup-common.c        |  2 +-
 arch/powerpc/kernel/setup_64.c            |  2 +-
 arch/powerpc/platforms/powernv/pci-ioda.c |  2 +-
 arch/powerpc/platforms/pseries/svm.c      |  4 +---
 arch/riscv/kernel/setup.c                 |  4 ++--
 arch/s390/kernel/setup.c                  |  8 ++++----
 arch/s390/kernel/smp.c                    |  4 ++--
 arch/s390/kernel/uv.c                     |  2 +-
 arch/s390/mm/kasan_init.c                 |  2 +-
 arch/sh/boards/mach-ap325rxa/setup.c      |  2 +-
 arch/sh/boards/mach-ecovec24/setup.c      |  4 ++--
 arch/sh/boards/mach-kfr2r09/setup.c       |  2 +-
 arch/sh/boards/mach-migor/setup.c         |  2 +-
 arch/sh/boards/mach-se/7724/setup.c       |  4 ++--
 arch/sparc/kernel/smp_64.c                |  2 +-
 arch/um/kernel/mem.c                      |  2 +-
 arch/x86/kernel/setup.c                   |  4 ++--
 arch/x86/kernel/setup_percpu.c            |  2 +-
 arch/x86/mm/init.c                        |  2 +-
 arch/x86/mm/kasan_init_64.c               |  4 ++--
 arch/x86/mm/numa.c                        |  2 +-
 arch/x86/mm/numa_emulation.c              |  2 +-
 arch/x86/xen/mmu_pv.c                     |  6 +++---
 arch/x86/xen/p2m.c                        |  2 +-
 arch/x86/xen/setup.c                      |  6 +++---
 drivers/base/arch_numa.c                  | 10 ++++------
 drivers/firmware/efi/memmap.c             |  2 +-
 drivers/macintosh/smu.c                   |  2 +-
 drivers/of/kexec.c                        |  2 +-
 drivers/of/of_reserved_mem.c              |  4 ++--
 drivers/s390/char/sclp_early.c            |  2 +-
 drivers/usb/early/xhci-dbc.c              | 10 +++++-----
 drivers/xen/swiotlb-xen.c                 |  2 +-
 include/linux/memblock.h                  | 16 ++--------------
 init/initramfs.c                          |  2 +-
 init/main.c                               |  2 +-
 kernel/dma/swiotlb.c                      |  2 +-
 kernel/printk/printk.c                    |  4 ++--
 lib/bootconfig.c                          |  2 +-
 lib/cpumask.c                             |  2 +-
 mm/cma.c                                  |  2 +-
 mm/memblock.c                             | 20 ++++++++++----------
 mm/memory_hotplug.c                       |  2 +-
 mm/percpu.c                               |  8 ++++----
 mm/sparse.c                               |  2 +-
 tools/bootconfig/include/linux/memblock.h |  2 +-
 55 files changed, 94 insertions(+), 110 deletions(-)


base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
-- 
2.28.0


Re: [PATCH 0/3] memblock: cleanup memblock_free interface
Posted by Linus Torvalds 3 years, 2 months ago
On Thu, Sep 23, 2021 at 12:43 AM Mike Rapoport <rppt@kernel.org> wrote:
>
> The core change is in the third patch that makes memblock_free() a
> counterpart of memblock_alloc() and adds memblock_phys_alloc() to be a

^^^^^^^^^^^^^^^^^^^
> counterpart of memblock_phys_alloc().

That should be 'memblock_phys_free()'

HOWEVER.

The real reason I'm replying is that this patch is horribly buggy, and
will cause subtle problems that are nasty to debug.

You need to be a LOT more careful.

From a trivial check - exactly because I looked at doing it with a
script, and decided it's not so easy - I found cases like this:

-               memblock_free(__pa(paca_ptrs) + new_ptrs_size,
+               memblock_free(paca_ptrs + new_ptrs_size,

which is COMPLETELY wrong.

Why? Because now that addition is done as _pointer_ addition, not as
an integer addition, and the end result is something completely
different.

pcac_ptrs is of type 'struct paca_struct **', so when you add
new_ptrs_size to it, it will add it in terms of that many pointers,
not that many bytes.

You need to use some smarter scripting, or some way to validate it.

And no, making the scripting just replace '__pa(x)' with '(void *)(x)'
- which _would_ be mindless and get the same result - is not
acceptable either, because it avoids one of the big improvements from
using the right interface, namely having compiler type checking (and
saner code that people understand).

So NAK. No broken automated scripting patches.

               Linus

Re: [PATCH 0/3] memblock: cleanup memblock_free interface
Posted by Mike Rapoport 3 years, 2 months ago
Hi Linus,

On Thu, Sep 23, 2021 at 09:01:46AM -0700, Linus Torvalds wrote:
> On Thu, Sep 23, 2021 at 12:43 AM Mike Rapoport <rppt@kernel.org> wrote:
> >
> You need to be a LOT more careful.
> 
> From a trivial check - exactly because I looked at doing it with a
> script, and decided it's not so easy - I found cases like this:
> 
> -               memblock_free(__pa(paca_ptrs) + new_ptrs_size,
> +               memblock_free(paca_ptrs + new_ptrs_size,
> 
> which is COMPLETELY wrong.

I did use a coccinelle script that's slightly more robust that a sed you've
sent, but then I did a manual review, hence the two small patches with
fixes. Indeed I missed this one, so to be on the safe side I'll rename only
the obvious cases where coccinelle can be used reliably and leave all the
rest as it's now. If somebody cares enough they can update it later.
 
> And no, making the scripting just replace '__pa(x)' with '(void *)(x)'

These were actually manual and they are required for variables that
used as virtual addresses but have unsigned long type, like e.g.
initrd_start. So it's either __pa(x) or (void *).

-- 
Sincerely yours,
Mike.