[PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[David Hildenbrand]

Now that we always check against a zone, we can stop checking against
the nid, it is implicitly covered by the zone.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/memory_hotplug.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 46b2e056a43f..602f753c662c 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -344,17 +344,14 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
 }
 
 /* find the smallest valid pfn in the range [start_pfn, end_pfn) */
-static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
-				     unsigned long start_pfn,
-				     unsigned long end_pfn)
+static unsigned long find_smallest_section_pfn(struct zone *zone,
+					       unsigned long start_pfn,
+					       unsigned long end_pfn)
 {
 	for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SUBSECTION) {
 		if (unlikely(!pfn_to_online_page(start_pfn)))
 			continue;
 
-		if (unlikely(pfn_to_nid(start_pfn) != nid))
-			continue;
-
 		if (zone != page_zone(pfn_to_page(start_pfn)))
 			continue;
 
@@ -365,9 +362,9 @@ static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
 }
 
 /* find the biggest valid pfn in the range [start_pfn, end_pfn). */
-static unsigned long find_biggest_section_pfn(int nid, struct zone *zone,
-				    unsigned long start_pfn,
-				    unsigned long end_pfn)
+static unsigned long find_biggest_section_pfn(struct zone *zone,
+					      unsigned long start_pfn,
+					      unsigned long end_pfn)
 {
 	unsigned long pfn;
 
@@ -377,9 +374,6 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone,
 		if (unlikely(!pfn_to_online_page(pfn)))
 			continue;
 
-		if (unlikely(pfn_to_nid(pfn) != nid))
-			continue;
-
 		if (zone != page_zone(pfn_to_page(pfn)))
 			continue;
 
@@ -393,7 +387,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
 			     unsigned long end_pfn)
 {
 	unsigned long pfn;
-	int nid = zone_to_nid(zone);
 
 	zone_span_writelock(zone);
 	if (zone->zone_start_pfn == start_pfn) {
@@ -403,7 +396,7 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
 		 * In this case, we find second smallest valid mem_section
 		 * for shrinking zone.
 		 */
-		pfn = find_smallest_section_pfn(nid, zone, end_pfn,
+		pfn = find_smallest_section_pfn(zone, end_pfn,
 						zone_end_pfn(zone));
 		if (pfn) {
 			zone->spanned_pages = zone_end_pfn(zone) - pfn;
@@ -419,7 +412,7 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
 		 * In this case, we find second biggest valid mem_section for
 		 * shrinking zone.
 		 */
-		pfn = find_biggest_section_pfn(nid, zone, zone->zone_start_pfn,
+		pfn = find_biggest_section_pfn(zone, zone->zone_start_pfn,
 					       start_pfn);
 		if (pfn)
 			zone->spanned_pages = pfn - zone->zone_start_pfn + 1;
-- 
2.21.0

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Qian Cai]

> On Nov 27, 2019, at 12:42 PM, David Hildenbrand <david@redhat.com> wrote:
> 
> Now that we always check against a zone, we can stop checking against
> the nid, it is implicitly covered by the zone.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> mm/memory_hotplug.c | 23 ++++++++---------------
> 1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 46b2e056a43f..602f753c662c 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -344,17 +344,14 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> }
> 
> /* find the smallest valid pfn in the range [start_pfn, end_pfn) */
> -static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
> -                     unsigned long start_pfn,
> -                     unsigned long end_pfn)
> +static unsigned long find_smallest_section_pfn(struct zone *zone,
> +                           unsigned long start_pfn,
> +                           unsigned long end_pfn)
> {
>    for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SUBSECTION) {
>        if (unlikely(!pfn_to_online_page(start_pfn)))
>            continue;
> 
> -        if (unlikely(pfn_to_nid(start_pfn) != nid))
> -            continue;

Are you sure? I thought this is to check against machines with odd layouts, no? 

			/*
			 * Nodes's pfns can be overlapping.
			 * We know some arch can have a nodes layout such as
			 * -------------pfn-------------->
			 * N0 | N1 | N2 | N0 | N1 | N2|....
			 */

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[David Hildenbrand]

> Am 27.11.2019 um 20:03 schrieb Qian Cai <cai@lca.pw>:
> 
> 
> 
>> On Nov 27, 2019, at 12:42 PM, David Hildenbrand <david@redhat.com> wrote:
>> 
>> Now that we always check against a zone, we can stop checking against
>> the nid, it is implicitly covered by the zone.
>> 
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Oscar Salvador <osalvador@suse.de>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> mm/memory_hotplug.c | 23 ++++++++---------------
>> 1 file changed, 8 insertions(+), 15 deletions(-)
>> 
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 46b2e056a43f..602f753c662c 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -344,17 +344,14 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>> }
>> 
>> /* find the smallest valid pfn in the range [start_pfn, end_pfn) */
>> -static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
>> -                     unsigned long start_pfn,
>> -                     unsigned long end_pfn)
>> +static unsigned long find_smallest_section_pfn(struct zone *zone,
>> +                           unsigned long start_pfn,
>> +                           unsigned long end_pfn)
>> {
>>   for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SUBSECTION) {
>>       if (unlikely(!pfn_to_online_page(start_pfn)))
>>           continue;
>> 
>> -        if (unlikely(pfn_to_nid(start_pfn) != nid))
>> -            continue;
> 
> Are you sure? I thought this is to check against machines with odd layouts, no? 

The zone pointer is unique for every node. (in contrast to the zone index).

Thanks!

> 
>            /*
>             * Nodes's pfns can be overlapping.
>             * We know some arch can have a nodes layout such as
>             * -------------pfn-------------->
>             * N0 | N1 | N2 | N0 | N1 | N2|....
>             */
> 

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Qian Cai]

> On Nov 27, 2019, at 2:06 PM, David Hildenbrand <david@redhat.com> wrote:
> 
> The zone pointer is unique for every node. (in contrast to the zone index).

I am not sure if it is worth optimizing there. The existing nid check looks quite straight-forward and cheap.

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[David Hildenbrand]

> Am 27.11.2019 um 20:37 schrieb Qian Cai <cai@lca.pw>:
> 
> 
> 
>> On Nov 27, 2019, at 2:06 PM, David Hildenbrand <david@redhat.com> wrote:
>> 
>> The zone pointer is unique for every node. (in contrast to the zone index).
> 
> I am not sure if it is worth optimizing there. The existing nid check looks quite straight-forward and cheap.

I understand but strongly dislike your attitude towards code changes ;)

> 

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[David Hildenbrand]

On 27.11.19 20:52, David Hildenbrand wrote:
> 
> 
>> Am 27.11.2019 um 20:37 schrieb Qian Cai <cai@lca.pw>:
>>
>> 
>>
>>> On Nov 27, 2019, at 2:06 PM, David Hildenbrand <david@redhat.com> wrote:
>>>
>>> The zone pointer is unique for every node. (in contrast to the zone index).
>>
>> I am not sure if it is worth optimizing there. The existing nid check looks quite straight-forward and cheap.
> 
> I understand but strongly dislike your attitude towards code changes ;)
> 

I think that came out wrong, let me rephrase:

This is not a performance optimization but a cleanup. Once you
understood how zone pointers work, you immediately see why the nid
checks are only here for legacy reasons (see "mm/memory_hotplug: we
always have a zone in find_(smallest|biggest)_section_pfn" in linux-next).

(I am a friend of cleaning up and refactoring code to make it easier to
understand, maintain and extend. I was assuming your mentality is to
rather keeping code changes minimal if there is a chance to break things
- I'm sorry if that assumption was wrong.)

-- 
Thanks,

David / dhildenb

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Qian Cai]

> On Nov 27, 2019, at 3:49 PM, David Hildenbrand <david@redhat.com> wrote:
> 
> (I am a friend of cleaning up and refactoring code to make it easier to
> understand, maintain and extend. I was assuming your mentality is to
> rather keeping code changes minimal if there is a chance to break things
> - I'm sorry if that assumption was wrong.)

Yes, I tested linux-next everyday and saw enough of those cleanup efforts ends up introducing regressions. It is almost every day or two I had to investigate at least one regression and pick the suckers out even though my testing is only focus on MM and friends. However, I do agree if there are worthy cleanup and refactoring but those tiny ones make me uncomfortable. See, I am just trying to save a real vacation for a few weeks in the future, but given the current situation, I’ll need to give up on this project [1] at all because I just have no courage to debug all the regressions there once back.

[1] https://github.com/cailca/linux-mm

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[David Hildenbrand]

On 27.11.19 23:56, Qian Cai wrote:
> 
> 
>> On Nov 27, 2019, at 3:49 PM, David Hildenbrand <david@redhat.com> wrote:
>>
>> (I am a friend of cleaning up and refactoring code to make it easier to
>> understand, maintain and extend. I was assuming your mentality is to
>> rather keeping code changes minimal if there is a chance to break things
>> - I'm sorry if that assumption was wrong.)
> 
> Yes, I tested linux-next everyday and saw enough of those cleanup efforts ends up introducing regressions. It is almost every day or two I had to investigate at least one regression and pick the suckers out even though my testing is only focus on MM and friends. However, I do agree if there are worthy cleanup and refactoring but those tiny ones make me uncomfortable. See, I am just trying to save a real vacation for a few weeks in the future, but given the current situation, I’ll need to give up on this project [1] at all because I just have no courage to debug all the regressions there once back.
> 
> [1] https://github.com/cailca/linux-mm
> 

I'm sorry to say but one of the main reasons we have linux-next for is
to find BUGs early, before they go upstream. It is a way of giving
patches *more* testing. Yes, you are doing to dirty work (which is
highly appreciated btw) by debugging all that crap, and I can understand
how that can be frustrating.

But believe me, the world won't end if your on vacation for a couple of
weeks, even though some BUGs could sneak in ... e.g., lately I try to
review as much as I can on the MM list (and Michal is steadily watching
out as well).

The solution to your problem is more review and testing, really. E.g.,
I'd be very happy if other developers would test their patches more
thoroughly and if there would be more review activity on the MM list in
general (my patches barely get any review ... and I sent a lot of fixes
lately).

As soon as we stop touching our code because we are afraid of BUGs, we
lost the battle against an unmaintainable code base.


BTW: [1] mentions "unbalanced software development culture with regard
to quality vs quantity that supplies an endless stream of bugs". I don't
agree to this statement. There will *always* be an endless stream of
BUGs - and most of them come from new features and performance
improvements IMHO. To me, cleanups and refactorings are important tools
 to improve the software quality (and reduce the code size). All we can
do is try to minimize the number of BUGs - e.g., via more code review,
manual testing, automatic testing, and by actually understanding the
code. Cleanups/refactorings can even fix undiscovered BUGs (e.g., latest
example is [2])

[2]
https://lore.kernel.org/linux-mm/b2e31976-b07d-11e6-f806-f13f4619be4d@redhat.com/

-- 
Thanks,

David / dhildenb

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Qian Cai]

> On Nov 28, 2019, at 3:46 AM, David Hildenbrand <david@redhat.com> wrote:
> 
> I'm sorry to say but one of the main reasons we have linux-next for is
> to find BUGs early, before they go upstream. It is a way of giving
> patches *more* testing. Yes, you are doing to dirty work (which is
> highly appreciated btw) by debugging all that crap, and I can understand
> how that can be frustrating.

It is already an expensive development practice if developers need to rely on someone else to figure out their own bugs in linux-next. linux-next is for integration testing, but majority of those regressions I had to deal with nowadays have nothing to do with integration, i.e., interaction with other subsystems.

> 
> But believe me, the world won't end if your on vacation for a couple of
> weeks, even though some BUGs could sneak in ... e.g., lately I try to
> review as much as I can on the MM list (and Michal is steadily watching
> out as well).

Sure, the world will still be running, but good luck on solely rely on reviewing with bare eyes before merging.

> 
> The solution to your problem is more review and testing, really. E.g.,
> I'd be very happy if other developers would test their patches more
> thoroughly and if there would be more review activity on the MM list in
> general (my patches barely get any review ... and I sent a lot of fixes
> lately).

Of course, that helps but it is a culture that very difficult to change now. How many times I saw even high-profile developers proudly sent out patches labeled “no testing” explicitly and implicitly ?

> 
> As soon as we stop touching our code because we are afraid of BUGs, we
> lost the battle against an unmaintainable code base.

Your generalizations of things make me sorrow.

> 
> BTW: [1] mentions "unbalanced software development culture with regard
> to quality vs quantity that supplies an endless stream of bugs". I don't
> agree to this statement. There will *always* be an endless stream of
> BUGs - and most of them come from new features and performance
> improvements IMHO. To me, cleanups and refactorings are important tools
> to improve the software quality (and reduce the code size). All we can
> do is try to minimize the number of BUGs - e.g., via more code review,
> manual testing, automatic testing, and by actually understanding the
> code. Cleanups/refactorings can even fix undiscovered BUGs (e.g., latest
> example is [2])

Surely, most of people probably don’t care about those endless bugs because Linux is a monopoly in data center and open source and it is always like this since Linux was born as a hobby project.

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[David Hildenbrand]

>>
>> But believe me, the world won't end if your on vacation for a couple of
>> weeks, even though some BUGs could sneak in ... e.g., lately I try to
>> review as much as I can on the MM list (and Michal is steadily watching
>> out as well).
> 
> Sure, the world will still be running, but good luck on solely rely on reviewing with bare eyes before merging.

That's why we have linux-next and plenty of people playing with it
(including you and me for example).

> 
>>
>> The solution to your problem is more review and testing, really. E.g.,
>> I'd be very happy if other developers would test their patches more
>> thoroughly and if there would be more review activity on the MM list in
>> general (my patches barely get any review ... and I sent a lot of fixes
>> lately).
> 
> Of course, that helps but it is a culture that very difficult to change now. How many times I saw even high-profile developers proudly sent out patches labeled “no testing” explicitly and implicitly ?
>

That is a different story, and I do agree that we should be more careful
with such things. Personally, I test whatever I send upstream - as long
as there is a way for me to test. We can only change this culture slowly
- but frankly speaking "no small cleanups" is just the wrong approach to
this problem.

[...]

>> BTW: [1] mentions "unbalanced software development culture with regard
>> to quality vs quantity that supplies an endless stream of bugs". I don't
>> agree to this statement. There will *always* be an endless stream of
>> BUGs - and most of them come from new features and performance
>> improvements IMHO. To me, cleanups and refactorings are important tools
>> to improve the software quality (and reduce the code size). All we can
>> do is try to minimize the number of BUGs - e.g., via more code review,
>> manual testing, automatic testing, and by actually understanding the
>> code. Cleanups/refactorings can even fix undiscovered BUGs (e.g., latest
>> example is [2])
> 
> Surely, most of people probably don’t care about those endless bugs because Linux is a monopoly in data center and open source and it is always like this since Linux was born as a hobby project.
> 

Well, working for a distribution I do care a lot :)

Again, your work is highly appreciated, but you are trying to use a
questionable approach (limit code changes) to solve a fundamental
problem (people not testing stuff, lack of review).

-- 
Thanks,

David / dhildenb

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Qian Cai]

> On Nov 28, 2019, at 9:03 AM, David Hildenbrand <david@redhat.com> wrote:
> 
> That's why we have linux-next and plenty of people playing with it
> (including you and me for example).

As mentioned, it is an expensive development practice. Once a patch was merged into linux-next, it becomes someone else’s problems because if nobody flags it as problematic, all it needs is a good eye review and some time before it gets merged into mainline eventually.

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Michal Hocko]

On Thu 28-11-19 09:30:29, Qian Cai wrote:
> 
> 
> > On Nov 28, 2019, at 9:03 AM, David Hildenbrand <david@redhat.com> wrote:
> > 
> > That's why we have linux-next and plenty of people playing with it
> > (including you and me for example).
> 
> As mentioned, it is an expensive development practice. Once a patch
> was merged into linux-next, it becomes someone else’s problems
> because if nobody flags it as problematic, all it needs is a good eye
> review and some time before it gets merged into mainline eventually.

I would tend to agree. linux-next shouldn't be considered a low bar
target. Things should be reviewed before showing up there. There are
obviously some exceptions, as always, but it shouldn't be over used.

I wish MM patches would be applied to mmotm (and linux-next) more
conservatively.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[David Hildenbrand]

On 28.11.19 15:42, Michal Hocko wrote:
> On Thu 28-11-19 09:30:29, Qian Cai wrote:
>>
>>
>>> On Nov 28, 2019, at 9:03 AM, David Hildenbrand <david@redhat.com> wrote:
>>>
>>> That's why we have linux-next and plenty of people playing with it
>>> (including you and me for example).
>>
>> As mentioned, it is an expensive development practice. Once a patch
>> was merged into linux-next, it becomes someone else’s problems
>> because if nobody flags it as problematic, all it needs is a good eye
>> review and some time before it gets merged into mainline eventually.
> 
> I would tend to agree. linux-next shouldn't be considered a low bar
> target. Things should be reviewed before showing up there. There are
> obviously some exceptions, as always, but it shouldn't be over used.
> 
> I wish MM patches would be applied to mmotm (and linux-next) more
> conservatively.

I also agree that it should not be used for basic functional/compile
tests (I said "It is a way of giving patches *more* testing."). It
should not be the only place to test stuff (especially to let somebody
else do it).

However, sometimes we really have to get additional test coverage via
linux-next, especially for weird archs/configurations/setups.

... and if we don't have enough reviewers, it's really hard to get stuff
upstream.

I wish MM patches would get reviewed more thoroughly.

(If we all make a wish, maybe Santa Clause will listen ;) )

-- 
Thanks,

David / dhildenb

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Qian Cai]

> On Nov 28, 2019, at 9:52 AM, David Hildenbrand <david@redhat.com> wrote:
> 
> I also agree that it should not be used for basic functional/compile
> tests (I said "It is a way of giving patches *more* testing."). It
> should not be the only place to test stuff (especially to let somebody
> else do it).
> 
> However, sometimes we really have to get additional test coverage via
> linux-next, especially for weird archs/configurations/setups.
> 
> ... and if we don't have enough reviewers, it's really hard to get stuff
> upstream.
> 
> I wish MM patches would get reviewed more thoroughly.
> 
> (If we all make a wish, maybe Santa Clause will listen ;) )

What I don’t understand is that we have an policy prohibiting code churn like code optimization in cold path, but allow those micro cleanup of code. Those cleanup also tend to be unregulated and is subject to personal tastes (for example, your CS teachers may have a different taste from mine). I can understand developers want to have fun, but perhaps there are other playground areas that worth taking more risk? 

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[David Hildenbrand]

On 28.11.19 16:29, Qian Cai wrote:
> 
> 
>> On Nov 28, 2019, at 9:52 AM, David Hildenbrand <david@redhat.com> wrote:
>>
>> I also agree that it should not be used for basic functional/compile
>> tests (I said "It is a way of giving patches *more* testing."). It
>> should not be the only place to test stuff (especially to let somebody
>> else do it).
>>
>> However, sometimes we really have to get additional test coverage via
>> linux-next, especially for weird archs/configurations/setups.
>>
>> ... and if we don't have enough reviewers, it's really hard to get stuff
>> upstream.
>>
>> I wish MM patches would get reviewed more thoroughly.
>>
>> (If we all make a wish, maybe Santa Clause will listen ;) )
> 
> What I don’t understand is that we have an policy prohibiting code churn like code optimization in cold path, but allow those micro cleanup of code. Those cleanup also tend to be unregulated and is subject to personal tastes (for example, your CS teachers may have a different taste from mine). I can understand developers want to have fun, but perhaps there are other playground areas that worth taking more risk? 
> 

I said all I had to say about this topic, especially also why cleanups
and refactorings are important. No, you won't change the way I (and most
other people here) work, sorry.

-- 
Thanks,

David / dhildenb

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Qian Cai]

> On Nov 28, 2019, at 10:31 AM, David Hildenbrand <david@redhat.com> wrote:
> 
> I said all I had to say about this topic, especially also why cleanups
> and refactorings are important. No, you won't change the way I (and most
> other people here) work, sorry.

Yes, I don’t think I am going to change anybody. Also, it is useless to only improve the mindset of a few subsystems like MM because other subsystems will affect the whole greatly. The good news is that I probably never be out of the job.

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Michal Hocko]

On Thu 28-11-19 10:29:24, Qian Cai wrote:
> What I don’t understand is that we have an policy prohibiting code
> churn like code optimization in cold path, but allow those micro
> cleanup of code. Those cleanup also tend to be unregulated and is
> subject to personal tastes (for example, your CS teachers may have a
> different taste from mine).

I tend to push back on cleanups without a clear added value. In this
particular case it is not about aesthetic or a personal feeling about
the code though. Please read my comment with the ack.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Qian Cai]

> On Nov 28, 2019, at 10:46 AM, Michal Hocko <mhocko@kernel.org> wrote:
> 
> I tend to push back on cleanups without a clear added value. In this
> particular case it is not about aesthetic or a personal feeling about
> the code though. Please read my comment with the ack.

I can only encourage you to push back more in general especially for those micro-size cleanups. On the other hand, it could be arbitrary because once you are gone for vacations or some other reasons, it will be inconsistent of reviewing. You are mostly of my first defense those days.

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Michal Hocko]

On Wed 27-11-19 18:41:58, David Hildenbrand wrote:
> Now that we always check against a zone, we can stop checking against
> the nid, it is implicitly covered by the zone.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Signed-off-by: David Hildenbrand <david@redhat.com>

OK, this makes some sense to me. The node really is superfluous and it
doesn't add any clarity. Quite contrary it just brings question why do
we check it as well. If there ever is a need to check for the node then
we have it available in struct zone and that would be much more robust
approach because an accidental mismatch between parameters is ruled out.

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/memory_hotplug.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 46b2e056a43f..602f753c662c 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -344,17 +344,14 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>  }
>  
>  /* find the smallest valid pfn in the range [start_pfn, end_pfn) */
> -static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
> -				     unsigned long start_pfn,
> -				     unsigned long end_pfn)
> +static unsigned long find_smallest_section_pfn(struct zone *zone,
> +					       unsigned long start_pfn,
> +					       unsigned long end_pfn)
>  {
>  	for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SUBSECTION) {
>  		if (unlikely(!pfn_to_online_page(start_pfn)))
>  			continue;
>  
> -		if (unlikely(pfn_to_nid(start_pfn) != nid))
> -			continue;
> -
>  		if (zone != page_zone(pfn_to_page(start_pfn)))
>  			continue;
>  
> @@ -365,9 +362,9 @@ static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
>  }
>  
>  /* find the biggest valid pfn in the range [start_pfn, end_pfn). */
> -static unsigned long find_biggest_section_pfn(int nid, struct zone *zone,
> -				    unsigned long start_pfn,
> -				    unsigned long end_pfn)
> +static unsigned long find_biggest_section_pfn(struct zone *zone,
> +					      unsigned long start_pfn,
> +					      unsigned long end_pfn)
>  {
>  	unsigned long pfn;
>  
> @@ -377,9 +374,6 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone,
>  		if (unlikely(!pfn_to_online_page(pfn)))
>  			continue;
>  
> -		if (unlikely(pfn_to_nid(pfn) != nid))
> -			continue;
> -
>  		if (zone != page_zone(pfn_to_page(pfn)))
>  			continue;
>  
> @@ -393,7 +387,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>  			     unsigned long end_pfn)
>  {
>  	unsigned long pfn;
> -	int nid = zone_to_nid(zone);
>  
>  	zone_span_writelock(zone);
>  	if (zone->zone_start_pfn == start_pfn) {
> @@ -403,7 +396,7 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>  		 * In this case, we find second smallest valid mem_section
>  		 * for shrinking zone.
>  		 */
> -		pfn = find_smallest_section_pfn(nid, zone, end_pfn,
> +		pfn = find_smallest_section_pfn(zone, end_pfn,
>  						zone_end_pfn(zone));
>  		if (pfn) {
>  			zone->spanned_pages = zone_end_pfn(zone) - pfn;
> @@ -419,7 +412,7 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>  		 * In this case, we find second biggest valid mem_section for
>  		 * shrinking zone.
>  		 */
> -		pfn = find_biggest_section_pfn(nid, zone, zone->zone_start_pfn,
> +		pfn = find_biggest_section_pfn(zone, zone->zone_start_pfn,
>  					       start_pfn);
>  		if (pfn)
>  			zone->spanned_pages = pfn - zone->zone_start_pfn + 1;
> -- 
> 2.21.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm/memory_hotplug: don't check the nid in find_(smallest|biggest)_section_pfn

[Oscar Salvador]

On Wed, Nov 27, 2019 at 06:41:58PM +0100, David Hildenbrand wrote:
> Now that we always check against a zone, we can stop checking against
> the nid, it is implicitly covered by the zone.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Oscar Salvador <osalvador@suse.de>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Maybe the check was in place to play against the "assumption" that a zone can
span multiple nodes.
Hotplug code was full of those hardcoded assumtions (like working with holes and whatnot).

Anyway, this looks the right thing to do, and thanks for the previous
fixes/cleanups.

Reviewed-by: Oscar Salvador <osalvador@suse.de>

> ---
>  mm/memory_hotplug.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 46b2e056a43f..602f753c662c 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -344,17 +344,14 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>  }
>  
>  /* find the smallest valid pfn in the range [start_pfn, end_pfn) */
> -static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
> -				     unsigned long start_pfn,
> -				     unsigned long end_pfn)
> +static unsigned long find_smallest_section_pfn(struct zone *zone,
> +					       unsigned long start_pfn,
> +					       unsigned long end_pfn)
>  {
>  	for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SUBSECTION) {
>  		if (unlikely(!pfn_to_online_page(start_pfn)))
>  			continue;
>  
> -		if (unlikely(pfn_to_nid(start_pfn) != nid))
> -			continue;
> -
>  		if (zone != page_zone(pfn_to_page(start_pfn)))
>  			continue;
>  
> @@ -365,9 +362,9 @@ static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
>  }
>  
>  /* find the biggest valid pfn in the range [start_pfn, end_pfn). */
> -static unsigned long find_biggest_section_pfn(int nid, struct zone *zone,
> -				    unsigned long start_pfn,
> -				    unsigned long end_pfn)
> +static unsigned long find_biggest_section_pfn(struct zone *zone,
> +					      unsigned long start_pfn,
> +					      unsigned long end_pfn)
>  {
>  	unsigned long pfn;
>  
> @@ -377,9 +374,6 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone,
>  		if (unlikely(!pfn_to_online_page(pfn)))
>  			continue;
>  
> -		if (unlikely(pfn_to_nid(pfn) != nid))
> -			continue;
> -
>  		if (zone != page_zone(pfn_to_page(pfn)))
>  			continue;
>  
> @@ -393,7 +387,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>  			     unsigned long end_pfn)
>  {
>  	unsigned long pfn;
> -	int nid = zone_to_nid(zone);
>  
>  	zone_span_writelock(zone);
>  	if (zone->zone_start_pfn == start_pfn) {
> @@ -403,7 +396,7 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>  		 * In this case, we find second smallest valid mem_section
>  		 * for shrinking zone.
>  		 */
> -		pfn = find_smallest_section_pfn(nid, zone, end_pfn,
> +		pfn = find_smallest_section_pfn(zone, end_pfn,
>  						zone_end_pfn(zone));
>  		if (pfn) {
>  			zone->spanned_pages = zone_end_pfn(zone) - pfn;
> @@ -419,7 +412,7 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>  		 * In this case, we find second biggest valid mem_section for
>  		 * shrinking zone.
>  		 */
> -		pfn = find_biggest_section_pfn(nid, zone, zone->zone_start_pfn,
> +		pfn = find_biggest_section_pfn(zone, zone->zone_start_pfn,
>  					       start_pfn);
>  		if (pfn)
>  			zone->spanned_pages = pfn - zone->zone_start_pfn + 1;
> -- 
> 2.21.0
> 

-- 
Oscar Salvador
SUSE L3