Re: [PATCH v3 3/7] mm/hotplug: Allow architecture to override memmap on memory support check
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
Date: Fri Jul 1 13:04:04 2022 +0530
kexec_file: drop weak attribute from functions
As requested
(http://lkml.kernel.org/r/87ee0q7b92@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@linux.vnet.ibm.com
Link:
https://lkml.kernel.org/r/2cd7ca1fe4d6bb6ca38e3283c717878388ed6788.1656659357.git.naveen.n@linux.vnet.ibm.com
Signed-off-by: Naveen N. Rao
Suggested-by: Eric Biederman
Signed-off-by: Andrew Morton
Signed-off-by: Mimi Zohar
So, in general, it's use seems to be fine (unless some tool actually bails out).
https://lore.kernel.org/all/87ee0q7b92@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
Re: [PATCH v3 3/7] mm/hotplug: Allow architecture to override memmap on memory support check
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
Date: Fri Jul 1 13:04:04 2022 +0530
kexec_file: drop weak attribute from functions
As requested
(http://lkml.kernel.org/r/87ee0q7b92@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@linux.vnet.ibm.com
Link:
https://lkml.kernel.org/r/2cd7ca1fe4d6bb6ca38e3283c717878388ed6788.1656659357.git.naveen.n@linux.vnet.ibm.com
Signed-off-by: Naveen N. Rao
Suggested-by: Eric Biederman
Signed-off-by: Andrew Morton
Signed-off-by: Mimi Zohar
So, in general, it's use seems to be fine (unless some tool actually bails out).
https://lore.kernel.org/all/87ee0q7b92@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
Re: [PATCH v3 3/7] mm/hotplug: Allow architecture to override memmap on memory support check
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.) thanks, -- John Hubbard NVIDIA
Re: [PATCH v3 3/7] mm/hotplug: Allow architecture to override memmap on memory support check
On 11.07.23 18:07, Aneesh Kumar K V wrote: On 7/11/23 4:06 PM, David Hildenbrand wrote: On 11.07.23 06:48, Aneesh Kumar K.V wrote: Some architectures would want different restrictions. Hence add an architecture-specific override. Both the PMD_SIZE check and pageblock alignment check are moved there. Signed-off-by: Aneesh Kumar K.V --- mm/memory_hotplug.c | 17 - 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 1b19462f4e72..07c99b0cc371 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1247,12 +1247,20 @@ static int online_memory_block(struct memory_block *mem, void *arg) return device_online(>dev); } -static bool mhp_supports_memmap_on_memory(unsigned long size) +#ifndef arch_supports_memmap_on_memory 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. -- Cheers, David / dhildenb
Re: [PATCH v3 3/7] mm/hotplug: Allow architecture to override memmap on memory support check
On 7/11/23 4:06 PM, David Hildenbrand wrote:
> On 11.07.23 06:48, Aneesh Kumar K.V wrote:
>> Some architectures would want different restrictions. Hence add an
>> architecture-specific override.
>>
>> Both the PMD_SIZE check and pageblock alignment check are moved there.
>>
>> Signed-off-by: Aneesh Kumar K.V
>> ---
>> mm/memory_hotplug.c | 17 -
>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 1b19462f4e72..07c99b0cc371 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1247,12 +1247,20 @@ static int online_memory_block(struct memory_block
>> *mem, void *arg)
>> return device_online(>dev);
>> }
>> -static bool mhp_supports_memmap_on_memory(unsigned long size)
>> +#ifndef arch_supports_memmap_on_memory
>
> 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.
>
>> +static inline bool arch_supports_memmap_on_memory(unsigned long size)
>> {
>> - unsigned long nr_vmemmap_pages = size / PAGE_SIZE;
>> + unsigned long nr_vmemmap_pages = size >> PAGE_SHIFT;
>> unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
>> unsigned long remaining_size = size - vmemmap_size;
>> + return IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
>> + IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT));
>
> You're moving that check back to mhp_supports_memmap_on_memory() in the
> following patch, where it actually belongs. So this check should stay in
> mhp_supports_memmap_on_memory(). Might be reasonable to factor out the
> vmemmap_size calculation.
>
>
> Also, let's a comment
>
> /*
> * As default, we want the vmemmap to span a complete PMD such that we
> * can map the vmemmap using a single PMD if supported by the
> * architecture.
> */
> return IS_ALIGNED(vmemmap_size, PMD_SIZE);
>
>> +}
>> +#endif
>> +
>> +static bool mhp_supports_memmap_on_memory(unsigned long size)
>> +{
>> /*
>> * Besides having arch support and the feature enabled at runtime, we
>> * need a few more assumptions to hold true:
>> @@ -1280,9 +1288,8 @@ static bool mhp_supports_memmap_on_memory(unsigned
>> long size)
>> * populate a single PMD.
>> */
>> return mhp_memmap_on_memory() &&
>> - size == memory_block_size_bytes() &&
>> - IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
>> - IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT));
>> + size == memory_block_size_bytes() &&
>
> If you keep the properly aligned indentation, this will not be detected as a
> change by git.
>
>> + arch_supports_memmap_on_memory(size);
>> }
>> /*
>
Will update the code based on the above feedback.
-aneesh
Re: [PATCH v3 3/7] mm/hotplug: Allow architecture to override memmap on memory support check
On 11.07.23 06:48, Aneesh Kumar K.V wrote:
Some architectures would want different restrictions. Hence add an
architecture-specific override.
Both the PMD_SIZE check and pageblock alignment check are moved there.
Signed-off-by: Aneesh Kumar K.V
---
mm/memory_hotplug.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 1b19462f4e72..07c99b0cc371 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1247,12 +1247,20 @@ static int online_memory_block(struct memory_block
*mem, void *arg)
return device_online(>dev);
}
-static bool mhp_supports_memmap_on_memory(unsigned long size)
+#ifndef arch_supports_memmap_on_memory
Can we make that a __weak function instead?
+static inline bool arch_supports_memmap_on_memory(unsigned long size)
{
- unsigned long nr_vmemmap_pages = size / PAGE_SIZE;
+ unsigned long nr_vmemmap_pages = size >> PAGE_SHIFT;
unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
unsigned long remaining_size = size - vmemmap_size;
+ return IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
+ IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT));
You're moving that check back to mhp_supports_memmap_on_memory() in the
following patch, where it actually belongs. So this check should stay in
mhp_supports_memmap_on_memory(). Might be reasonable to factor out the
vmemmap_size calculation.
Also, let's a comment
/*
* As default, we want the vmemmap to span a complete PMD such that we
* can map the vmemmap using a single PMD if supported by the
* architecture.
*/
return IS_ALIGNED(vmemmap_size, PMD_SIZE);
+}
+#endif
+
+static bool mhp_supports_memmap_on_memory(unsigned long size)
+{
/*
* Besides having arch support and the feature enabled at runtime, we
* need a few more assumptions to hold true:
@@ -1280,9 +1288,8 @@ static bool mhp_supports_memmap_on_memory(unsigned long
size)
* populate a single PMD.
*/
return mhp_memmap_on_memory() &&
- size == memory_block_size_bytes() &&
- IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
- IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT));
+ size == memory_block_size_bytes() &&
If you keep the properly aligned indentation, this will not be detected
as a change by git.
+ arch_supports_memmap_on_memory(size);
}
/*
--
Cheers,
David / dhildenb
[PATCH v3 3/7] mm/hotplug: Allow architecture to override memmap on memory support check
Some architectures would want different restrictions. Hence add an
architecture-specific override.
Both the PMD_SIZE check and pageblock alignment check are moved there.
Signed-off-by: Aneesh Kumar K.V
---
mm/memory_hotplug.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 1b19462f4e72..07c99b0cc371 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1247,12 +1247,20 @@ static int online_memory_block(struct memory_block
*mem, void *arg)
return device_online(>dev);
}
-static bool mhp_supports_memmap_on_memory(unsigned long size)
+#ifndef arch_supports_memmap_on_memory
+static inline bool arch_supports_memmap_on_memory(unsigned long size)
{
- unsigned long nr_vmemmap_pages = size / PAGE_SIZE;
+ unsigned long nr_vmemmap_pages = size >> PAGE_SHIFT;
unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
unsigned long remaining_size = size - vmemmap_size;
+ return IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
+ IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT));
+}
+#endif
+
+static bool mhp_supports_memmap_on_memory(unsigned long size)
+{
/*
* Besides having arch support and the feature enabled at runtime, we
* need a few more assumptions to hold true:
@@ -1280,9 +1288,8 @@ static bool mhp_supports_memmap_on_memory(unsigned long
size)
* populate a single PMD.
*/
return mhp_memmap_on_memory() &&
- size == memory_block_size_bytes() &&
- IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
- IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT));
+ size == memory_block_size_bytes() &&
+ arch_supports_memmap_on_memory(size);
}
/*
--
2.41.0
