| Message ID | 20191006085646.5768-9-david@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | mm/memory_hotplug: Shrink zones before removing memory | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch next (6edfc6487b474fe01857dc3f1a9cd701bb9b21c8) |
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch merge (b05e997bf5d33f38e3fc6a66d52303eb109598ec) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 47 lines checked |
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 <akpm@linux-foundation.org> > Cc: Oscar Salvador <osalvador@suse.de> > Cc: Michal Hocko <mhocko@suse.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Pavel Tatashin <pasha.tatashin@soleen.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Wei Yang <richardw.yang@linux.intel.com> > Signed-off-by: David Hildenbrand <david@redhat.com> 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 <osalvador@suse.de> > --- > 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; > + } > } > - > - /* > - * 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 >
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 <akpm@linux-foundation.org> >> Cc: Oscar Salvador <osalvador@suse.de> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: David Hildenbrand <david@redhat.com> >> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> >> Cc: Dan Williams <dan.j.williams@intel.com> >> Cc: Wei Yang <richardw.yang@linux.intel.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > 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 <osalvador@suse.de> Thanks for your review Oscar!
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 <akpm@linux-foundation.org> > Cc: Oscar Salvador <osalvador@suse.de> > Cc: Michal Hocko <mhocko@suse.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Pavel Tatashin <pasha.tatashin@soleen.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Wei Yang <richardw.yang@linux.intel.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > 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 > >
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 <akpm@linux-foundation.org> >> Cc: Oscar Salvador <osalvador@suse.de> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: David Hildenbrand <david@redhat.com> >> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> >> Cc: Dan Williams <dan.j.williams@intel.com> >> Cc: Wei Yang <richardw.yang@linux.intel.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> 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).
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 <akpm@linux-foundation.org> >Cc: Oscar Salvador <osalvador@suse.de> >Cc: Michal Hocko <mhocko@suse.com> >Cc: David Hildenbrand <david@redhat.com> >Cc: Pavel Tatashin <pasha.tatashin@soleen.com> >Cc: Dan Williams <dan.j.williams@intel.com> >Cc: Wei Yang <richardw.yang@linux.intel.com> >Signed-off-by: David Hildenbrand <david@redhat.com> >--- > 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
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 <akpm@linux-foundation.org> > >> Cc: Oscar Salvador <osalvador@suse.de> > >> Cc: Michal Hocko <mhocko@suse.com> > >> Cc: David Hildenbrand <david@redhat.com> > >> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> > >> Cc: Dan Williams <dan.j.williams@intel.com> > >> Cc: Wei Yang <richardw.yang@linux.intel.com> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> 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.
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 <akpm@linux-foundation.org> >>>> Cc: Oscar Salvador <osalvador@suse.de> >>>> Cc: Michal Hocko <mhocko@suse.com> >>>> Cc: David Hildenbrand <david@redhat.com> >>>> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> >>>> Cc: Dan Williams <dan.j.williams@intel.com> >>>> Cc: Wei Yang <richardw.yang@linux.intel.com> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> 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 :)
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 <akpm@linux-foundation.org> > >>>> Cc: Oscar Salvador <osalvador@suse.de> > >>>> Cc: Michal Hocko <mhocko@suse.com> > >>>> Cc: David Hildenbrand <david@redhat.com> > >>>> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> > >>>> Cc: Dan Williams <dan.j.williams@intel.com> > >>>> Cc: Wei Yang <richardw.yang@linux.intel.com> > >>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>> --- > >>>> 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.
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 <akpm@linux-foundation.org> >>>>>> Cc: Oscar Salvador <osalvador@suse.de> >>>>>> Cc: Michal Hocko <mhocko@suse.com> >>>>>> Cc: David Hildenbrand <david@redhat.com> >>>>>> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> >>>>>> Cc: Dan Williams <dan.j.williams@intel.com> >>>>>> Cc: Wei Yang <richardw.yang@linux.intel.com> >>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>>> --- >>>>>> 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!
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 <akpm@linux-foundation.org> > >>>>>> Cc: Oscar Salvador <osalvador@suse.de> > >>>>>> Cc: Michal Hocko <mhocko@suse.com> > >>>>>> Cc: David Hildenbrand <david@redhat.com> > >>>>>> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> > >>>>>> Cc: Dan Williams <dan.j.williams@intel.com> > >>>>>> Cc: Wei Yang <richardw.yang@linux.intel.com> > >>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>>>> --- > >>>>>> 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.
>>>> 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!
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.
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 >
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)
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 :)
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.
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; }
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
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.
On Thu, Feb 06, 2020 at 07:30:51AM +0800, Baoquan He wrote: >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. Sure :-)
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; + } } - - /* - * 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); }
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 <akpm@linux-foundation.org> Cc: Oscar Salvador <osalvador@suse.de> Cc: Michal Hocko <mhocko@suse.com> Cc: David Hildenbrand <david@redhat.com> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Wei Yang <richardw.yang@linux.intel.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- mm/memory_hotplug.c | 34 +++++++--------------------------- 1 file changed, 7 insertions(+), 27 deletions(-)