2022-05-25 07:38:58

by Anshuman Khandual

Subject: [PATCH] mm/memory_hotplug: Drop 'reason' argument from check_pfn_span()

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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
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

2022-05-26 05:52:32

by Anshuman Khandual

Subject: Re: [PATCH] mm/memory_hotplug: Drop 'reason' argument from check_pfn_span()

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 <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Anshuman Khandual <[email protected]>
>
> 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.

2022-05-26 18:29:53

by Oscar Salvador

Subject: Re: [PATCH] mm/memory_hotplug: Drop 'reason' argument from check_pfn_span()

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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>

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