Re: [PATCH v3 3/7] mm/hotplug: Allow architecture to override memmap on memory support check

[John Hubbard <jhubbard@nvidia.com>]

On 7/13/23 02:08, David Hildenbrand wrote:
...
>>     **WEAK_DECLARATION**
>>       Using weak declarations like __attribute__((weak)) or __weak
>>       can have unintended link defects.  Avoid using them.
>>
>> ...which seems deeply out of touch with how arch layers work these days,
>> doesn't it? (This is not rhetorical; I'm asking in order to get an
>> opinion or two on the topic.)
> 
> Did some digging:
> 
> commit 65d9a9a60fd71be964effb2e94747a6acb6e7015
> Author: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Date:   Fri Jul 1 13:04:04 2022 +0530
> 
>      kexec_file: drop weak attribute from functions
>      As requested
>      (http://lkml.kernel.org/r/87ee0q7b92.fsf@email.froward.int.ebiederm.org),
>      this series converts weak functions in kexec to use the #ifdef approach.
>      Quoting the 3e35142ef99fe ("kexec_file: drop weak attribute from
>      arch_kexec_apply_relocations[_add]") changelog:
>      : Since commit d1bcae833b32f1 ("ELF: Don't generate unused section symbols")
>      : [1], binutils (v2.36+) started dropping section symbols that it thought
>      : were unused.  This isn't an issue in general, but with kexec_file.c, gcc
>      : is placing kexec_arch_apply_relocations[_add] into a separate
>      : .text.unlikely section and the section symbol ".text.unlikely" is being
>      : dropped.  Due to this, recordmcount is unable to find a non-weak symbol in
>      : .text.unlikely to generate a relocation record against.
>      This patch (of 2);
>      Drop __weak attribute from functions in kexec_file.c:
>      - arch_kexec_kernel_image_probe()
>      - arch_kimage_file_post_load_cleanup()
>      - arch_kexec_kernel_image_load()
>      - arch_kexec_locate_mem_hole()
>      - arch_kexec_kernel_verify_sig()
>      arch_kexec_kernel_image_load() calls into kexec_image_load_default(), so
>      drop the static attribute for the latter.
>      arch_kexec_kernel_verify_sig() is not overridden by any architecture, so
>      drop the __weak attribute.
>      Link: https://lkml.kernel.org/r/cover.1656659357.git.naveen.n.rao@linux.vnet.ibm.com
>      Link: https://lkml.kernel.org/r/2cd7ca1fe4d6bb6ca38e3283c717878388ed6788.1656659357.git.naveen.n.rao@linux.vnet.ibm.com
>      Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>      Suggested-by: Eric Biederman <ebiederm@xmission.com>
>      Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>      Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> 
> 
> So, in general, it's use seems to be fine (unless some tool actually bails out).
> 
> https://lore.kernel.org/all/87ee0q7b92.fsf@email.froward.int.ebiederm.org/T/#u
> 
> Also mentions that__weak and non __weak variants ending up in the vmlinux. Did not
> check if that's actually (still) the case.
> 

OK, I looked at that commit and the associated discussion, and now have a
pretty clear picture of the preferred ways to do arch overrides.

Thanks for taking the time to look into it, and also to explain it.
Much appreciated!


thanks,
-- 
John Hubbard
NVIDIA