| Message ID | 20241102201621.95291-1-liuq131@chinatelecom.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/compaction: fix the total_isolated in strict mode | expand |
On Sat, 2 Nov 2024 20:16:21 +0000 Qiang Liu <liuq131@chinatelecom.cn> wrote: > If the last cycle reads bogus compound_order() and blockpfn > end_pfn occurs, > it is possible that total_isolated will be less than nr_scanned. In this case, > strict mode should return 0, but the “if (strict && blockpfn < end_pfn)” > statement cannot recognize this situation > > ... > > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -699,7 +699,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, > * pages requested were isolated. If there were any failures, 0 is > * returned and CMA will fail. > */ > - if (strict && blockpfn < end_pfn) > + if (strict && (blockpfn < end_pfn || total_isolated != nr_scanned)) > total_isolated = 0; > > cc->total_free_scanned += nr_scanned; That's really old code. What userspace-visible effects might this have? Is this from code inspection, or was some misbehaviour observed? Thanks.
On 2024/11/3 04:16, Qiang Liu wrote: > If the last cycle reads bogus compound_order() and blockpfn > end_pfn occurs, if blockpfn > end_pfn occurs, we will reset the blockpfn, right? /* * Be careful to not go outside of the pageblock. */ if (unlikely(blockpfn > end_pfn)) blockpfn = end_pfn; So how this can happen? > it is possible that total_isolated will be less than nr_scanned. In this case, > strict mode should return 0, but the “if (strict && blockpfn < end_pfn)” > statement cannot recognize this situation > > Signed-off-by: Qiang Liu <liuq131@chinatelecom.cn> > --- > mm/compaction.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index a2b16b08cbbf..6009f5d1021a 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -699,7 +699,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, > * pages requested were isolated. If there were any failures, 0 is > * returned and CMA will fail. > */ > - if (strict && blockpfn < end_pfn) > + if (strict && (blockpfn < end_pfn || total_isolated != nr_scanned)) > total_isolated = 0; > > cc->total_free_scanned += nr_scanned;
On 2024/11/12 10:16, liuq131@chinatelecom.cn wrote: > "We assume that the block we are currently processing is distributed as follows: > 0 1 2 511 > -------------------------------------------------- > | | | | > --------------------------------------------------- > Index 0 and 1 are both pages with an order of 0. > Index 2 has a bogus order (let's assume the order is 9). > When the for loop reaches index 2, it will enter the following code: > /* > * For compound pages such as THP and hugetlbfs, we can save > * potentially a lot of iterations if we skip them at once. > * The check is racy, but we can consider only valid values > * and the only danger is skipping too much. > */ > if (PageCompound(page)) { > const unsigned int order = compound_order(page); > if (blockpfn + (1UL << order) <= end_pfn) { > blockpfn += (1UL << order) - 1; > page += (1UL << order) - 1; > nr_scanned += (1UL << order) - 1; > } > goto isolate_fail; > } > > After exiting the for loop: > blockpfn =basepfn+ 2+2^9 = basepfn+514 > endpfn = basepfn +512 > total_isolated = 2 > nr_scanned = 514 In your case, the 'blockpfn' will not be updated to 'basepfn+514', because 'blockpfn + (1UL << order) > end_pfn', right? And remember the 'end_pfn' is the end of the pageblock. So I'm still confused about your case. Is this from code inspection? > /* > * Be careful to not go outside of the pageblock. > */ > if (unlikely(blockpfn > end_pfn)) > blockpfn = end_pfn; > > So this can happen > > /* > * If strict isolation is requested by CMA then check that all the > * pages requested were isolated. If there were any failures, 0 is > * returned and CMA will fail. > */ > if (strict && blockpfn < end_pfn) > total_isolated = 0; > > If processed according to the old code, it will not enter the if statement to reset total_isolated, but the correct handling is to reset total_isolated to 0. Please do not top-posting: " - Use interleaved ("inline") replies, which makes your response easier to read. (i.e. avoid top-posting -- the practice of putting your answer above the quoted text you are responding to.) For more details, see :ref:`Documentation/process/submitting-patches.rst <interleaved_replies>`. "
diff --git a/mm/compaction.c b/mm/compaction.c index a2b16b08cbbf..6009f5d1021a 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -699,7 +699,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, * pages requested were isolated. If there were any failures, 0 is * returned and CMA will fail. */ - if (strict && blockpfn < end_pfn) + if (strict && (blockpfn < end_pfn || total_isolated != nr_scanned)) total_isolated = 0; cc->total_free_scanned += nr_scanned;
If the last cycle reads bogus compound_order() and blockpfn > end_pfn occurs, it is possible that total_isolated will be less than nr_scanned. In this case, strict mode should return 0, but the “if (strict && blockpfn < end_pfn)” statement cannot recognize this situation Signed-off-by: Qiang Liu <liuq131@chinatelecom.cn> --- mm/compaction.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)