On Sat, Oct 31, 2020 at 2:10 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Oct 29, 2020 at 07:29:45PM -0700, Dan Williams wrote:
> > The core-mm has a default __weak implementation of phys_to_target_node()
> > when the architecture does not override it. That symbol is exported
> > for modules. However, while the export in mm/memory_hotplug.c exported
> > the symbol in the configuration cases of:
>
> Which just means that we should never export weak symbols.  So instead
> of hacking around this introduce a symbol that indicates that the
> architecture impements phys_to_target_node, and don't defined it at all
> in common code for that case.

So I agree with this, but it made me realize that the way
memory_add_physaddr_to_nid() was defined as an exported weak symbol is
similarly broken.

> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -365,9 +365,14 @@ int __weak phys_to_target_node(u64 start)
> >                       start);
> >       return 0;
> >  }
> > +
> > +/* If the arch did not export a strong symbol, export the weak one. */
> > +#ifndef CONFIG_NUMA_KEEP_MEMINFO
> >  EXPORT_SYMBOL_GPL(phys_to_target_node);
> >  #endif
> >
> > +#endif
>
> i.e. move the ifdef to include the actual phys_to_target_node
> definition, and remove the __weak from it here.

The trick is finding an arch common way to pick up the presence of the
phys_to_target_node() override, and it still has the wart of ifdefery
in C code.

I went a bit deeper and moved all the fallback routines to
linux/numa.h and the overrides in all archs that care to
asm/sparsemem.h. Note that asm/sparsemem.h was not my first choice,
but it happened to be where powerpc was already defining its
phys-addr-to-node-id infrastructure, and my first choice header,
asm/numa.h, is not universally available.

The attached patch is going through some kbuild-robot exposure to make
sure I did not break anything else.