| Message ID | 20220525033910.3781764-1-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memory_hotplug: Drop 'reason' argument from check_pfn_span() | expand |
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.
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.
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();
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(-)