LinuxLists.cc - [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 04.02.20 10:13, Oscar Salvador wrote:
> On Sun, Oct 06, 2019 at 10:56:44AM +0200, David Hildenbrand wrote:
>> If we have holes, the holes will automatically get detected and removed
>> once we remove the next bigger/smaller section. The extra checks can
>> go.
>>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Oscar Salvador <[email protected]>
>> Cc: Michal Hocko <[email protected]>
>> Cc: David Hildenbrand <[email protected]>
>> Cc: Pavel Tatashin <[email protected]>
>> Cc: Dan Williams <[email protected]>
>> Cc: Wei Yang <[email protected]>
>> Signed-off-by: David Hildenbrand <[email protected]>
>
> Heh, I have been here before.
> I have to confess that when I wrote my version of this I was not really 100%
> about removing it, because hotplug was a sort of a "catchall" for all sort of weird
> and corner-cases configurations, but thinking more about it, I cannot think of
> any situation that would make this blow up.
>
> Reviewed-by: Oscar Salvador <[email protected]>

Thanks for your review Oscar!

--
Thanks,

David / dhildenb

2020-02-04 14:27:47

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 10/06/19 at 10:56am, David Hildenbrand wrote:
> If we have holes, the holes will automatically get detected and removed
> once we remove the next bigger/smaller section. The extra checks can
> go.
>
> Cc: Andrew Morton <[email protected]>
> Cc: Oscar Salvador <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Pavel Tatashin <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Wei Yang <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
> ---
> mm/memory_hotplug.c | 34 +++++++---------------------------
> 1 file changed, 7 insertions(+), 27 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index f294918f7211..8dafa1ba8d9f 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> if (pfn) {
> zone->zone_start_pfn = pfn;
> zone->spanned_pages = zone_end_pfn - pfn;
> + } else {
> + zone->zone_start_pfn = 0;
> + zone->spanned_pages = 0;
> }
> } else if (zone_end_pfn == end_pfn) {
> /*
> @@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> start_pfn);
> if (pfn)
> zone->spanned_pages = pfn - zone_start_pfn + 1;
> + else {
> + zone->zone_start_pfn = 0;
> + zone->spanned_pages = 0;

Thinking in which case (zone_start_pfn != start_pfn) and it comes here.

> + }
> }
> -
> - /*
> - * The section is not biggest or smallest mem_section in the zone, it
> - * only creates a hole in the zone. So in this case, we need not
> - * change the zone. But perhaps, the zone has only hole data. Thus
> - * it check the zone has only hole or not.
> - */
> - pfn = zone_start_pfn;
> - for (; pfn < zone_end_pfn; pfn += PAGES_PER_SUBSECTION) {
> - if (unlikely(!pfn_to_online_page(pfn)))
> - continue;
> -
> - if (page_zone(pfn_to_page(pfn)) != zone)
> - continue;
> -
> - /* Skip range to be removed */
> - if (pfn >= start_pfn && pfn < end_pfn)
> - continue;
> -
> - /* If we find valid section, we have nothing to do */
> - zone_span_writeunlock(zone);
> - return;
> - }
> -
> - /* The zone has no valid section */
> - zone->zone_start_pfn = 0;
> - zone->spanned_pages = 0;
> zone_span_writeunlock(zone);
> }
>
> --
> 2.21.0
>
>

2020-02-04 14:45:20

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 04.02.20 15:25, Baoquan He wrote:
> On 10/06/19 at 10:56am, David Hildenbrand wrote:
>> If we have holes, the holes will automatically get detected and removed
>> once we remove the next bigger/smaller section. The extra checks can
>> go.
>>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Oscar Salvador <[email protected]>
>> Cc: Michal Hocko <[email protected]>
>> Cc: David Hildenbrand <[email protected]>
>> Cc: Pavel Tatashin <[email protected]>
>> Cc: Dan Williams <[email protected]>
>> Cc: Wei Yang <[email protected]>
>> Signed-off-by: David Hildenbrand <[email protected]>
>> ---
>> mm/memory_hotplug.c | 34 +++++++---------------------------
>> 1 file changed, 7 insertions(+), 27 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index f294918f7211..8dafa1ba8d9f 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>> if (pfn) {
>> zone->zone_start_pfn = pfn;
>> zone->spanned_pages = zone_end_pfn - pfn;
>> + } else {
>> + zone->zone_start_pfn = 0;
>> + zone->spanned_pages = 0;
>> }
>> } else if (zone_end_pfn == end_pfn) {
>> /*
>> @@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>> start_pfn);
>> if (pfn)
>> zone->spanned_pages = pfn - zone_start_pfn + 1;
>> + else {
>> + zone->zone_start_pfn = 0;
>> + zone->spanned_pages = 0;
>
> Thinking in which case (zone_start_pfn != start_pfn) and it comes here.

Could only happen in case the zone_start_pfn would have been "out of the
zone already". If you ask me: unlikely :)

This change at least maintains the same result as before (where the
all-holes check would have caught it).

--
Thanks,

David / dhildenb

2020-02-05 10:00:48

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On Sun, Oct 06, 2019 at 10:56:44AM +0200, David Hildenbrand wrote:
>If we have holes, the holes will automatically get detected and removed
>once we remove the next bigger/smaller section. The extra checks can
>go.
>
>Cc: Andrew Morton <[email protected]>
>Cc: Oscar Salvador <[email protected]>
>Cc: Michal Hocko <[email protected]>
>Cc: David Hildenbrand <[email protected]>
>Cc: Pavel Tatashin <[email protected]>
>Cc: Dan Williams <[email protected]>
>Cc: Wei Yang <[email protected]>
>Signed-off-by: David Hildenbrand <[email protected]>
>---
> mm/memory_hotplug.c | 34 +++++++---------------------------
> 1 file changed, 7 insertions(+), 27 deletions(-)
>
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index f294918f7211..8dafa1ba8d9f 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> if (pfn) {
> zone->zone_start_pfn = pfn;
> zone->spanned_pages = zone_end_pfn - pfn;
>+ } else {
>+ zone->zone_start_pfn = 0;
>+ zone->spanned_pages = 0;
> }
> } else if (zone_end_pfn == end_pfn) {
> /*
>@@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> start_pfn);
> if (pfn)
> zone->spanned_pages = pfn - zone_start_pfn + 1;
>+ else {
>+ zone->zone_start_pfn = 0;
>+ zone->spanned_pages = 0;
>+ }
> }

If it is me, I would like to take out these two similar logic out.

For example:

if () {
} else if () {
} else {
goto out;
}

/* The zone has no valid section */
if (!pfn) {
zone->zone_start_pfn = 0;
zone->spanned_pages = 0;
}

out:
zone_span_writeunlock(zone);

Well, this is just my personal taste :-)

>-
>- /*
>- * The section is not biggest or smallest mem_section in the zone, it
>- * only creates a hole in the zone. So in this case, we need not
>- * change the zone. But perhaps, the zone has only hole data. Thus
>- * it check the zone has only hole or not.
>- */
>- pfn = zone_start_pfn;
>- for (; pfn < zone_end_pfn; pfn += PAGES_PER_SUBSECTION) {
>- if (unlikely(!pfn_to_online_page(pfn)))
>- continue;
>-
>- if (page_zone(pfn_to_page(pfn)) != zone)
>- continue;
>-
>- /* Skip range to be removed */
>- if (pfn >= start_pfn && pfn < end_pfn)
>- continue;
>-
>- /* If we find valid section, we have nothing to do */
>- zone_span_writeunlock(zone);
>- return;
>- }
>-
>- /* The zone has no valid section */
>- zone->zone_start_pfn = 0;
>- zone->spanned_pages = 0;
> zone_span_writeunlock(zone);
> }
>
>--
>2.21.0

--
Wei Yang
Help you, Help me

2020-02-05 12:45:48

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 02/04/20 at 03:42pm, David Hildenbrand wrote:
> On 04.02.20 15:25, Baoquan He wrote:
> > On 10/06/19 at 10:56am, David Hildenbrand wrote:
> >> If we have holes, the holes will automatically get detected and removed
> >> once we remove the next bigger/smaller section. The extra checks can
> >> go.
> >>
> >> Cc: Andrew Morton <[email protected]>
> >> Cc: Oscar Salvador <[email protected]>
> >> Cc: Michal Hocko <[email protected]>
> >> Cc: David Hildenbrand <[email protected]>
> >> Cc: Pavel Tatashin <[email protected]>
> >> Cc: Dan Williams <[email protected]>
> >> Cc: Wei Yang <[email protected]>
> >> Signed-off-by: David Hildenbrand <[email protected]>
> >> ---
> >> mm/memory_hotplug.c | 34 +++++++---------------------------
> >> 1 file changed, 7 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >> index f294918f7211..8dafa1ba8d9f 100644
> >> --- a/mm/memory_hotplug.c
> >> +++ b/mm/memory_hotplug.c
> >> @@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> >> if (pfn) {
> >> zone->zone_start_pfn = pfn;
> >> zone->spanned_pages = zone_end_pfn - pfn;
> >> + } else {
> >> + zone->zone_start_pfn = 0;
> >> + zone->spanned_pages = 0;
> >> }
> >> } else if (zone_end_pfn == end_pfn) {
> >> /*
> >> @@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> >> start_pfn);
> >> if (pfn)
> >> zone->spanned_pages = pfn - zone_start_pfn + 1;
> >> + else {
> >> + zone->zone_start_pfn = 0;
> >> + zone->spanned_pages = 0;
> >
> > Thinking in which case (zone_start_pfn != start_pfn) and it comes here.
>
> Could only happen in case the zone_start_pfn would have been "out of the
> zone already". If you ask me: unlikely :)

Yeah, I also think it's unlikely to come here.

The 'if (zone_start_pfn == start_pfn)' checking also covers the case
(zone_start_pfn == start_pfn && zone_end_pfn == end_pfn). So this
zone_start_pfn/spanned_pages resetting can be removed to avoid
confusion.

2020-02-05 13:23:25

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 05.02.20 13:43, Baoquan He wrote:
> On 02/04/20 at 03:42pm, David Hildenbrand wrote:
>> On 04.02.20 15:25, Baoquan He wrote:
>>> On 10/06/19 at 10:56am, David Hildenbrand wrote:
>>>> If we have holes, the holes will automatically get detected and removed
>>>> once we remove the next bigger/smaller section. The extra checks can
>>>> go.
>>>>
>>>> Cc: Andrew Morton <[email protected]>
>>>> Cc: Oscar Salvador <[email protected]>
>>>> Cc: Michal Hocko <[email protected]>
>>>> Cc: David Hildenbrand <[email protected]>
>>>> Cc: Pavel Tatashin <[email protected]>
>>>> Cc: Dan Williams <[email protected]>
>>>> Cc: Wei Yang <[email protected]>
>>>> Signed-off-by: David Hildenbrand <[email protected]>
>>>> ---
>>>> mm/memory_hotplug.c | 34 +++++++---------------------------
>>>> 1 file changed, 7 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>>> index f294918f7211..8dafa1ba8d9f 100644
>>>> --- a/mm/memory_hotplug.c
>>>> +++ b/mm/memory_hotplug.c
>>>> @@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>>>> if (pfn) {
>>>> zone->zone_start_pfn = pfn;
>>>> zone->spanned_pages = zone_end_pfn - pfn;
>>>> + } else {
>>>> + zone->zone_start_pfn = 0;
>>>> + zone->spanned_pages = 0;
>>>> }
>>>> } else if (zone_end_pfn == end_pfn) {
>>>> /*
>>>> @@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>>>> start_pfn);
>>>> if (pfn)
>>>> zone->spanned_pages = pfn - zone_start_pfn + 1;
>>>> + else {
>>>> + zone->zone_start_pfn = 0;
>>>> + zone->spanned_pages = 0;
>>>
>>> Thinking in which case (zone_start_pfn != start_pfn) and it comes here.
>>
>> Could only happen in case the zone_start_pfn would have been "out of the
>> zone already". If you ask me: unlikely :)
>
> Yeah, I also think it's unlikely to come here.
>
> The 'if (zone_start_pfn == start_pfn)' checking also covers the case
> (zone_start_pfn == start_pfn && zone_end_pfn == end_pfn). So this
> zone_start_pfn/spanned_pages resetting can be removed to avoid
> confusion.

At least I would find it more confusing without it (or want a comment
explaining why this does not have to be handled and why the !pfn case is
not possible).

Anyhow, that patch is already upstream and I don't consider this high
priority. Thanks :)

--
Thanks,

David / dhildenb

2020-02-05 13:36:23

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 02/05/20 at 02:20pm, David Hildenbrand wrote:
> On 05.02.20 13:43, Baoquan He wrote:
> > On 02/04/20 at 03:42pm, David Hildenbrand wrote:
> >> On 04.02.20 15:25, Baoquan He wrote:
> >>> On 10/06/19 at 10:56am, David Hildenbrand wrote:
> >>>> If we have holes, the holes will automatically get detected and removed
> >>>> once we remove the next bigger/smaller section. The extra checks can
> >>>> go.
> >>>>
> >>>> Cc: Andrew Morton <[email protected]>
> >>>> Cc: Oscar Salvador <[email protected]>
> >>>> Cc: Michal Hocko <[email protected]>
> >>>> Cc: David Hildenbrand <[email protected]>
> >>>> Cc: Pavel Tatashin <[email protected]>
> >>>> Cc: Dan Williams <[email protected]>
> >>>> Cc: Wei Yang <[email protected]>
> >>>> Signed-off-by: David Hildenbrand <[email protected]>
> >>>> ---
> >>>> mm/memory_hotplug.c | 34 +++++++---------------------------
> >>>> 1 file changed, 7 insertions(+), 27 deletions(-)
> >>>>
> >>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >>>> index f294918f7211..8dafa1ba8d9f 100644
> >>>> --- a/mm/memory_hotplug.c
> >>>> +++ b/mm/memory_hotplug.c
> >>>> @@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> >>>> if (pfn) {
> >>>> zone->zone_start_pfn = pfn;
> >>>> zone->spanned_pages = zone_end_pfn - pfn;
> >>>> + } else {
> >>>> + zone->zone_start_pfn = 0;
> >>>> + zone->spanned_pages = 0;
> >>>> }
> >>>> } else if (zone_end_pfn == end_pfn) {
> >>>> /*
> >>>> @@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> >>>> start_pfn);
> >>>> if (pfn)
> >>>> zone->spanned_pages = pfn - zone_start_pfn + 1;
> >>>> + else {
> >>>> + zone->zone_start_pfn = 0;
> >>>> + zone->spanned_pages = 0;
> >>>
> >>> Thinking in which case (zone_start_pfn != start_pfn) and it comes here.
> >>
> >> Could only happen in case the zone_start_pfn would have been "out of the
> >> zone already". If you ask me: unlikely :)
> >
> > Yeah, I also think it's unlikely to come here.
> >
> > The 'if (zone_start_pfn == start_pfn)' checking also covers the case
> > (zone_start_pfn == start_pfn && zone_end_pfn == end_pfn). So this
> > zone_start_pfn/spanned_pages resetting can be removed to avoid
> > confusion.
>
> At least I would find it more confusing without it (or want a comment
> explaining why this does not have to be handled and why the !pfn case is
> not possible).

I don't get why being w/o it will be more confusing, but it's OK since
it doesn't impact anything.

>
> Anyhow, that patch is already upstream and I don't consider this high
> priority. Thanks :)

Yeah, noticed you told Wei the status in another patch thread, I am fine
with it, just leave it to you to decide. Thanks.

2020-02-05 13:39:59

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 05.02.20 14:34, Baoquan He wrote:
> On 02/05/20 at 02:20pm, David Hildenbrand wrote:
>> On 05.02.20 13:43, Baoquan He wrote:
>>> On 02/04/20 at 03:42pm, David Hildenbrand wrote:
>>>> On 04.02.20 15:25, Baoquan He wrote:
>>>>> On 10/06/19 at 10:56am, David Hildenbrand wrote:
>>>>>> If we have holes, the holes will automatically get detected and removed
>>>>>> once we remove the next bigger/smaller section. The extra checks can
>>>>>> go.
>>>>>>
>>>>>> Cc: Andrew Morton <[email protected]>
>>>>>> Cc: Oscar Salvador <[email protected]>
>>>>>> Cc: Michal Hocko <[email protected]>
>>>>>> Cc: David Hildenbrand <[email protected]>
>>>>>> Cc: Pavel Tatashin <[email protected]>
>>>>>> Cc: Dan Williams <[email protected]>
>>>>>> Cc: Wei Yang <[email protected]>
>>>>>> Signed-off-by: David Hildenbrand <[email protected]>
>>>>>> ---
>>>>>> mm/memory_hotplug.c | 34 +++++++---------------------------
>>>>>> 1 file changed, 7 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>>>>> index f294918f7211..8dafa1ba8d9f 100644
>>>>>> --- a/mm/memory_hotplug.c
>>>>>> +++ b/mm/memory_hotplug.c
>>>>>> @@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>>>>>> if (pfn) {
>>>>>> zone->zone_start_pfn = pfn;
>>>>>> zone->spanned_pages = zone_end_pfn - pfn;
>>>>>> + } else {
>>>>>> + zone->zone_start_pfn = 0;
>>>>>> + zone->spanned_pages = 0;
>>>>>> }
>>>>>> } else if (zone_end_pfn == end_pfn) {
>>>>>> /*
>>>>>> @@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>>>>>> start_pfn);
>>>>>> if (pfn)
>>>>>> zone->spanned_pages = pfn - zone_start_pfn + 1;
>>>>>> + else {
>>>>>> + zone->zone_start_pfn = 0;
>>>>>> + zone->spanned_pages = 0;
>>>>>
>>>>> Thinking in which case (zone_start_pfn != start_pfn) and it comes here.
>>>>
>>>> Could only happen in case the zone_start_pfn would have been "out of the
>>>> zone already". If you ask me: unlikely :)
>>>
>>> Yeah, I also think it's unlikely to come here.
>>>
>>> The 'if (zone_start_pfn == start_pfn)' checking also covers the case
>>> (zone_start_pfn == start_pfn && zone_end_pfn == end_pfn). So this
>>> zone_start_pfn/spanned_pages resetting can be removed to avoid
>>> confusion.
>>
>> At least I would find it more confusing without it (or want a comment
>> explaining why this does not have to be handled and why the !pfn case is
>> not possible).
>
> I don't get why being w/o it will be more confusing, but it's OK since
> it doesn't impact anything.

Because we could actually BUG_ON(!pfn) here, right? Only having a "if
(pfn)" leaves the reader wondering "why is the other case not handled".

>
>>
>> Anyhow, that patch is already upstream and I don't consider this high
>> priority. Thanks :)
>
> Yeah, noticed you told Wei the status in another patch thread, I am fine
> with it, just leave it to you to decide. Thanks.

I am fairly busy right now. Can you send a patch (double-checking and
making this eventually unconditional?). Thanks!

--
Thanks,

David / dhildenb

2020-02-05 14:14:45

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 02/05/20 at 02:38pm, David Hildenbrand wrote:
> On 05.02.20 14:34, Baoquan He wrote:
> > On 02/05/20 at 02:20pm, David Hildenbrand wrote:
> >> On 05.02.20 13:43, Baoquan He wrote:
> >>> On 02/04/20 at 03:42pm, David Hildenbrand wrote:
> >>>> On 04.02.20 15:25, Baoquan He wrote:
> >>>>> On 10/06/19 at 10:56am, David Hildenbrand wrote:
> >>>>>> If we have holes, the holes will automatically get detected and removed
> >>>>>> once we remove the next bigger/smaller section. The extra checks can
> >>>>>> go.
> >>>>>>
> >>>>>> Cc: Andrew Morton <[email protected]>
> >>>>>> Cc: Oscar Salvador <[email protected]>
> >>>>>> Cc: Michal Hocko <[email protected]>
> >>>>>> Cc: David Hildenbrand <[email protected]>
> >>>>>> Cc: Pavel Tatashin <[email protected]>
> >>>>>> Cc: Dan Williams <[email protected]>
> >>>>>> Cc: Wei Yang <[email protected]>
> >>>>>> Signed-off-by: David Hildenbrand <[email protected]>
> >>>>>> ---
> >>>>>> mm/memory_hotplug.c | 34 +++++++---------------------------
> >>>>>> 1 file changed, 7 insertions(+), 27 deletions(-)
> >>>>>>
> >>>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >>>>>> index f294918f7211..8dafa1ba8d9f 100644
> >>>>>> --- a/mm/memory_hotplug.c
> >>>>>> +++ b/mm/memory_hotplug.c
> >>>>>> @@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> >>>>>> if (pfn) {
> >>>>>> zone->zone_start_pfn = pfn;
> >>>>>> zone->spanned_pages = zone_end_pfn - pfn;
> >>>>>> + } else {
> >>>>>> + zone->zone_start_pfn = 0;
> >>>>>> + zone->spanned_pages = 0;
> >>>>>> }
> >>>>>> } else if (zone_end_pfn == end_pfn) {
> >>>>>> /*
> >>>>>> @@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> >>>>>> start_pfn);
> >>>>>> if (pfn)
> >>>>>> zone->spanned_pages = pfn - zone_start_pfn + 1;
> >>>>>> + else {
> >>>>>> + zone->zone_start_pfn = 0;
> >>>>>> + zone->spanned_pages = 0;
> >>>>>
> >>>>> Thinking in which case (zone_start_pfn != start_pfn) and it comes here.
> >>>>
> >>>> Could only happen in case the zone_start_pfn would have been "out of the
> >>>> zone already". If you ask me: unlikely :)
> >>>
> >>> Yeah, I also think it's unlikely to come here.
> >>>
> >>> The 'if (zone_start_pfn == start_pfn)' checking also covers the case
> >>> (zone_start_pfn == start_pfn && zone_end_pfn == end_pfn). So this
> >>> zone_start_pfn/spanned_pages resetting can be removed to avoid
> >>> confusion.
> >>
> >> At least I would find it more confusing without it (or want a comment
> >> explaining why this does not have to be handled and why the !pfn case is
> >> not possible).
> >
> > I don't get why being w/o it will be more confusing, but it's OK since
> > it doesn't impact anything.
>
> Because we could actually BUG_ON(!pfn) here, right? Only having a "if
> (pfn)" leaves the reader wondering "why is the other case not handled".
>
> >
> >>
> >> Anyhow, that patch is already upstream and I don't consider this high
> >> priority. Thanks :)
> >
> > Yeah, noticed you told Wei the status in another patch thread, I am fine
> > with it, just leave it to you to decide. Thanks.
>
> I am fairly busy right now. Can you send a patch (double-checking and
> making this eventually unconditional?). Thanks!

Understood, sorry about the noise, David. I will think about this.

2020-02-05 14:17:41

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

>>>> Anyhow, that patch is already upstream and I don't consider this high
>>>> priority. Thanks :)
>>>
>>> Yeah, noticed you told Wei the status in another patch thread, I am fine
>>> with it, just leave it to you to decide. Thanks.
>>
>> I am fairly busy right now. Can you send a patch (double-checking and
>> making this eventually unconditional?). Thanks!
>
> Understood, sorry about the noise, David. I will think about this.
>

No need to excuse, really, I'm very happy about review feedback!

The review of this series happened fairly late. Bad, because it's not
perfect, but good, because no serious stuff was found (so far :) ). If
you also don't have time to look into this, I can put it onto my todo
list, just let me know.

Thanks!

--
Thanks,

David / dhildenb

2020-02-05 14:27:41

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 02/05/20 at 03:16pm, David Hildenbrand wrote:
> >>>> Anyhow, that patch is already upstream and I don't consider this high
> >>>> priority. Thanks :)
> >>>
> >>> Yeah, noticed you told Wei the status in another patch thread, I am fine
> >>> with it, just leave it to you to decide. Thanks.
> >>
> >> I am fairly busy right now. Can you send a patch (double-checking and
> >> making this eventually unconditional?). Thanks!
> >
> > Understood, sorry about the noise, David. I will think about this.
> >
>
> No need to excuse, really, I'm very happy about review feedback!
>

Glad to hear it, thanks.

> The review of this series happened fairly late. Bad, because it's not
> perfect, but good, because no serious stuff was found (so far :) ). If
> you also don't have time to look into this, I can put it onto my todo
> list, just let me know.

Both is OK to me, as long as thing is clear to us. I will discuss with
Wei Yang for now. You can post patch anytime if you make one.

2020-02-05 14:49:45

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

Hi Wei Yang,

On 02/05/20 at 05:59pm, Wei Yang wrote:
> >diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >index f294918f7211..8dafa1ba8d9f 100644
> >--- a/mm/memory_hotplug.c
> >+++ b/mm/memory_hotplug.c
> >@@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> > if (pfn) {
> > zone->zone_start_pfn = pfn;
> > zone->spanned_pages = zone_end_pfn - pfn;
> >+ } else {
> >+ zone->zone_start_pfn = 0;
> >+ zone->spanned_pages = 0;
> > }
> > } else if (zone_end_pfn == end_pfn) {
> > /*
> >@@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> > start_pfn);
> > if (pfn)
> > zone->spanned_pages = pfn - zone_start_pfn + 1;
> >+ else {
> >+ zone->zone_start_pfn = 0;
> >+ zone->spanned_pages = 0;
> >+ }
> > }
>
> If it is me, I would like to take out these two similar logic out.

I also like this style.
>
> For example:
>
> if () {
> } else if () {
> } else {
> goto out;
Here the last else is unnecessary, right?

> }
>
>

Like this, I believe both David and I will be satisfactory. Even though
I still think his 2nd resetting is not needed :-)

> /* The zone has no valid section */
> if (!pfn) {
> zone->zone_start_pfn = 0;
> zone->spanned_pages = 0;
> }
>
> out:
> zone_span_writeunlock(zone);
>
> Well, this is just my personal taste :-)
>
> >-
> >- /*
> >- * The section is not biggest or smallest mem_section in the zone, it
> >- * only creates a hole in the zone. So in this case, we need not
> >- * change the zone. But perhaps, the zone has only hole data. Thus
> >- * it check the zone has only hole or not.
> >- */
> >- pfn = zone_start_pfn;
> >- for (; pfn < zone_end_pfn; pfn += PAGES_PER_SUBSECTION) {
> >- if (unlikely(!pfn_to_online_page(pfn)))
> >- continue;
> >-
> >- if (page_zone(pfn_to_page(pfn)) != zone)
> >- continue;
> >-
> >- /* Skip range to be removed */
> >- if (pfn >= start_pfn && pfn < end_pfn)
> >- continue;
> >-
> >- /* If we find valid section, we have nothing to do */
> >- zone_span_writeunlock(zone);
> >- return;
> >- }
> >-
> >- /* The zone has no valid section */
> >- zone->zone_start_pfn = 0;
> >- zone->spanned_pages = 0;
> > zone_span_writeunlock(zone);
> > }
> >
> >--
> >2.21.0
>
> --
> Wei Yang
> Help you, Help me
>

2020-02-05 14:55:57

by David Laight

[permalink] [raw]

Subject: RE: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

From: Wei Yang
> Sent: 05 February 2020 09:59
...
> If it is me, I would like to take out these two similar logic out.
>
> For example:
>
> if () {
> } else if () {
> } else {
> goto out;
> }

I'm pretty sure the kernel layout rules disallow 'else if'.
It is also pretty horrid unless the conditionals are all related
(so it is almost a switch statement).

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

2020-02-05 14:57:54

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 05.02.20 15:54, David Laight wrote:
> From: Wei Yang
>> Sent: 05 February 2020 09:59
> ...
>> If it is me, I would like to take out these two similar logic out.
>>
>> For example:
>>
>> if () {
>> } else if () {
>> } else {
>> goto out;
>> }
>
> I'm pretty sure the kernel layout rules disallow 'else if'.

Huh?

git grep "else if" | wc -l
49336

I'm afraid I don't get what you mean :)

--
Thanks,

David / dhildenb

2020-02-05 22:57:24

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On Wed, Feb 05, 2020 at 10:48:11PM +0800, Baoquan He wrote:
>Hi Wei Yang,
>
>On 02/05/20 at 05:59pm, Wei Yang wrote:
>> >diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> >index f294918f7211..8dafa1ba8d9f 100644
>> >--- a/mm/memory_hotplug.c
>> >+++ b/mm/memory_hotplug.c
>> >@@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>> > if (pfn) {
>> > zone->zone_start_pfn = pfn;
>> > zone->spanned_pages = zone_end_pfn - pfn;
>> >+ } else {
>> >+ zone->zone_start_pfn = 0;
>> >+ zone->spanned_pages = 0;
>> > }
>> > } else if (zone_end_pfn == end_pfn) {
>> > /*
>> >@@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>> > start_pfn);
>> > if (pfn)
>> > zone->spanned_pages = pfn - zone_start_pfn + 1;
>> >+ else {
>> >+ zone->zone_start_pfn = 0;
>> >+ zone->spanned_pages = 0;
>> >+ }
>> > }
>>
>> If it is me, I would like to take out these two similar logic out.
>
>I also like this style.
>>
>> For example:
>>
>> if () {
>> } else if () {
>> } else {
>> goto out;
>Here the last else is unnecessary, right?
>

I am afraid not.

If the range is not the first or last, we would leave pfn not initialized.

--
Wei Yang
Help you, Help me

2020-02-05 23:11:40

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 02/06/20 at 06:56am, Wei Yang wrote:
> On Wed, Feb 05, 2020 at 10:48:11PM +0800, Baoquan He wrote:
> >Hi Wei Yang,
> >
> >On 02/05/20 at 05:59pm, Wei Yang wrote:
> >> >diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >> >index f294918f7211..8dafa1ba8d9f 100644
> >> >--- a/mm/memory_hotplug.c
> >> >+++ b/mm/memory_hotplug.c
> >> >@@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> >> > if (pfn) {
> >> > zone->zone_start_pfn = pfn;
> >> > zone->spanned_pages = zone_end_pfn - pfn;
> >> >+ } else {
> >> >+ zone->zone_start_pfn = 0;
> >> >+ zone->spanned_pages = 0;
> >> > }
> >> > } else if (zone_end_pfn == end_pfn) {
> >> > /*
> >> >@@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> >> > start_pfn);
> >> > if (pfn)
> >> > zone->spanned_pages = pfn - zone_start_pfn + 1;
> >> >+ else {
> >> >+ zone->zone_start_pfn = 0;
> >> >+ zone->spanned_pages = 0;
> >> >+ }
> >> > }
> >>
> >> If it is me, I would like to take out these two similar logic out.
> >
> >I also like this style.
> >>
> >> For example:
> >>
> >> if () {
> >> } else if () {
> >> } else {
> >> goto out;
> >Here the last else is unnecessary, right?
> >
>
> I am afraid not.
>
> If the range is not the first or last, we would leave pfn not initialized.

Ah, you are right. I forgot that one. Then pfn can be assigned the
zone_start_pfn as the old code. Then the following logic is the same
as the original code, find_smallest_section_pfn()/find_biggest_section_pfn()
have done the iteration the old for loop was doing.

unsigned long pfn = zone_start_pfn;
if () {
} else if () {
}

/* The zone has no valid section */
if (!pfn) {
zone->zone_start_pfn = 0;
zone->spanned_pages = 0;
}

2020-02-05 23:32:14

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On 02/06/20 at 07:26am, Wei Yang wrote:
> On Thu, Feb 06, 2020 at 07:08:26AM +0800, Baoquan He wrote:
> >On 02/06/20 at 06:56am, Wei Yang wrote:
> >> On Wed, Feb 05, 2020 at 10:48:11PM +0800, Baoquan He wrote:
> >> >Hi Wei Yang,
> >> >
> >> >On 02/05/20 at 05:59pm, Wei Yang wrote:
> >> >> >diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >> >> >index f294918f7211..8dafa1ba8d9f 100644
> >> >> >--- a/mm/memory_hotplug.c
> >> >> >+++ b/mm/memory_hotplug.c
> >> >> >@@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> >> >> > if (pfn) {
> >> >> > zone->zone_start_pfn = pfn;
> >> >> > zone->spanned_pages = zone_end_pfn - pfn;
> >> >> >+ } else {
> >> >> >+ zone->zone_start_pfn = 0;
> >> >> >+ zone->spanned_pages = 0;
> >> >> > }
> >> >> > } else if (zone_end_pfn == end_pfn) {
> >> >> > /*
> >> >> >@@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
> >> >> > start_pfn);
> >> >> > if (pfn)
> >> >> > zone->spanned_pages = pfn - zone_start_pfn + 1;
> >> >> >+ else {
> >> >> >+ zone->zone_start_pfn = 0;
> >> >> >+ zone->spanned_pages = 0;
> >> >> >+ }
> >> >> > }
> >> >>
> >> >> If it is me, I would like to take out these two similar logic out.
> >> >
> >> >I also like this style.
> >> >>
> >> >> For example:
> >> >>
> >> >> if () {
> >> >> } else if () {
> >> >> } else {
> >> >> goto out;
> >> >Here the last else is unnecessary, right?
> >> >
> >>
> >> I am afraid not.
> >>
> >> If the range is not the first or last, we would leave pfn not initialized.
> >
> >Ah, you are right. I forgot that one. Then pfn can be assigned the
> >zone_start_pfn as the old code. Then the following logic is the same
> >as the original code, find_smallest_section_pfn()/find_biggest_section_pfn()
> >have done the iteration the old for loop was doing.
> >
> > unsigned long pfn = zone_start_pfn;
> > if () {
> > } else if () {
> > }
> >
> > /* The zone has no valid section */
> > if (!pfn) {
> > zone->zone_start_pfn = 0;
> > zone->spanned_pages = 0;
> > }
>
> This one look better :-)

Thanks for your confirmation, I will make one patch like this and post.

2020-02-05 23:35:07

[permalink] [raw]

Subject: Re: [PATCH v6 08/10] mm/memory_hotplug: Don't check for "all holes" in shrink_zone_span()

On Thu, Feb 06, 2020 at 07:08:26AM +0800, Baoquan He wrote:
>On 02/06/20 at 06:56am, Wei Yang wrote:
>> On Wed, Feb 05, 2020 at 10:48:11PM +0800, Baoquan He wrote:
>> >Hi Wei Yang,
>> >
>> >On 02/05/20 at 05:59pm, Wei Yang wrote:
>> >> >diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> >> >index f294918f7211..8dafa1ba8d9f 100644
>> >> >--- a/mm/memory_hotplug.c
>> >> >+++ b/mm/memory_hotplug.c
>> >> >@@ -393,6 +393,9 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>> >> > if (pfn) {
>> >> > zone->zone_start_pfn = pfn;
>> >> > zone->spanned_pages = zone_end_pfn - pfn;
>> >> >+ } else {
>> >> >+ zone->zone_start_pfn = 0;
>> >> >+ zone->spanned_pages = 0;
>> >> > }
>> >> > } else if (zone_end_pfn == end_pfn) {
>> >> > /*
>> >> >@@ -405,34 +408,11 @@ static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
>> >> > start_pfn);
>> >> > if (pfn)
>> >> > zone->spanned_pages = pfn - zone_start_pfn + 1;
>> >> >+ else {
>> >> >+ zone->zone_start_pfn = 0;
>> >> >+ zone->spanned_pages = 0;
>> >> >+ }
>> >> > }
>> >>
>> >> If it is me, I would like to take out these two similar logic out.
>> >
>> >I also like this style.
>> >>
>> >> For example:
>> >>
>> >> if () {
>> >> } else if () {
>> >> } else {
>> >> goto out;
>> >Here the last else is unnecessary, right?
>> >
>>
>> I am afraid not.
>>
>> If the range is not the first or last, we would leave pfn not initialized.
>
>Ah, you are right. I forgot that one. Then pfn can be assigned the
>zone_start_pfn as the old code. Then the following logic is the same
>as the original code, find_smallest_section_pfn()/find_biggest_section_pfn()
>have done the iteration the old for loop was doing.
>
> unsigned long pfn = zone_start_pfn;
> if () {
> } else if () {
> }
>
> /* The zone has no valid section */
> if (!pfn) {
> zone->zone_start_pfn = 0;
> zone->spanned_pages = 0;
> }

This one look better :-)

Thanks

--
Wei Yang
Help you, Help me

2020-02-05 23:37:47