On Wed, Dec 6, 2023 at 10:54 PM Baolin Wang
<baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
>
>
>
> On 11/29/2023 6:45 PM, Barry Song wrote:
> > Testing shows fast_isolate_freepages can blindly choose an unsuitable
> > pageblock from time to time particularly while the min mark is used
> > from XXX path:
> > if (!page) {
> > cc->fast_search_fail++;
> > if (scan_start) {
> > /*
> > * Use the highest PFN found above min. If one was
> > * not found, be pessimistic for direct compaction
> > * and use the min mark.
> > */
> > if (highest >= min_pfn) {
> > page = pfn_to_page(highest);
> > cc->free_pfn = highest;
> > } else {
> > if (cc->direct_compaction && pfn_valid(min_pfn)) { /* XXX */
> > page = pageblock_pfn_to_page(min_pfn,
> > min(pageblock_end_pfn(min_pfn),
> > zone_end_pfn(cc->zone)),
> > cc->zone);
> > cc->free_pfn = min_pfn;
> > }
> > }
> > }
> > }
>
> Yes, the min_pfn can be an unsuitable migration target. But I think we
> can just add the suitable_migration_target() validation into 'min_pfn'
> case? Since other cases must be suitable target which found from
> MIGRATE_MOVABLE free list. Something like below:
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 01ba298739dd..4e8eb4571909 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1611,6 +1611,8 @@ static void fast_isolate_freepages(struct
> compact_control *cc)
>
> min(pageblock_end_pfn(min_pfn),
>
> zone_end_pfn(cc->zone)),
> cc->zone);
> + if
> (!suitable_migration_target(cc, page))
> + page = NULL;
> cc->free_pfn = min_pfn;
> }
> }
>
yes. this makes more senses.
> By the way, I wonder if this patch can improve the efficiency of
> compaction in your test case?
This happens not quite often. when running 25 machines for
one night, most of them can hit this unexpected code path.
but the frequency isn't many times in one second. it might
be one time in a couple of hours.
so it is very difficult to measure the visible performance impact
in my machines though the affection of choosing the unsuitable
migration_target should be negative.
I feel like it's worth fixing this to at least make the code theoretically
self-explanatory? as it is quite odd unsuitable_migration_target can
be still migration_target?
>
> > In contrast, slow path is skipping unsuitable pageblocks in a decent way.
> >
> > I don't know if it is an intended design or just an oversight. But
> > it seems more sensible to skip unsuitable pageblock.
> >
> > Reported-by: Zhanyuan Hu <huzhanyuan@xxxxxxxx>
> > Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
> > ---
> > mm/compaction.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 01ba298739dd..98c485a25614 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -1625,6 +1625,12 @@ static void fast_isolate_freepages(struct compact_control *cc)
> > cc->total_free_scanned += nr_scanned;
> > if (!page)
> > return;
> > + /*
> > + * Otherwise, we can blindly choose an improper pageblock especially
> > + * while using the min mark
> > + */
> > + if (!suitable_migration_target(cc, page))
> > + return;
> >
> > low_pfn = page_to_pfn(page);
> > fast_isolate_around(cc, low_pfn);
Thanks
Barry
