RE: [PATCH v3] mm/memory hotplug/unplug: Optimize zone contiguous check when changing pfn range

[Liu, Yuan1]

[...]

> > The set_zone_contiguous/clear_zone_contiguous can be ignored I think.
> >
> > The comment about shrink_zone_span() is likely not realistic.
> > shrink_zone_span() would not shrink over boot holes.
> >
> > Well, unless we have an odd case where the hole+memory starts in the
> > middle of a "PAGES_PER_SUBSECTION". That would already be problematic if
> > memory starts/ends in the middle of a PAGES_PER_SUBSECTION chunk. I
> > don't such a case exists.
> >
> > We could improve shrink_zone_span() to let
> > find_smallest_section_pfn/find_biggest_section_pfn test the pfn_to_nid()
> > and page_zone() not on;y on the smallest/highest pfn, but also on the
> > highest/smallest PFN in a PAGES_PER_SUBSECTION chunk.
> >
> > No need to test pfn_to_online_page() twice, as that is the same result
> > for all pages in a PAGES_PER_SUBSECTION chunk.

Hi David

Sorry, I still have one question.

As I understand it, when a hole and memory fall within the same subsection,
we check the nid and zone on both sides of the subsection to avoid incorrect shrink. In this case, it seems the hole would always share the same zone and nid as the memory within that subsection.

I may be missing something, but I’m not entirely clear on the use case for this optimization. Could you help clarify?

Below is my proposed change. Please let me know if I’ve understood this
correctly.

--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -435,14 +435,20 @@ static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
                                     unsigned long start_pfn,
                                     unsigned long end_pfn)
 {
-       for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SUBSECTION) {
+       unsigned long next_pfn;
+
+       for (; start_pfn < end_pfn; start_pfn = next_pfn) {
+               next_pfn = start_pfn + PAGES_PER_SUBSECTION;
+
                if (unlikely(!pfn_to_online_page(start_pfn)))
                        continue;

-               if (unlikely(pfn_to_nid(start_pfn) != nid))
+               if (unlikely(pfn_to_nid(start_pfn) != nid &&
+                            pfn_to_nid(next_pfn - 1) != nid))
                        continue;

-               if (zone != page_zone(pfn_to_page(start_pfn)))
+               if (zone != page_zone(pfn_to_page(start_pfn)) &&
+                   zone != page_zone(pfn_to_page(next_pfn - 1)))
                        continue;

                return start_pfn;
@@ -457,17 +463,22 @@ static unsigned long find_biggest_section_pfn(int nid, struct zone *zone,
                                    unsigned long end_pfn)
 {
        unsigned long pfn;
+       unsigned long next_pfn;

        /* pfn is the end pfn of a memory section. */
        pfn = end_pfn - 1;
-       for (; pfn >= start_pfn; pfn -= PAGES_PER_SUBSECTION) {
+       for (; pfn >= start_pfn; pfn = next_pfn) {
+               next_pfn = pfn - PAGES_PER_SUBSECTION;
+
                if (unlikely(!pfn_to_online_page(pfn)))
                        continue;

-               if (unlikely(pfn_to_nid(pfn) != nid))
+               if (unlikely(pfn_to_nid(pfn) != nid &&
+                            pfn_to_nid(next_pfn + 1) != nid))
                        continue;

-               if (zone != page_zone(pfn_to_page(pfn)))
+               if (zone != page_zone(pfn_to_page(pfn)) &&
+                   zone != page_zone(pfn_to_page(next_pfn + 1)))
                        continue;

                return pfn;


> 
> > --
> > Cheers,
> >
> > David