* [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic
@ 2020-02-06 5:39 Baoquan He
2020-02-06 6:16 ` Wei Yang
2020-02-06 8:50 ` David Hildenbrand
0 siblings, 2 replies; 10+ messages in thread
From: Baoquan He @ 2020-02-06 5:39 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-mm, akpm, richardw.yang, david, mhocko, osalvador, bhe
In commit 950b68d9178b ("mm/memory_hotplug: don't check for "all holes"
in shrink_zone_span()"), the zone->zone_start_pfn/->spanned_pages
resetting is moved into the if()/else if() branches, if the zone becomes
empty. However the 2nd resetting code block may cause misunderstanding.
So take the resetting codes out of the conditional checking and handling
branches just as the old code does, the find_smallest_section_pfn()and
find_biggest_section_pfn() searching have done the the same thing as
the old for loop did, the logic is kept the same as the old code. This
can remove the possible confusion.
Signed-off-by: Baoquan He <bhe@redhat.com>
---
mm/memory_hotplug.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 089b6c826a9e..475d0d68a32c 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -398,7 +398,7 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone,
static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
unsigned long end_pfn)
{
- unsigned long pfn;
+ unsigned long pfn = zone->zone_start_pfn;
int nid = zone_to_nid(zone);
zone_span_writelock(zone);
@@ -414,9 +414,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
if (pfn) {
zone->spanned_pages = zone_end_pfn(zone) - pfn;
zone->zone_start_pfn = pfn;
- } else {
- zone->zone_start_pfn = 0;
- zone->spanned_pages = 0;
}
} else if (zone_end_pfn(zone) == end_pfn) {
/*
@@ -429,10 +426,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
start_pfn);
if (pfn)
zone->spanned_pages = pfn - zone->zone_start_pfn + 1;
- else {
- zone->zone_start_pfn = 0;
- zone->spanned_pages = 0;
- }
+ }
+
+ if (!pfn) {
+ zone->zone_start_pfn = 0;
+ zone->spanned_pages = 0;
}
zone_span_writeunlock(zone);
}
--
2.17.2
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic 2020-02-06 5:39 [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic Baoquan He @ 2020-02-06 6:16 ` Wei Yang 2020-02-06 8:50 ` David Hildenbrand 1 sibling, 0 replies; 10+ messages in thread From: Wei Yang @ 2020-02-06 6:16 UTC (permalink / raw) To: Baoquan He Cc: linux-kernel, linux-mm, akpm, richardw.yang, david, mhocko, osalvador On Thu, Feb 06, 2020 at 01:39:12PM +0800, Baoquan He wrote: >In commit 950b68d9178b ("mm/memory_hotplug: don't check for "all holes" >in shrink_zone_span()"), the zone->zone_start_pfn/->spanned_pages >resetting is moved into the if()/else if() branches, if the zone becomes >empty. However the 2nd resetting code block may cause misunderstanding. > >So take the resetting codes out of the conditional checking and handling >branches just as the old code does, the find_smallest_section_pfn()and >find_biggest_section_pfn() searching have done the the same thing as >the old for loop did, the logic is kept the same as the old code. This >can remove the possible confusion. > >Signed-off-by: Baoquan He <bhe@redhat.com> Reviewed-by: Wei Yang <richardw.yang@linux.intel.com> >--- > mm/memory_hotplug.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > >diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >index 089b6c826a9e..475d0d68a32c 100644 >--- a/mm/memory_hotplug.c >+++ b/mm/memory_hotplug.c >@@ -398,7 +398,7 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone, > static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > unsigned long end_pfn) > { >- unsigned long pfn; >+ unsigned long pfn = zone->zone_start_pfn; > int nid = zone_to_nid(zone); > > zone_span_writelock(zone); >@@ -414,9 +414,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > if (pfn) { > zone->spanned_pages = zone_end_pfn(zone) - pfn; > zone->zone_start_pfn = pfn; >- } else { >- zone->zone_start_pfn = 0; >- zone->spanned_pages = 0; > } > } else if (zone_end_pfn(zone) == end_pfn) { > /* >@@ -429,10 +426,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > start_pfn); > if (pfn) > zone->spanned_pages = pfn - zone->zone_start_pfn + 1; >- else { >- zone->zone_start_pfn = 0; >- zone->spanned_pages = 0; >- } >+ } >+ >+ if (!pfn) { >+ zone->zone_start_pfn = 0; >+ zone->spanned_pages = 0; > } > zone_span_writeunlock(zone); > } >-- >2.17.2 -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic 2020-02-06 5:39 [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic Baoquan He 2020-02-06 6:16 ` Wei Yang @ 2020-02-06 8:50 ` David Hildenbrand 2020-02-06 9:35 ` Baoquan He 2020-02-06 23:44 ` Wei Yang 1 sibling, 2 replies; 10+ messages in thread From: David Hildenbrand @ 2020-02-06 8:50 UTC (permalink / raw) To: Baoquan He, linux-kernel; +Cc: linux-mm, akpm, richardw.yang, mhocko, osalvador On 06.02.20 06:39, Baoquan He wrote: > In commit 950b68d9178b ("mm/memory_hotplug: don't check for "all holes" > in shrink_zone_span()"), the zone->zone_start_pfn/->spanned_pages > resetting is moved into the if()/else if() branches, if the zone becomes > empty. However the 2nd resetting code block may cause misunderstanding. > > So take the resetting codes out of the conditional checking and handling > branches just as the old code does, the find_smallest_section_pfn()and > find_biggest_section_pfn() searching have done the the same thing as > the old for loop did, the logic is kept the same as the old code. This > can remove the possible confusion. > > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > mm/memory_hotplug.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 089b6c826a9e..475d0d68a32c 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -398,7 +398,7 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone, > static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > unsigned long end_pfn) > { > - unsigned long pfn; > + unsigned long pfn = zone->zone_start_pfn; > int nid = zone_to_nid(zone); > > zone_span_writelock(zone); > @@ -414,9 +414,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > if (pfn) { > zone->spanned_pages = zone_end_pfn(zone) - pfn; > zone->zone_start_pfn = pfn; > - } else { > - zone->zone_start_pfn = 0; > - zone->spanned_pages = 0; > } > } else if (zone_end_pfn(zone) == end_pfn) { > /* > @@ -429,10 +426,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > start_pfn); > if (pfn) > zone->spanned_pages = pfn - zone->zone_start_pfn + 1; > - else { > - zone->zone_start_pfn = 0; > - zone->spanned_pages = 0; > - } > + } > + > + if (!pfn) { > + zone->zone_start_pfn = 0; > + zone->spanned_pages = 0; > } > zone_span_writeunlock(zone); > } > So, what if your zone starts at pfn 0? Unlikely that we can actually offline that, but still it is more confusing than the old code IMHO. Then I prefer to drop the second else case as discussed instead. -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic 2020-02-06 8:50 ` David Hildenbrand @ 2020-02-06 9:35 ` Baoquan He 2020-02-06 9:48 ` David Hildenbrand 2020-02-06 23:44 ` Wei Yang 1 sibling, 1 reply; 10+ messages in thread From: Baoquan He @ 2020-02-06 9:35 UTC (permalink / raw) To: David Hildenbrand Cc: linux-kernel, linux-mm, akpm, richardw.yang, mhocko, osalvador On 02/06/20 at 09:50am, David Hildenbrand wrote: > On 06.02.20 06:39, Baoquan He wrote: > > In commit 950b68d9178b ("mm/memory_hotplug: don't check for "all holes" > > in shrink_zone_span()"), the zone->zone_start_pfn/->spanned_pages > > resetting is moved into the if()/else if() branches, if the zone becomes > > empty. However the 2nd resetting code block may cause misunderstanding. > > > > So take the resetting codes out of the conditional checking and handling > > branches just as the old code does, the find_smallest_section_pfn()and > > find_biggest_section_pfn() searching have done the the same thing as > > the old for loop did, the logic is kept the same as the old code. This > > can remove the possible confusion. > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > --- > > mm/memory_hotplug.c | 14 ++++++-------- > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > > index 089b6c826a9e..475d0d68a32c 100644 > > --- a/mm/memory_hotplug.c > > +++ b/mm/memory_hotplug.c > > @@ -398,7 +398,7 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone, > > static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > > unsigned long end_pfn) > > { > > - unsigned long pfn; > > + unsigned long pfn = zone->zone_start_pfn; > > int nid = zone_to_nid(zone); > > > > zone_span_writelock(zone); > > @@ -414,9 +414,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > > if (pfn) { > > zone->spanned_pages = zone_end_pfn(zone) - pfn; > > zone->zone_start_pfn = pfn; > > - } else { > > - zone->zone_start_pfn = 0; > > - zone->spanned_pages = 0; > > } > > } else if (zone_end_pfn(zone) == end_pfn) { > > /* > > @@ -429,10 +426,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > > start_pfn); > > if (pfn) > > zone->spanned_pages = pfn - zone->zone_start_pfn + 1; > > - else { > > - zone->zone_start_pfn = 0; > > - zone->spanned_pages = 0; > > - } > > + } > > + > > + if (!pfn) { > > + zone->zone_start_pfn = 0; > > + zone->spanned_pages = 0; > > } > > zone_span_writeunlock(zone); > > } > > > > So, what if your zone starts at pfn 0? Unlikely that we can actually > offline that, but still it is more confusing than the old code IMHO. > Then I prefer to drop the second else case as discussed instead. Hmm, pfn is initialized as zone->zone_start_pfn, does it matter? The impossible empty zone won't go wrong if it really happen. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic 2020-02-06 9:35 ` Baoquan He @ 2020-02-06 9:48 ` David Hildenbrand 2020-02-06 10:00 ` Baoquan He 0 siblings, 1 reply; 10+ messages in thread From: David Hildenbrand @ 2020-02-06 9:48 UTC (permalink / raw) To: Baoquan He; +Cc: linux-kernel, linux-mm, akpm, richardw.yang, mhocko, osalvador On 06.02.20 10:35, Baoquan He wrote: > On 02/06/20 at 09:50am, David Hildenbrand wrote: >> On 06.02.20 06:39, Baoquan He wrote: >>> In commit 950b68d9178b ("mm/memory_hotplug: don't check for "all holes" >>> in shrink_zone_span()"), the zone->zone_start_pfn/->spanned_pages >>> resetting is moved into the if()/else if() branches, if the zone becomes >>> empty. However the 2nd resetting code block may cause misunderstanding. >>> >>> So take the resetting codes out of the conditional checking and handling >>> branches just as the old code does, the find_smallest_section_pfn()and >>> find_biggest_section_pfn() searching have done the the same thing as >>> the old for loop did, the logic is kept the same as the old code. This >>> can remove the possible confusion. >>> >>> Signed-off-by: Baoquan He <bhe@redhat.com> >>> --- >>> mm/memory_hotplug.c | 14 ++++++-------- >>> 1 file changed, 6 insertions(+), 8 deletions(-) >>> >>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >>> index 089b6c826a9e..475d0d68a32c 100644 >>> --- a/mm/memory_hotplug.c >>> +++ b/mm/memory_hotplug.c >>> @@ -398,7 +398,7 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone, >>> static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >>> unsigned long end_pfn) >>> { >>> - unsigned long pfn; >>> + unsigned long pfn = zone->zone_start_pfn; >>> int nid = zone_to_nid(zone); >>> >>> zone_span_writelock(zone); >>> @@ -414,9 +414,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >>> if (pfn) { >>> zone->spanned_pages = zone_end_pfn(zone) - pfn; >>> zone->zone_start_pfn = pfn; >>> - } else { >>> - zone->zone_start_pfn = 0; >>> - zone->spanned_pages = 0; >>> } >>> } else if (zone_end_pfn(zone) == end_pfn) { >>> /* >>> @@ -429,10 +426,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >>> start_pfn); >>> if (pfn) >>> zone->spanned_pages = pfn - zone->zone_start_pfn + 1; >>> - else { >>> - zone->zone_start_pfn = 0; >>> - zone->spanned_pages = 0; >>> - } >>> + } >>> + >>> + if (!pfn) { >>> + zone->zone_start_pfn = 0; >>> + zone->spanned_pages = 0; >>> } >>> zone_span_writeunlock(zone); >>> } >>> >> >> So, what if your zone starts at pfn 0? Unlikely that we can actually >> offline that, but still it is more confusing than the old code IMHO. >> Then I prefer to drop the second else case as discussed instead. > > Hmm, pfn is initialized as zone->zone_start_pfn, does it matter? > The impossible empty zone won't go wrong if it really happen. > If you offline any memory block that belongs to the lowest zone (zone->zone_start_pfn == 0) but does not fall on a boundary (so that you can actually shrink), you would mark the whole zone offline. That's broken unless I am missing something. -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic 2020-02-06 9:48 ` David Hildenbrand @ 2020-02-06 10:00 ` Baoquan He 2020-02-06 10:02 ` David Hildenbrand 0 siblings, 1 reply; 10+ messages in thread From: Baoquan He @ 2020-02-06 10:00 UTC (permalink / raw) To: David Hildenbrand Cc: linux-kernel, linux-mm, akpm, richardw.yang, mhocko, osalvador On 02/06/20 at 10:48am, David Hildenbrand wrote: > On 06.02.20 10:35, Baoquan He wrote: > > On 02/06/20 at 09:50am, David Hildenbrand wrote: > >> On 06.02.20 06:39, Baoquan He wrote: > >>> In commit 950b68d9178b ("mm/memory_hotplug: don't check for "all holes" > >>> in shrink_zone_span()"), the zone->zone_start_pfn/->spanned_pages > >>> resetting is moved into the if()/else if() branches, if the zone becomes > >>> empty. However the 2nd resetting code block may cause misunderstanding. > >>> > >>> So take the resetting codes out of the conditional checking and handling > >>> branches just as the old code does, the find_smallest_section_pfn()and > >>> find_biggest_section_pfn() searching have done the the same thing as > >>> the old for loop did, the logic is kept the same as the old code. This > >>> can remove the possible confusion. > >>> > >>> Signed-off-by: Baoquan He <bhe@redhat.com> > >>> --- > >>> mm/memory_hotplug.c | 14 ++++++-------- > >>> 1 file changed, 6 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > >>> index 089b6c826a9e..475d0d68a32c 100644 > >>> --- a/mm/memory_hotplug.c > >>> +++ b/mm/memory_hotplug.c > >>> @@ -398,7 +398,7 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone, > >>> static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > >>> unsigned long end_pfn) > >>> { > >>> - unsigned long pfn; > >>> + unsigned long pfn = zone->zone_start_pfn; > >>> int nid = zone_to_nid(zone); > >>> > >>> zone_span_writelock(zone); > >>> @@ -414,9 +414,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > >>> if (pfn) { > >>> zone->spanned_pages = zone_end_pfn(zone) - pfn; > >>> zone->zone_start_pfn = pfn; > >>> - } else { > >>> - zone->zone_start_pfn = 0; > >>> - zone->spanned_pages = 0; > >>> } > >>> } else if (zone_end_pfn(zone) == end_pfn) { > >>> /* > >>> @@ -429,10 +426,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > >>> start_pfn); > >>> if (pfn) > >>> zone->spanned_pages = pfn - zone->zone_start_pfn + 1; > >>> - else { > >>> - zone->zone_start_pfn = 0; > >>> - zone->spanned_pages = 0; > >>> - } > >>> + } > >>> + > >>> + if (!pfn) { > >>> + zone->zone_start_pfn = 0; > >>> + zone->spanned_pages = 0; > >>> } > >>> zone_span_writeunlock(zone); > >>> } > >>> > >> > >> So, what if your zone starts at pfn 0? Unlikely that we can actually > >> offline that, but still it is more confusing than the old code IMHO. > >> Then I prefer to drop the second else case as discussed instead. > > > > Hmm, pfn is initialized as zone->zone_start_pfn, does it matter? > > The impossible empty zone won't go wrong if it really happen. > > > > If you offline any memory block that belongs to the lowest zone > (zone->zone_start_pfn == 0) but does not fall on a boundary (so that you > can actually shrink), you would mark the whole zone offline. That's > broken unless I am missing something. AFAIK, the page 0 is reserved. No valid zone can start at 0, only empty zone is. Please correct me if I am wrong. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic 2020-02-06 10:00 ` Baoquan He @ 2020-02-06 10:02 ` David Hildenbrand 2020-02-06 10:05 ` David Hildenbrand 0 siblings, 1 reply; 10+ messages in thread From: David Hildenbrand @ 2020-02-06 10:02 UTC (permalink / raw) To: Baoquan He; +Cc: linux-kernel, linux-mm, akpm, richardw.yang, mhocko, osalvador On 06.02.20 11:00, Baoquan He wrote: > On 02/06/20 at 10:48am, David Hildenbrand wrote: >> On 06.02.20 10:35, Baoquan He wrote: >>> On 02/06/20 at 09:50am, David Hildenbrand wrote: >>>> On 06.02.20 06:39, Baoquan He wrote: >>>>> In commit 950b68d9178b ("mm/memory_hotplug: don't check for "all holes" >>>>> in shrink_zone_span()"), the zone->zone_start_pfn/->spanned_pages >>>>> resetting is moved into the if()/else if() branches, if the zone becomes >>>>> empty. However the 2nd resetting code block may cause misunderstanding. >>>>> >>>>> So take the resetting codes out of the conditional checking and handling >>>>> branches just as the old code does, the find_smallest_section_pfn()and >>>>> find_biggest_section_pfn() searching have done the the same thing as >>>>> the old for loop did, the logic is kept the same as the old code. This >>>>> can remove the possible confusion. >>>>> >>>>> Signed-off-by: Baoquan He <bhe@redhat.com> >>>>> --- >>>>> mm/memory_hotplug.c | 14 ++++++-------- >>>>> 1 file changed, 6 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >>>>> index 089b6c826a9e..475d0d68a32c 100644 >>>>> --- a/mm/memory_hotplug.c >>>>> +++ b/mm/memory_hotplug.c >>>>> @@ -398,7 +398,7 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone, >>>>> static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >>>>> unsigned long end_pfn) >>>>> { >>>>> - unsigned long pfn; >>>>> + unsigned long pfn = zone->zone_start_pfn; >>>>> int nid = zone_to_nid(zone); >>>>> >>>>> zone_span_writelock(zone); >>>>> @@ -414,9 +414,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >>>>> if (pfn) { >>>>> zone->spanned_pages = zone_end_pfn(zone) - pfn; >>>>> zone->zone_start_pfn = pfn; >>>>> - } else { >>>>> - zone->zone_start_pfn = 0; >>>>> - zone->spanned_pages = 0; >>>>> } >>>>> } else if (zone_end_pfn(zone) == end_pfn) { >>>>> /* >>>>> @@ -429,10 +426,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >>>>> start_pfn); >>>>> if (pfn) >>>>> zone->spanned_pages = pfn - zone->zone_start_pfn + 1; >>>>> - else { >>>>> - zone->zone_start_pfn = 0; >>>>> - zone->spanned_pages = 0; >>>>> - } >>>>> + } >>>>> + >>>>> + if (!pfn) { >>>>> + zone->zone_start_pfn = 0; >>>>> + zone->spanned_pages = 0; >>>>> } >>>>> zone_span_writeunlock(zone); >>>>> } >>>>> >>>> >>>> So, what if your zone starts at pfn 0? Unlikely that we can actually >>>> offline that, but still it is more confusing than the old code IMHO. >>>> Then I prefer to drop the second else case as discussed instead. >>> >>> Hmm, pfn is initialized as zone->zone_start_pfn, does it matter? >>> The impossible empty zone won't go wrong if it really happen. >>> >> >> If you offline any memory block that belongs to the lowest zone >> (zone->zone_start_pfn == 0) but does not fall on a boundary (so that you >> can actually shrink), you would mark the whole zone offline. That's >> broken unless I am missing something. > > AFAIK, the page 0 is reserved. No valid zone can start at 0, only empty > zone is. Please correct me if I am wrong. At least on x86 it indeed is :) So if this holds true for all archs Acked-by: David Hildenbrand <david@redhat.com> Thanks! -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic 2020-02-06 10:02 ` David Hildenbrand @ 2020-02-06 10:05 ` David Hildenbrand 2020-02-06 10:12 ` Baoquan He 0 siblings, 1 reply; 10+ messages in thread From: David Hildenbrand @ 2020-02-06 10:05 UTC (permalink / raw) To: Baoquan He; +Cc: linux-kernel, linux-mm, akpm, richardw.yang, mhocko, osalvador On 06.02.20 11:02, David Hildenbrand wrote: > On 06.02.20 11:00, Baoquan He wrote: >> On 02/06/20 at 10:48am, David Hildenbrand wrote: >>> On 06.02.20 10:35, Baoquan He wrote: >>>> On 02/06/20 at 09:50am, David Hildenbrand wrote: >>>>> On 06.02.20 06:39, Baoquan He wrote: >>>>>> In commit 950b68d9178b ("mm/memory_hotplug: don't check for "all holes" >>>>>> in shrink_zone_span()"), the zone->zone_start_pfn/->spanned_pages >>>>>> resetting is moved into the if()/else if() branches, if the zone becomes >>>>>> empty. However the 2nd resetting code block may cause misunderstanding. >>>>>> >>>>>> So take the resetting codes out of the conditional checking and handling >>>>>> branches just as the old code does, the find_smallest_section_pfn()and >>>>>> find_biggest_section_pfn() searching have done the the same thing as >>>>>> the old for loop did, the logic is kept the same as the old code. This >>>>>> can remove the possible confusion. >>>>>> >>>>>> Signed-off-by: Baoquan He <bhe@redhat.com> >>>>>> --- >>>>>> mm/memory_hotplug.c | 14 ++++++-------- >>>>>> 1 file changed, 6 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >>>>>> index 089b6c826a9e..475d0d68a32c 100644 >>>>>> --- a/mm/memory_hotplug.c >>>>>> +++ b/mm/memory_hotplug.c >>>>>> @@ -398,7 +398,7 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone, >>>>>> static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >>>>>> unsigned long end_pfn) >>>>>> { >>>>>> - unsigned long pfn; >>>>>> + unsigned long pfn = zone->zone_start_pfn; >>>>>> int nid = zone_to_nid(zone); >>>>>> >>>>>> zone_span_writelock(zone); >>>>>> @@ -414,9 +414,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >>>>>> if (pfn) { >>>>>> zone->spanned_pages = zone_end_pfn(zone) - pfn; >>>>>> zone->zone_start_pfn = pfn; >>>>>> - } else { >>>>>> - zone->zone_start_pfn = 0; >>>>>> - zone->spanned_pages = 0; >>>>>> } >>>>>> } else if (zone_end_pfn(zone) == end_pfn) { >>>>>> /* >>>>>> @@ -429,10 +426,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >>>>>> start_pfn); >>>>>> if (pfn) >>>>>> zone->spanned_pages = pfn - zone->zone_start_pfn + 1; >>>>>> - else { >>>>>> - zone->zone_start_pfn = 0; >>>>>> - zone->spanned_pages = 0; >>>>>> - } >>>>>> + } >>>>>> + >>>>>> + if (!pfn) { >>>>>> + zone->zone_start_pfn = 0; >>>>>> + zone->spanned_pages = 0; >>>>>> } >>>>>> zone_span_writeunlock(zone); >>>>>> } >>>>>> >>>>> >>>>> So, what if your zone starts at pfn 0? Unlikely that we can actually >>>>> offline that, but still it is more confusing than the old code IMHO. >>>>> Then I prefer to drop the second else case as discussed instead. >>>> >>>> Hmm, pfn is initialized as zone->zone_start_pfn, does it matter? >>>> The impossible empty zone won't go wrong if it really happen. >>>> >>> >>> If you offline any memory block that belongs to the lowest zone >>> (zone->zone_start_pfn == 0) but does not fall on a boundary (so that you >>> can actually shrink), you would mark the whole zone offline. That's >>> broken unless I am missing something. >> >> AFAIK, the page 0 is reserved. No valid zone can start at 0, only empty >> zone is. Please correct me if I am wrong. > > At least on x86 it indeed is :) So if this holds true for all archs > > Acked-by: David Hildenbrand <david@redhat.com> > > Thanks! > > Correction Nacked-by: David Hildenbrand <david@redhat.com> s390x: [linux1@rhkvm01 ~]$ cat /proc/zoneinfo Node 0, zone DMA per-node stats [...] node_unreclaimable: 0 start_pfn: 0 -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic 2020-02-06 10:05 ` David Hildenbrand @ 2020-02-06 10:12 ` Baoquan He 0 siblings, 0 replies; 10+ messages in thread From: Baoquan He @ 2020-02-06 10:12 UTC (permalink / raw) To: David Hildenbrand Cc: linux-kernel, linux-mm, akpm, richardw.yang, mhocko, osalvador On 02/06/20 at 11:05am, David Hildenbrand wrote: > On 06.02.20 11:02, David Hildenbrand wrote: > > On 06.02.20 11:00, Baoquan He wrote: > >> On 02/06/20 at 10:48am, David Hildenbrand wrote: > >>> On 06.02.20 10:35, Baoquan He wrote: > >>>> On 02/06/20 at 09:50am, David Hildenbrand wrote: > >>>>> On 06.02.20 06:39, Baoquan He wrote: > >>>>>> In commit 950b68d9178b ("mm/memory_hotplug: don't check for "all holes" > >>>>>> in shrink_zone_span()"), the zone->zone_start_pfn/->spanned_pages > >>>>>> resetting is moved into the if()/else if() branches, if the zone becomes > >>>>>> empty. However the 2nd resetting code block may cause misunderstanding. > >>>>>> > >>>>>> So take the resetting codes out of the conditional checking and handling > >>>>>> branches just as the old code does, the find_smallest_section_pfn()and > >>>>>> find_biggest_section_pfn() searching have done the the same thing as > >>>>>> the old for loop did, the logic is kept the same as the old code. This > >>>>>> can remove the possible confusion. > >>>>>> > >>>>>> Signed-off-by: Baoquan He <bhe@redhat.com> > >>>>>> --- > >>>>>> mm/memory_hotplug.c | 14 ++++++-------- > >>>>>> 1 file changed, 6 insertions(+), 8 deletions(-) > >>>>>> > >>>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > >>>>>> index 089b6c826a9e..475d0d68a32c 100644 > >>>>>> --- a/mm/memory_hotplug.c > >>>>>> +++ b/mm/memory_hotplug.c > >>>>>> @@ -398,7 +398,7 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone, > >>>>>> static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > >>>>>> unsigned long end_pfn) > >>>>>> { > >>>>>> - unsigned long pfn; > >>>>>> + unsigned long pfn = zone->zone_start_pfn; > >>>>>> int nid = zone_to_nid(zone); > >>>>>> > >>>>>> zone_span_writelock(zone); > >>>>>> @@ -414,9 +414,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > >>>>>> if (pfn) { > >>>>>> zone->spanned_pages = zone_end_pfn(zone) - pfn; > >>>>>> zone->zone_start_pfn = pfn; > >>>>>> - } else { > >>>>>> - zone->zone_start_pfn = 0; > >>>>>> - zone->spanned_pages = 0; > >>>>>> } > >>>>>> } else if (zone_end_pfn(zone) == end_pfn) { > >>>>>> /* > >>>>>> @@ -429,10 +426,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, > >>>>>> start_pfn); > >>>>>> if (pfn) > >>>>>> zone->spanned_pages = pfn - zone->zone_start_pfn + 1; > >>>>>> - else { > >>>>>> - zone->zone_start_pfn = 0; > >>>>>> - zone->spanned_pages = 0; > >>>>>> - } > >>>>>> + } > >>>>>> + > >>>>>> + if (!pfn) { > >>>>>> + zone->zone_start_pfn = 0; > >>>>>> + zone->spanned_pages = 0; > >>>>>> } > >>>>>> zone_span_writeunlock(zone); > >>>>>> } > >>>>>> > >>>>> > >>>>> So, what if your zone starts at pfn 0? Unlikely that we can actually > >>>>> offline that, but still it is more confusing than the old code IMHO. > >>>>> Then I prefer to drop the second else case as discussed instead. > >>>> > >>>> Hmm, pfn is initialized as zone->zone_start_pfn, does it matter? > >>>> The impossible empty zone won't go wrong if it really happen. > >>>> > >>> > >>> If you offline any memory block that belongs to the lowest zone > >>> (zone->zone_start_pfn == 0) but does not fall on a boundary (so that you > >>> can actually shrink), you would mark the whole zone offline. That's > >>> broken unless I am missing something. > >> > >> AFAIK, the page 0 is reserved. No valid zone can start at 0, only empty > >> zone is. Please correct me if I am wrong. > > > > At least on x86 it indeed is :) So if this holds true for all archs > > > > Acked-by: David Hildenbrand <david@redhat.com> > > > > Thanks! > > > > > > Correction > > Nacked-by: David Hildenbrand <david@redhat.com> > > s390x: > [linux1@rhkvm01 ~]$ cat /proc/zoneinfo > Node 0, zone DMA > per-node stats > [...] > node_unreclaimable: 0 > start_pfn: 0 OK, it's very interesting, and good to know. This should be discarded. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic 2020-02-06 8:50 ` David Hildenbrand 2020-02-06 9:35 ` Baoquan He @ 2020-02-06 23:44 ` Wei Yang 1 sibling, 0 replies; 10+ messages in thread From: Wei Yang @ 2020-02-06 23:44 UTC (permalink / raw) To: David Hildenbrand Cc: Baoquan He, linux-kernel, linux-mm, akpm, richardw.yang, mhocko, osalvador On Thu, Feb 06, 2020 at 09:50:07AM +0100, David Hildenbrand wrote: >On 06.02.20 06:39, Baoquan He wrote: >> In commit 950b68d9178b ("mm/memory_hotplug: don't check for "all holes" >> in shrink_zone_span()"), the zone->zone_start_pfn/->spanned_pages >> resetting is moved into the if()/else if() branches, if the zone becomes >> empty. However the 2nd resetting code block may cause misunderstanding. >> >> So take the resetting codes out of the conditional checking and handling >> branches just as the old code does, the find_smallest_section_pfn()and >> find_biggest_section_pfn() searching have done the the same thing as >> the old for loop did, the logic is kept the same as the old code. This >> can remove the possible confusion. >> >> Signed-off-by: Baoquan He <bhe@redhat.com> >> --- >> mm/memory_hotplug.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >> index 089b6c826a9e..475d0d68a32c 100644 >> --- a/mm/memory_hotplug.c >> +++ b/mm/memory_hotplug.c >> @@ -398,7 +398,7 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone, >> static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >> unsigned long end_pfn) >> { >> - unsigned long pfn; >> + unsigned long pfn = zone->zone_start_pfn; >> int nid = zone_to_nid(zone); >> >> zone_span_writelock(zone); >> @@ -414,9 +414,6 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >> if (pfn) { >> zone->spanned_pages = zone_end_pfn(zone) - pfn; >> zone->zone_start_pfn = pfn; >> - } else { >> - zone->zone_start_pfn = 0; >> - zone->spanned_pages = 0; >> } >> } else if (zone_end_pfn(zone) == end_pfn) { >> /* >> @@ -429,10 +426,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn, >> start_pfn); >> if (pfn) >> zone->spanned_pages = pfn - zone->zone_start_pfn + 1; >> - else { >> - zone->zone_start_pfn = 0; >> - zone->spanned_pages = 0; >> - } >> + } >> + >> + if (!pfn) { >> + zone->zone_start_pfn = 0; >> + zone->spanned_pages = 0; >> } >> zone_span_writeunlock(zone); >> } >> > >So, what if your zone starts at pfn 0? Unlikely that we can actually >offline that, but still it is more confusing than the old code IMHO. >Then I prefer to drop the second else case as discussed instead. > Sorry, I just catch up with this thread. If zone starts at 0, find_smallest_section_pfn() will be called only when we want to remove the first section [0, secion_size]. Then find_smallest_section_pfn() return value has two possibilities: * a non-0 section pfn if it still has * 0 if the zone is empty This looks good to me. Or I may misunderstand your point, would you mind sharing more light on your finding? Thanks :-) >-- >Thanks, > >David / dhildenb -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-02-06 23:44 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-06 5:39 [PATCH] mm/hotplug: Adjust shrink_zone_span() to keep the old logic Baoquan He 2020-02-06 6:16 ` Wei Yang 2020-02-06 8:50 ` David Hildenbrand 2020-02-06 9:35 ` Baoquan He 2020-02-06 9:48 ` David Hildenbrand 2020-02-06 10:00 ` Baoquan He 2020-02-06 10:02 ` David Hildenbrand 2020-02-06 10:05 ` David Hildenbrand 2020-02-06 10:12 ` Baoquan He 2020-02-06 23:44 ` Wei Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox