* [PATCH] mm/memory_hotplug: Drop 'reason' argument from check_pfn_span()
@ 2022-05-25 3:39 Anshuman Khandual
2022-05-25 8:25 ` Oscar Salvador
0 siblings, 1 reply; 3+ messages in thread
From: Anshuman Khandual @ 2022-05-25 3:39 UTC (permalink / raw)
To: linux-mm
Cc: Anshuman Khandual, David Hildenbrand, Andrew Morton, linux-kernel
In check_pfn_span(), a 'reason' string is being used to recreate the caller
function name, while printing the warning message. It is really unnecessary
as the warning message could just be printed inside the caller depending on
the return code. Currentlyy there are just two callers for check_pfn_span()
i.e __add_pages() and __remove_pages(). Let's clean this up.
Cc: David Hildenbrand <david@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
mm/memory_hotplug.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 416b38ca8def..9b3d7295ef93 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -220,8 +220,7 @@ static void release_memory_resource(struct resource *res)
kfree(res);
}
-static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
- const char *reason)
+static int check_pfn_span(unsigned long pfn, unsigned long nr_pages)
{
/*
* Disallow all operations smaller than a sub-section and only
@@ -238,12 +237,8 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
min_align = PAGES_PER_SUBSECTION;
else
min_align = PAGES_PER_SECTION;
- if (!IS_ALIGNED(pfn, min_align)
- || !IS_ALIGNED(nr_pages, min_align)) {
- WARN(1, "Misaligned __%s_pages start: %#lx end: #%lx\n",
- reason, pfn, pfn + nr_pages - 1);
+ if (!IS_ALIGNED(pfn, min_align) || !IS_ALIGNED(nr_pages, min_align))
return -EINVAL;
- }
return 0;
}
@@ -320,9 +315,11 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
altmap->alloc = 0;
}
- err = check_pfn_span(pfn, nr_pages, "add");
- if (err)
+ err = check_pfn_span(pfn, nr_pages);
+ if (err) {
+ WARN(1, "Misaligned %s start: %#lx end: #%lx\n", __func__, pfn, pfn + nr_pages - 1);
return err;
+ }
for (; pfn < end_pfn; pfn += cur_nr_pages) {
/* Select all remaining pages up to the next section boundary */
@@ -518,8 +515,10 @@ void __remove_pages(unsigned long pfn, unsigned long nr_pages,
map_offset = vmem_altmap_offset(altmap);
- if (check_pfn_span(pfn, nr_pages, "remove"))
+ if (check_pfn_span(pfn, nr_pages)) {
+ WARN(1, "Misaligned %s start: %#lx end: #%lx\n", __func__, pfn, pfn + nr_pages - 1);
return;
+ }
for (; pfn < end_pfn; pfn += cur_nr_pages) {
cond_resched();
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] mm/memory_hotplug: Drop 'reason' argument from check_pfn_span()
2022-05-25 3:39 [PATCH] mm/memory_hotplug: Drop 'reason' argument from check_pfn_span() Anshuman Khandual
@ 2022-05-25 8:25 ` Oscar Salvador
2022-05-25 9:38 ` Anshuman Khandual
0 siblings, 1 reply; 3+ messages in thread
From: Oscar Salvador @ 2022-05-25 8:25 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-mm, David Hildenbrand, Andrew Morton, linux-kernel
On Wed, May 25, 2022 at 09:09:09AM +0530, Anshuman Khandual wrote:
> In check_pfn_span(), a 'reason' string is being used to recreate the caller
> function name, while printing the warning message. It is really unnecessary
> as the warning message could just be printed inside the caller depending on
> the return code. Currentlyy there are just two callers for check_pfn_span()
Currently
> i.e __add_pages() and __remove_pages(). Let's clean this up.
>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
One could argue if this is really a cleanup.
I kind of agree that the "reason" thingy is a bit shaky, but instead of having a
single place where we call WARN(), we now do have two.
> ---
> mm/memory_hotplug.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 416b38ca8def..9b3d7295ef93 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -220,8 +220,7 @@ static void release_memory_resource(struct resource *res)
> kfree(res);
> }
>
> -static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
> - const char *reason)
> +static int check_pfn_span(unsigned long pfn, unsigned long nr_pages)
> {
> /*
> * Disallow all operations smaller than a sub-section and only
> @@ -238,12 +237,8 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
> min_align = PAGES_PER_SUBSECTION;
> else
> min_align = PAGES_PER_SECTION;
> - if (!IS_ALIGNED(pfn, min_align)
> - || !IS_ALIGNED(nr_pages, min_align)) {
> - WARN(1, "Misaligned __%s_pages start: %#lx end: #%lx\n",
> - reason, pfn, pfn + nr_pages - 1);
> + if (!IS_ALIGNED(pfn, min_align) || !IS_ALIGNED(nr_pages, min_align))
> return -EINVAL;
> - }
> return 0;
> }
>
> @@ -320,9 +315,11 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> altmap->alloc = 0;
> }
>
> - err = check_pfn_span(pfn, nr_pages, "add");
> - if (err)
> + err = check_pfn_span(pfn, nr_pages);
> + if (err) {
> + WARN(1, "Misaligned %s start: %#lx end: #%lx\n", __func__, pfn, pfn + nr_pages - 1);
> return err;
> + }
If you want to further clean this up, I would just do
if (check_pfn_span()) {
WARN(....)
return -EINVAL;
}
here as we do in __remove_pages(). check_pfn_span() can either return 0 or -EINVAL,
so I think it is fine.
--
Oscar Salvador
SUSE Labs
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] mm/memory_hotplug: Drop 'reason' argument from check_pfn_span()
2022-05-25 8:25 ` Oscar Salvador
@ 2022-05-25 9:38 ` Anshuman Khandual
0 siblings, 0 replies; 3+ messages in thread
From: Anshuman Khandual @ 2022-05-25 9:38 UTC (permalink / raw)
To: Oscar Salvador; +Cc: linux-mm, David Hildenbrand, Andrew Morton, linux-kernel
On 5/25/22 13:55, Oscar Salvador wrote:
> On Wed, May 25, 2022 at 09:09:09AM +0530, Anshuman Khandual wrote:
>> In check_pfn_span(), a 'reason' string is being used to recreate the caller
>> function name, while printing the warning message. It is really unnecessary
>> as the warning message could just be printed inside the caller depending on
>> the return code. Currentlyy there are just two callers for check_pfn_span()
> Currently
Ahh, will fix.
>> i.e __add_pages() and __remove_pages(). Let's clean this up.
>>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>
> One could argue if this is really a cleanup.
> I kind of agree that the "reason" thingy is a bit shaky, but instead of having a
> single place where we call WARN(), we now do have two.
check_pfn_span() is basically ensuring minimum alignment for both pfn and
nr_pages. Resulting error message when this alignment check does not hold
true, is caller specific than not.
>
>> ---
>> mm/memory_hotplug.c | 19 +++++++++----------
>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 416b38ca8def..9b3d7295ef93 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -220,8 +220,7 @@ static void release_memory_resource(struct resource *res)
>> kfree(res);
>> }
>>
>> -static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>> - const char *reason)
>> +static int check_pfn_span(unsigned long pfn, unsigned long nr_pages)
>> {
>> /*
>> * Disallow all operations smaller than a sub-section and only
>> @@ -238,12 +237,8 @@ static int check_pfn_span(unsigned long pfn, unsigned long nr_pages,
>> min_align = PAGES_PER_SUBSECTION;
>> else
>> min_align = PAGES_PER_SECTION;
>> - if (!IS_ALIGNED(pfn, min_align)
>> - || !IS_ALIGNED(nr_pages, min_align)) {
>> - WARN(1, "Misaligned __%s_pages start: %#lx end: #%lx\n",
>> - reason, pfn, pfn + nr_pages - 1);
>> + if (!IS_ALIGNED(pfn, min_align) || !IS_ALIGNED(nr_pages, min_align))
>> return -EINVAL;
>> - }
>> return 0;
>> }
>>
>> @@ -320,9 +315,11 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>> altmap->alloc = 0;
>> }
>>
>> - err = check_pfn_span(pfn, nr_pages, "add");
>> - if (err)
>> + err = check_pfn_span(pfn, nr_pages);
>> + if (err) {
>> + WARN(1, "Misaligned %s start: %#lx end: #%lx\n", __func__, pfn, pfn + nr_pages - 1);
>> return err;
>> + }
>
> If you want to further clean this up, I would just do
>
> if (check_pfn_span()) {
> WARN(....)
> return -EINVAL;
> }
>
> here as we do in __remove_pages(). check_pfn_span() can either return 0 or -EINVAL,
> so I think it is fine.
Sure, will change.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-25 9:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-25 3:39 [PATCH] mm/memory_hotplug: Drop 'reason' argument from check_pfn_span() Anshuman Khandual
2022-05-25 8:25 ` Oscar Salvador
2022-05-25 9:38 ` Anshuman Khandual
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).