| Message ID | 20230711044834.72809-4-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for memmap on memory feature on ppc64 | expand |
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 <aneesh.kumar@linux.ibm.com> > --- > 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(&mem->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); > } > > /*
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 <aneesh.kumar@linux.ibm.com> >> --- >> 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(&mem->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
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 <aneesh.kumar@linux.ibm.com> >>> --- >>> 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(&mem->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.
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,
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.
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,
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(&mem->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); } /*
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 <aneesh.kumar@linux.ibm.com> --- mm/memory_hotplug.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)