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

[David Hildenbrand <david@redhat.com>]

On 12.07.23 22:07, John Hubbard wrote:
> On 7/11/23 09:09, David Hildenbrand wrote:
> ...
>>>> Can we make that a __weak function instead?
>>>
>>> We can. It is confusing because we do have these two patterns within the kernel where we use
>>>
>>> #ifndef x
>>> #endif
>>>
>>> vs
>>>
>>> __weak x
>>>
>>> What is the recommended way to override ? I have mostly been using #ifndef for most of the arch overrides till now.
>>>
>>
>> I think when placing the implementation in a C file, it's __weak. But don't ask me :)
>>
>> We do this already for arch_get_mappable_range() in mm/memory_hotplug.c and IMHO it looks quite nice.
>>
> 
> It does look nice. I always forget which parts are supposed to be
> __weak, so I went to check Documentation/ , and it was quite
> entertaining. There are only two search hits: one trivial reference in
> Documentation/conf.py, and the other in checkpatch.rst, which says:
> 
>     **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.

-- 
Cheers,

David / dhildenb