Re: [PATCH] memblock: stop using implicit alignement to SMP_CACHE_BYTES
On Fri, Oct 05, 2018 at 03:19:34PM -0700, Andrew Morton wrote:
> On Fri, 5 Oct 2018 00:07:04 +0300 Mike Rapoport
> wrote:
>
> > When a memblock allocation APIs are called with align = 0, the alignment is
> > implicitly set to SMP_CACHE_BYTES.
> >
> > Replace all such uses of memblock APIs with the 'align' parameter explicitly
> > set to SMP_CACHE_BYTES and stop implicit alignment assignment in the
> > memblock internal allocation functions.
> >
> > For the case when memblock APIs are used via helper functions, e.g. like
> > iommu_arena_new_node() in Alpha, the helper functions were detected with
> > Coccinelle's help and then manually examined and updated where appropriate.
> >
> > ...
> >
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -1298,9 +1298,6 @@ static phys_addr_t __init
> > memblock_alloc_range_nid(phys_addr_t size,
> > {
> > phys_addr_t found;
> >
> > - if (!align)
> > - align = SMP_CACHE_BYTES;
> > -
>
> Can we add a WARN_ON_ONCE(!align) here? To catch unconverted code
> which sneaks in later on.
Here it goes:
>From baec825c58e8bc11371433d3a4b20b2216877a50 Mon Sep 17 00:00:00 2001
From: Mike Rapoport
Date: Mon, 8 Oct 2018 11:22:10 +0300
Subject: [PATCH] memblock: warn if zero alignment was requested
After update of all memblock users to explicitly specify SMP_CACHE_BYTES
alignment rather than use 0, it is still possible that uncovered users
may sneak in. Add a WARN_ON_ONCE for such cases.
Signed-off-by: Mike Rapoport
---
mm/memblock.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/mm/memblock.c b/mm/memblock.c
index 0bbae56..5fefc70 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1298,6 +1298,9 @@ static phys_addr_t __init
memblock_alloc_range_nid(phys_addr_t size,
{
phys_addr_t found;
+ if (WARN_ON_ONCE(!align))
+ align = SMP_CACHE_BYTES;
+
found = memblock_find_in_range_node(size, align, start, end, nid,
flags);
if (found && !memblock_reserve(found, size)) {
@@ -1420,6 +1423,9 @@ static void * __init memblock_alloc_internal(
if (WARN_ON_ONCE(slab_is_available()))
return kzalloc_node(size, GFP_NOWAIT, nid);
+ if (WARN_ON_ONCE(!align))
+ align = SMP_CACHE_BYTES;
+
if (max_addr > memblock.current_limit)
max_addr = memblock.current_limit;
again:
--
2.7.4
--
Sincerely yours,
Mike.
Re: [PATCH] memblock: stop using implicit alignement to SMP_CACHE_BYTES
Mike Rapoport writes: > When a memblock allocation APIs are called with align = 0, the alignment is > implicitly set to SMP_CACHE_BYTES. > > Replace all such uses of memblock APIs with the 'align' parameter explicitly > set to SMP_CACHE_BYTES and stop implicit alignment assignment in the > memblock internal allocation functions. > > For the case when memblock APIs are used via helper functions, e.g. like > iommu_arena_new_node() in Alpha, the helper functions were detected with > Coccinelle's help and then manually examined and updated where appropriate. > > The direct memblock APIs users were updated using the semantic patch below: > > @@ > expression size, min_addr, max_addr, nid; > @@ > ( > | > - memblock_alloc_try_nid_raw(size, 0, min_addr, max_addr, nid) > + memblock_alloc_try_nid_raw(size, SMP_CACHE_BYTES, min_addr, max_addr, > nid) > | > - memblock_alloc_try_nid_nopanic(size, 0, min_addr, max_addr, nid) > + memblock_alloc_try_nid_nopanic(size, SMP_CACHE_BYTES, min_addr, max_addr, > nid) > | > - memblock_alloc_try_nid(size, 0, min_addr, max_addr, nid) > + memblock_alloc_try_nid(size, SMP_CACHE_BYTES, min_addr, max_addr, nid) > | > - memblock_alloc(size, 0) > + memblock_alloc(size, SMP_CACHE_BYTES) > | > - memblock_alloc_raw(size, 0) > + memblock_alloc_raw(size, SMP_CACHE_BYTES) > | > - memblock_alloc_from(size, 0, min_addr) > + memblock_alloc_from(size, SMP_CACHE_BYTES, min_addr) > | > - memblock_alloc_nopanic(size, 0) > + memblock_alloc_nopanic(size, SMP_CACHE_BYTES) > | > - memblock_alloc_low(size, 0) > + memblock_alloc_low(size, SMP_CACHE_BYTES) > | > - memblock_alloc_low_nopanic(size, 0) > + memblock_alloc_low_nopanic(size, SMP_CACHE_BYTES) > | > - memblock_alloc_from_nopanic(size, 0, min_addr) > + memblock_alloc_from_nopanic(size, SMP_CACHE_BYTES, min_addr) > | > - memblock_alloc_node(size, 0, nid) > + memblock_alloc_node(size, SMP_CACHE_BYTES, nid) > ) > > Suggested-by: Michal Hocko > Signed-off-by: Mike Rapoport > --- ... > arch/powerpc/kernel/pci_32.c | 3 ++- > arch/powerpc/lib/alloc.c | 2 +- > arch/powerpc/mm/mmu_context_nohash.c | 7 +++--- > arch/powerpc/platforms/powermac/nvram.c | 2 +- > arch/powerpc/platforms/powernv/pci-ioda.c | 6 ++--- > arch/powerpc/sysdev/msi_bitmap.c | 2 +- The powerpc changes all look fine. I'm not quite clear on how SMP_CACHE_BYTES is getting included. I think it's: memblock.h -> mm.h -> mmzone.h -> cache.h So that's probably fine. Acked-by: Michael Ellerman (powerpc) cheers
Re: [PATCH] memblock: stop using implicit alignement to SMP_CACHE_BYTES
On Fri 05-10-18 00:07:04, Mike Rapoport wrote: > When a memblock allocation APIs are called with align = 0, the alignment is > implicitly set to SMP_CACHE_BYTES. I would add something like " Implicit alignment is done deep in the memblock allocator and it can come as a surprise. Not that such an alignment would be wrong even when used incorrectly but it is better to be explicit for the sake of clarity and the prinicple of the least surprise. " > Replace all such uses of memblock APIs with the 'align' parameter explicitly > set to SMP_CACHE_BYTES and stop implicit alignment assignment in the > memblock internal allocation functions. > > For the case when memblock APIs are used via helper functions, e.g. like > iommu_arena_new_node() in Alpha, the helper functions were detected with > Coccinelle's help and then manually examined and updated where appropriate. > > The direct memblock APIs users were updated using the semantic patch below: > > @@ > expression size, min_addr, max_addr, nid; > @@ > ( > | > - memblock_alloc_try_nid_raw(size, 0, min_addr, max_addr, nid) > + memblock_alloc_try_nid_raw(size, SMP_CACHE_BYTES, min_addr, max_addr, > nid) > | > - memblock_alloc_try_nid_nopanic(size, 0, min_addr, max_addr, nid) > + memblock_alloc_try_nid_nopanic(size, SMP_CACHE_BYTES, min_addr, max_addr, > nid) > | > - memblock_alloc_try_nid(size, 0, min_addr, max_addr, nid) > + memblock_alloc_try_nid(size, SMP_CACHE_BYTES, min_addr, max_addr, nid) > | > - memblock_alloc(size, 0) > + memblock_alloc(size, SMP_CACHE_BYTES) > | > - memblock_alloc_raw(size, 0) > + memblock_alloc_raw(size, SMP_CACHE_BYTES) > | > - memblock_alloc_from(size, 0, min_addr) > + memblock_alloc_from(size, SMP_CACHE_BYTES, min_addr) > | > - memblock_alloc_nopanic(size, 0) > + memblock_alloc_nopanic(size, SMP_CACHE_BYTES) > | > - memblock_alloc_low(size, 0) > + memblock_alloc_low(size, SMP_CACHE_BYTES) > | > - memblock_alloc_low_nopanic(size, 0) > + memblock_alloc_low_nopanic(size, SMP_CACHE_BYTES) > | > - memblock_alloc_from_nopanic(size, 0, min_addr) > + memblock_alloc_from_nopanic(size, SMP_CACHE_BYTES, min_addr) > | > - memblock_alloc_node(size, 0, nid) > + memblock_alloc_node(size, SMP_CACHE_BYTES, nid) > ) > > Suggested-by: Michal Hocko > Signed-off-by: Mike Rapoport I do agree that this is an improvement. I would also add WARN_ON_ONCE on 0 alignment to catch some left overs. If we ever grown a user which would explicitly require the zero alignment (I would be surprised) then we can remove the warning. Acked-by: Michal Hocko -- Michal Hocko SUSE Labs
Re: [PATCH] memblock: stop using implicit alignement to SMP_CACHE_BYTES
Hi Mike, On Fri, Oct 05, 2018 at 12:07:04AM +0300, Mike Rapoport wrote: > When a memblock allocation APIs are called with align = 0, the alignment is > implicitly set to SMP_CACHE_BYTES. > > Replace all such uses of memblock APIs with the 'align' parameter explicitly > set to SMP_CACHE_BYTES and stop implicit alignment assignment in the > memblock internal allocation functions. > >% > > Suggested-by: Michal Hocko > Signed-off-by: Mike Rapoport Acked-by: Paul Burton # MIPS part Thanks, Paul
Re: [PATCH] memblock: stop using implicit alignement to SMP_CACHE_BYTES
On Fri, 5 Oct 2018 00:07:04 +0300 Mike Rapoport
wrote:
> When a memblock allocation APIs are called with align = 0, the alignment is
> implicitly set to SMP_CACHE_BYTES.
>
> Replace all such uses of memblock APIs with the 'align' parameter explicitly
> set to SMP_CACHE_BYTES and stop implicit alignment assignment in the
> memblock internal allocation functions.
>
> For the case when memblock APIs are used via helper functions, e.g. like
> iommu_arena_new_node() in Alpha, the helper functions were detected with
> Coccinelle's help and then manually examined and updated where appropriate.
>
> ...
>
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1298,9 +1298,6 @@ static phys_addr_t __init
> memblock_alloc_range_nid(phys_addr_t size,
> {
> phys_addr_t found;
>
> - if (!align)
> - align = SMP_CACHE_BYTES;
> -
Can we add a WARN_ON_ONCE(!align) here? To catch unconverted code
which sneaks in later on.
Re: [PATCH] memblock: stop using implicit alignement to SMP_CACHE_BYTES
On October 5, 2018 6:25:38 AM GMT+03:00, Benjamin Herrenschmidt wrote: >On Fri, 2018-10-05 at 00:07 +0300, Mike Rapoport wrote: >> When a memblock allocation APIs are called with align = 0, the >alignment is >> implicitly set to SMP_CACHE_BYTES. >> >> Replace all such uses of memblock APIs with the 'align' parameter >explicitly >> set to SMP_CACHE_BYTES and stop implicit alignment assignment in the >> memblock internal allocation functions. >> >> For the case when memblock APIs are used via helper functions, e.g. >like >> iommu_arena_new_node() in Alpha, the helper functions were detected >with >> Coccinelle's help and then manually examined and updated where >appropriate. >> >> The direct memblock APIs users were updated using the semantic patch >below: > >What is the purpose of this ? It sounds rather counter-intuitive... Why? I think it actually more intuitive to explicitly set alignment to SMP_CACHE_BYTES rather than use align = 0 because deeply inside allocator it will be implicitly reset to SMP_CACHE_BYTES... >Ben. -- Sincerely yours, Mike.
Re: [PATCH] memblock: stop using implicit alignement to SMP_CACHE_BYTES
On Fri, 2018-10-05 at 00:07 +0300, Mike Rapoport wrote: > When a memblock allocation APIs are called with align = 0, the alignment is > implicitly set to SMP_CACHE_BYTES. > > Replace all such uses of memblock APIs with the 'align' parameter explicitly > set to SMP_CACHE_BYTES and stop implicit alignment assignment in the > memblock internal allocation functions. > > For the case when memblock APIs are used via helper functions, e.g. like > iommu_arena_new_node() in Alpha, the helper functions were detected with > Coccinelle's help and then manually examined and updated where appropriate. > > The direct memblock APIs users were updated using the semantic patch below: What is the purpose of this ? It sounds rather counter-intuitive... Ben.
[PATCH] memblock: stop using implicit alignement to SMP_CACHE_BYTES
When a memblock allocation APIs are called with align = 0, the alignment is implicitly set to SMP_CACHE_BYTES. Replace all such uses of memblock APIs with the 'align' parameter explicitly set to SMP_CACHE_BYTES and stop implicit alignment assignment in the memblock internal allocation functions. For the case when memblock APIs are used via helper functions, e.g. like iommu_arena_new_node() in Alpha, the helper functions were detected with Coccinelle's help and then manually examined and updated where appropriate. The direct memblock APIs users were updated using the semantic patch below: @@ expression size, min_addr, max_addr, nid; @@ ( | - memblock_alloc_try_nid_raw(size, 0, min_addr, max_addr, nid) + memblock_alloc_try_nid_raw(size, SMP_CACHE_BYTES, min_addr, max_addr, nid) | - memblock_alloc_try_nid_nopanic(size, 0, min_addr, max_addr, nid) + memblock_alloc_try_nid_nopanic(size, SMP_CACHE_BYTES, min_addr, max_addr, nid) | - memblock_alloc_try_nid(size, 0, min_addr, max_addr, nid) + memblock_alloc_try_nid(size, SMP_CACHE_BYTES, min_addr, max_addr, nid) | - memblock_alloc(size, 0) + memblock_alloc(size, SMP_CACHE_BYTES) | - memblock_alloc_raw(size, 0) + memblock_alloc_raw(size, SMP_CACHE_BYTES) | - memblock_alloc_from(size, 0, min_addr) + memblock_alloc_from(size, SMP_CACHE_BYTES, min_addr) | - memblock_alloc_nopanic(size, 0) + memblock_alloc_nopanic(size, SMP_CACHE_BYTES) | - memblock_alloc_low(size, 0) + memblock_alloc_low(size, SMP_CACHE_BYTES) | - memblock_alloc_low_nopanic(size, 0) + memblock_alloc_low_nopanic(size, SMP_CACHE_BYTES) | - memblock_alloc_from_nopanic(size, 0, min_addr) + memblock_alloc_from_nopanic(size, SMP_CACHE_BYTES, min_addr) | - memblock_alloc_node(size, 0, nid) + memblock_alloc_node(size, SMP_CACHE_BYTES, nid) ) Suggested-by: Michal Hocko Signed-off-by: Mike Rapoport --- arch/alpha/kernel/core_apecs.c| 3 ++- arch/alpha/kernel/core_lca.c | 3 ++- arch/alpha/kernel/core_marvel.c | 4 ++-- arch/alpha/kernel/core_mcpcia.c | 6 +++-- arch/alpha/kernel/core_t2.c | 2 +- arch/alpha/kernel/core_titan.c| 6 +++-- arch/alpha/kernel/core_tsunami.c | 6 +++-- arch/alpha/kernel/core_wildfire.c | 6 +++-- arch/alpha/kernel/pci-noop.c | 4 ++-- arch/alpha/kernel/pci.c | 4 ++-- arch/alpha/kernel/pci_iommu.c | 4 ++-- arch/arm/kernel/setup.c | 4 ++-- arch/arm/mach-omap2/omap_hwmod.c | 8 --- arch/arm64/kernel/setup.c | 2 +- arch/ia64/kernel/mca.c| 4 ++-- arch/ia64/mm/tlb.c| 6 +++-- arch/ia64/sn/kernel/io_common.c | 4 +++- arch/ia64/sn/kernel/setup.c | 5 ++-- arch/m68k/sun3/sun3dvma.c | 2 +- arch/microblaze/mm/init.c | 2 +- arch/mips/kernel/setup.c | 2 +- arch/powerpc/kernel/pci_32.c | 3 ++- arch/powerpc/lib/alloc.c | 2 +- arch/powerpc/mm/mmu_context_nohash.c | 7 +++--- arch/powerpc/platforms/powermac/nvram.c | 2 +- arch/powerpc/platforms/powernv/pci-ioda.c | 6 ++--- arch/powerpc/sysdev/msi_bitmap.c | 2 +- arch/um/drivers/net_kern.c| 2 +- arch/um/drivers/vector_kern.c | 2 +- arch/um/kernel/initrd.c | 2 +- arch/unicore32/kernel/setup.c | 2 +- arch/x86/kernel/acpi/boot.c | 2 +- arch/x86/kernel/apic/io_apic.c| 2 +- arch/x86/kernel/e820.c| 3 ++- arch/x86/platform/olpc/olpc_dt.c | 2 +- arch/xtensa/platforms/iss/network.c | 2 +- drivers/clk/ti/clk.c | 2 +- drivers/firmware/memmap.c | 3 ++- drivers/macintosh/smu.c | 2 +- drivers/of/of_reserved_mem.c | 1 + include/linux/memblock.h | 3 ++- init/main.c | 13 +++ kernel/power/snapshot.c | 3 ++- lib/cpumask.c | 2 +- mm/memblock.c | 8 --- mm/page_alloc.c | 6 +++-- mm/percpu.c | 39 --- mm/sparse.c | 3 ++- 48 files changed, 118 insertions(+), 95 deletions(-) diff --git a/arch/alpha/kernel/core_apecs.c b/arch/alpha/kernel/core_apecs.c index 1bf3eef..6df765f 100644 --- a/arch/alpha/kernel/core_apecs.c +++ b/arch/alpha/kernel/core_apecs.c @@ -346,7 +346,8 @@ apecs_init_arch(void) * Window 1 is direct access 1GB at 1GB * Window 2 is scatter-gather 8MB at 8MB (for isa) */ - hose->sg_isa = iommu_arena_new(hose, 0x0080, 0x0080, 0); + hose->sg_isa = iommu_arena_new(hose, 0x0080, 0x0080, + SMP_CACHE_BYTES); hose->sg_pci =
