| Message ID | 20200206053912.1211-1-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hotplug: Adjust shrink_zone_span() to keep the old logic | expand |
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
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.
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.
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.
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.
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!
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
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.
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
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); }
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(-)