Re: [PATCH] mm, compaction: fix fast_isolate_around() to stay within boundaries

Re: [PATCH] mm, compaction: fix fast_isolate_around() to stay within boundaries

[Andrew Morton]

On Wed, 26 Oct 2022 20:24:38 +0900 NARIBAYASHI Akira <a.naribayashi@fujitsu.com> wrote:

> Depending on the memory configuration, isolate_freepages_block() may
> scan pages out of the target range and causes panic.
> 
> The problem is that pfn as argument of fast_isolate_around() could
> be out of the target range. Therefore we should consider the case
> where pfn < start_pfn, and also the case where end_pfn < pfn.
> 
> This problem should have been addressd by the commit 6e2b7044c199
> ("mm, compaction: make fast_isolate_freepages() stay within zone")
> but there was an oversight.
> 
>  Case1: pfn < start_pfn
> 
>   <at memory compaction for node Y>
>   |  node X's zone  | node Y's zone
>   +-----------------+------------------------------...
>    pageblock    ^   ^     ^
>   +-----------+-----------+-----------+-----------+...
>                 ^   ^     ^
>                 ^   ^      end_pfn
>                 ^    start_pfn = cc->zone->zone_start_pfn
>                  pfn
>                 <---------> scanned range by "Scan After"
> 
>  Case2: end_pfn < pfn
> 
>   <at memory compaction for node X>
>   |  node X's zone  | node Y's zone
>   +-----------------+------------------------------...
>    pageblock  ^     ^   ^
>   +-----------+-----------+-----------+-----------+...
>               ^     ^   ^
>               ^     ^    pfn
>               ^      end_pfn
>                start_pfn
>               <---------> scanned range by "Scan Before"
> 
> It seems that there is no good reason to skip nr_isolated pages
> just after given pfn. So let perform simple scan from start to end
> instead of dividing the scan into "Before" and "After".

Under what circumstances will this panic occur?  I assume those
circumstnces are pretty rare, give that 6e2b7044c1992 was nearly two
years ago.

Did you consider the desirability of backporting this fix into earlier
kernels?

Re: [PATCH] mm, compaction: fix fast_isolate_around() to stay within boundaries

[Mel Gorman]

On Thu, Oct 27, 2022 at 01:25:57PM -0700, Andrew Morton wrote:
> On Wed, 26 Oct 2022 20:24:38 +0900 NARIBAYASHI Akira <a.naribayashi@fujitsu.com> wrote:
> 
> > Depending on the memory configuration, isolate_freepages_block() may
> > scan pages out of the target range and causes panic.
> > 
> > The problem is that pfn as argument of fast_isolate_around() could
> > be out of the target range. Therefore we should consider the case
> > where pfn < start_pfn, and also the case where end_pfn < pfn.
> > 
> > This problem should have been addressd by the commit 6e2b7044c199
> > ("mm, compaction: make fast_isolate_freepages() stay within zone")
> > but there was an oversight.
> > 
> >  Case1: pfn < start_pfn
> > 
> >   <at memory compaction for node Y>
> >   |  node X's zone  | node Y's zone
> >   +-----------------+------------------------------...
> >    pageblock    ^   ^     ^
> >   +-----------+-----------+-----------+-----------+...
> >                 ^   ^     ^
> >                 ^   ^      end_pfn
> >                 ^    start_pfn = cc->zone->zone_start_pfn
> >                  pfn
> >                 <---------> scanned range by "Scan After"
> > 
> >  Case2: end_pfn < pfn
> > 
> >   <at memory compaction for node X>
> >   |  node X's zone  | node Y's zone
> >   +-----------------+------------------------------...
> >    pageblock  ^     ^   ^
> >   +-----------+-----------+-----------+-----------+...
> >               ^     ^   ^
> >               ^     ^    pfn
> >               ^      end_pfn
> >                start_pfn
> >               <---------> scanned range by "Scan Before"
> > 
> > It seems that there is no good reason to skip nr_isolated pages
> > just after given pfn. So let perform simple scan from start to end
> > instead of dividing the scan into "Before" and "After".
> 
> Under what circumstances will this panic occur? 

I'd also like to see a warning or oops report combined with the
/proc/zoneinfo file of the machine affected. This is to confirm it's an
actual bug and not a suspicion based on code inspection and a simplification
of the code. The answer determines whether this is a -stable candidate
or not.

Both Case 1 and 2 require that the initial pfn started outside the zone
which is unexpected. The clamping on zone boundary in fast_isolate_aropund()
is happening due to pageblock alignment as there is no guarantee that zones
are aligned on a hugepage boundary. pfn itself should have been fine as
it is the PFN of a page that was recently isolated.

The Scan After logic should also be ok. In the context it's called,
nr_isolated is the number of pages that were just isolated so 
pfn + nr_isolated is the end of the free page that was just isolated.

The patch itself should be functionally fine but it rescans a region that has
already been isolated which is a little wasteful but it is straight-forward
and the overhead is probably negligible.

-- 
Mel Gorman
SUSE Labs

RE: [PATCH] mm, compaction: fix fast_isolate_around() to stay within boundaries

[Akira Naribayashi (Fujitsu)]

Sorry, I may not have sent the email correctly. 
I will resend it.

On Thu, 27 Oct 2022 20:26:04 +0000 Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 26 Oct 2022 20:24:38 +0900 NARIBAYASHI Akira <a.naribayashi@fujitsu.com> wrote:
> 
> > Depending on the memory configuration, isolate_freepages_block() may
> > scan pages out of the target range and causes panic.
> > 
> > The problem is that pfn as argument of fast_isolate_around() could
> > be out of the target range. Therefore we should consider the case
> > where pfn < start_pfn, and also the case where end_pfn < pfn.
> > 
> > This problem should have been addressd by the commit 6e2b7044c199
> > ("mm, compaction: make fast_isolate_freepages() stay within zone")
> > but there was an oversight.
> > 
> >  Case1: pfn < start_pfn
> > 
> >   <at memory compaction for node Y>
> >   |  node X's zone  | node Y's zone
> >   +-----------------+------------------------------...
> >    pageblock    ^   ^     ^
> >   +-----------+-----------+-----------+-----------+...
> >                 ^   ^     ^
> >                 ^   ^      end_pfn
> >                 ^    start_pfn = cc->zone->zone_start_pfn
> >                  pfn
> >                 <---------> scanned range by "Scan After"
> > 
> >  Case2: end_pfn < pfn
> > 
> >   <at memory compaction for node X>
> >   |  node X's zone  | node Y's zone
> >   +-----------------+------------------------------...
> >    pageblock  ^     ^   ^
> >   +-----------+-----------+-----------+-----------+...
> >               ^     ^   ^
> >               ^     ^    pfn
> >               ^      end_pfn
> >                start_pfn
> >               <---------> scanned range by "Scan Before"
> > 
> > It seems that there is no good reason to skip nr_isolated pages
> > just after given pfn. So let perform simple scan from start to end
> > instead of dividing the scan into "Before" and "After".
> 
> Under what circumstances will this panic occur?  I assume those
> circumstnces are pretty rare, give that 6e2b7044c1992 was nearly two
> years ago.
> 
> Did you consider the desirability of backporting this fix into earlier
> kernels?


Panic can occur on systems with multiple zones in a single pageblock.

The reason it is rare is that it only happens in special configurations.
Depending on how many similar systems there are, it may be a good idea to fix this problem for older kernels as well.

Re: [PATCH] mm, compaction: fix fast_isolate_around() to stay within boundaries

[Mel Gorman]

On Mon, Nov 07, 2022 at 12:32:34PM +0000, Akira Naribayashi (Fujitsu) wrote:
> > Under what circumstances will this panic occur?  I assume those
> > circumstnces are pretty rare, give that 6e2b7044c1992 was nearly two
> > years ago.
> > 
> > Did you consider the desirability of backporting this fix into earlier
> > kernels?
> 
> 
> Panic can occur on systems with multiple zones in a single pageblock.
> 

Please provide an example of the panic and the zoneinfo.

> The reason it is rare is that it only happens in special configurations.

How is this special configuration created?

-- 
Mel Gorman
SUSE Labs

RE: [PATCH] mm, compaction: fix fast_isolate_around() to stay within boundaries

[Akira Naribayashi (Fujitsu)]

On Mon, 7 Nov 2022 15:43:56 +0000, Mei Gorman wrote:
> On Mon, Nov 07, 2022 at 12:32:34PM +0000, Akira Naribayashi (Fujitsu) wrote:
> > > Under what circumstances will this panic occur?  I assume those
> > > circumstnces are pretty rare, give that 6e2b7044c1992 was nearly two
> > > years ago.
> > > 
> > > Did you consider the desirability of backporting this fix into earlier
> > > kernels?
> > 
> > 
> > Panic can occur on systems with multiple zones in a single pageblock.
> > 
> 
> Please provide an example of the panic and the zoneinfo.

This issue is occurring in our customer's environment and cannot 
be shared publicly as it contains customer information.
Also, the panic is occurring with the kernel in RHEL and may not 
panic with Upstream's community kernel.
In other words, it is possible to panic on older kernels.
I think this fix should be backported to stable kernel series.

> > The reason it is rare is that it only happens in special configurations.
> 
> How is this special configuration created?

This is the case when the node boundary is not aligned to pageblock boundary.

Re: [PATCH] mm, compaction: fix fast_isolate_around() to stay within boundaries

[Mel Gorman]

On Wed, Nov 09, 2022 at 05:41:12AM +0000, Akira Naribayashi (Fujitsu) wrote:
> On Mon, 7 Nov 2022 15:43:56 +0000, Mei Gorman wrote:
> > On Mon, Nov 07, 2022 at 12:32:34PM +0000, Akira Naribayashi (Fujitsu) wrote:
> > > > Under what circumstances will this panic occur?  I assume those
> > > > circumstnces are pretty rare, give that 6e2b7044c1992 was nearly two
> > > > years ago.
> > > > 
> > > > Did you consider the desirability of backporting this fix into earlier
> > > > kernels?
> > > 
> > > 
> > > Panic can occur on systems with multiple zones in a single pageblock.
> > > 
> > 
> > Please provide an example of the panic and the zoneinfo.
> 
> This issue is occurring in our customer's environment and cannot 
> be shared publicly as it contains customer information.
> Also, the panic is occurring with the kernel in RHEL and may not 
> panic with Upstream's community kernel.
> In other words, it is possible to panic on older kernels.
> I think this fix should be backported to stable kernel series.
> 
> > > The reason it is rare is that it only happens in special configurations.
> > 
> > How is this special configuration created?
> 
> This is the case when the node boundary is not aligned to pageblock boundary.

In that case, does this work to avoid rescanning an area that was already
isolated?

diff --git a/mm/compaction.c b/mm/compaction.c
index c51f7f545afe..58cf73ff20ff 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1346,7 +1346,7 @@ move_freelist_tail(struct list_head *freelist, struct page *freepage)
 static void
 fast_isolate_around(struct compact_control *cc, unsigned long pfn, unsigned long nr_isolated)
 {
-	unsigned long start_pfn, end_pfn;
+	unsigned long start_pfn, end_pfn, isolated_end;
 	struct page *page;
 
 	/* Do not search around if there are enough pages already */
@@ -1361,6 +1361,10 @@ fast_isolate_around(struct compact_control *cc, unsigned long pfn, unsigned long
 	start_pfn = max(pageblock_start_pfn(pfn), cc->zone->zone_start_pfn);
 	end_pfn = min(pageblock_end_pfn(pfn), zone_end_pfn(cc->zone));
 
+	/* Pageblock may straddle zone/node boundaries */
+	isolated_end = pfn + nr_isolated;
+	pfn = clamp(pfn, start_pfn, end_pfn);
+
 	page = pageblock_pfn_to_page(start_pfn, end_pfn, cc->zone);
 	if (!page)
 		return;
@@ -1373,7 +1377,7 @@ fast_isolate_around(struct compact_control *cc, unsigned long pfn, unsigned long
 	}
 
 	/* Scan after */
-	start_pfn = pfn + nr_isolated;
+	start_pfn = isolated_end;
 	if (start_pfn < end_pfn)
 		isolate_freepages_block(cc, &start_pfn, end_pfn, &cc->freepages, 1, false);
 

RE: [PATCH] mm, compaction: fix fast_isolate_around() to stay within boundaries

[Akira Naribayashi (Fujitsu)]

On Wed, 23 Nov 2022 10:26:05 +0000, Mei Gorman wrote:
> On Wed, Nov 09, 2022 at 05:41:12AM +0000, Akira Naribayashi (Fujitsu) wrote:
> > On Mon, 7 Nov 2022 15:43:56 +0000, Mei Gorman wrote:
> > > On Mon, Nov 07, 2022 at 12:32:34PM +0000, Akira Naribayashi (Fujitsu) wrote:
> > > > > Under what circumstances will this panic occur?  I assume those
> > > > > circumstnces are pretty rare, give that 6e2b7044c1992 was nearly two
> > > > > years ago.
> > > > > 
> > > > > Did you consider the desirability of backporting this fix into earlier
> > > > > kernels?
> > > > 
> > > > 
> > > > Panic can occur on systems with multiple zones in a single pageblock.
> > > > 
> > > 
> > > Please provide an example of the panic and the zoneinfo.
> > 
> > This issue is occurring in our customer's environment and cannot 
> > be shared publicly as it contains customer information.
> > Also, the panic is occurring with the kernel in RHEL and may not 
> > panic with Upstream's community kernel.
> > In other words, it is possible to panic on older kernels.
> > I think this fix should be backported to stable kernel series.
> > 
> > > > The reason it is rare is that it only happens in special configurations.
> > > 
> > > How is this special configuration created?
> > 
> > This is the case when the node boundary is not aligned to pageblock boundary.
> 
> In that case, does this work to avoid rescanning an area that was already
> isolated?

In the case of your patch, I think I need to clamp the isolated_end as well.
Because sometimes isolated_end < start_pfn(value before entering Scan after) < end_pfn.

After re-reading the source, I think the problem is that min_pfn and low_pfn
can be out of range in fast_isolate_freepages.
How about the following patch?

diff --git a/mm/compaction.c b/mm/compaction.c
index 1f6da31dd9a5..b67b82bb4944 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1436,6 +1436,11 @@ fast_isolate_freepages(struct compact_control *cc)
        if (WARN_ON_ONCE(min_pfn > low_pfn))
                low_pfn = min_pfn;

+       if (min_pfn < cc->migrate_pfn)
+               min_pfn = cc->migrate_pfn;
+       if (low_pfn < cc->migrate_pfn)
+               low_pfn = cc->migrate_pfn;
+
        /*
         * Search starts from the last successful isolation order or the next
         * order to search after a previous failure 

Re: [PATCH] mm, compaction: fix fast_isolate_around() to stay within boundaries

[Mel Gorman]

On Fri, Dec 09, 2022 at 09:19:37AM +0000, Akira Naribayashi (Fujitsu) wrote:
> On Wed, 23 Nov 2022 10:26:05 +0000, Mei Gorman wrote:
> > On Wed, Nov 09, 2022 at 05:41:12AM +0000, Akira Naribayashi (Fujitsu) wrote:
> > > On Mon, 7 Nov 2022 15:43:56 +0000, Mei Gorman wrote:
> > > > On Mon, Nov 07, 2022 at 12:32:34PM +0000, Akira Naribayashi (Fujitsu) wrote:
> > > > > > Under what circumstances will this panic occur?  I assume those
> > > > > > circumstnces are pretty rare, give that 6e2b7044c1992 was nearly two
> > > > > > years ago.
> > > > > > 
> > > > > > Did you consider the desirability of backporting this fix into earlier
> > > > > > kernels?
> > > > > 
> > > > > 
> > > > > Panic can occur on systems with multiple zones in a single pageblock.
> > > > > 
> > > > 
> > > > Please provide an example of the panic and the zoneinfo.
> > > 
> > > This issue is occurring in our customer's environment and cannot 
> > > be shared publicly as it contains customer information.
> > > Also, the panic is occurring with the kernel in RHEL and may not 
> > > panic with Upstream's community kernel.
> > > In other words, it is possible to panic on older kernels.
> > > I think this fix should be backported to stable kernel series.
> > > 
> > > > > The reason it is rare is that it only happens in special configurations.
> > > > 
> > > > How is this special configuration created?
> > > 
> > > This is the case when the node boundary is not aligned to pageblock boundary.
> > 
> > In that case, does this work to avoid rescanning an area that was already
> > isolated?
> 
> In the case of your patch, I think I need to clamp the isolated_end as well.
> Because sometimes isolated_end < start_pfn(value before entering Scan after) < end_pfn.
> 
> After re-reading the source, I think the problem is that min_pfn and low_pfn
> can be out of range in fast_isolate_freepages.
> How about the following patch?
> 

Ok, makes sense and it is a condition that could happen because of pageblock
alignment.

-- 
Mel Gorman
SUSE Labs