* [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 related [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.