On Thu, 4 Jul 2024, Kefeng Wang wrote:
> On 2024/7/4 11:21, Hugh Dickins wrote:
> > On Wed, 3 Jul 2024, Andrew Morton wrote:
> >> On Tue, 2 Jul 2024 00:40:55 -0700 (PDT) Hugh Dickins <hughd@xxxxxxxxxx>
> >> wrote:
> >>
> ...
> >
> > And perhaps a conflict with another one of Kefeng's, which deletes a hunk
> > in mm/migrate.c just above where I add a hunk: and that's indeed how it
> > should end up, hunk deleted by Kefeng, hunk added by me.
> >
> >>
> >> --- mm/memcontrol.c~mm-refactor-folio_undo_large_rmappable
> >> +++ mm/memcontrol.c
> >> @@ -7832,8 +7832,7 @@ void mem_cgroup_migrate(struct folio *ol
> >> * In addition, the old folio is about to be freed after migration, so
> >> * removing from the split queue a bit earlier seems reasonable.
> >> */
> >> - if (folio_test_large(old) && folio_test_large_rmappable(old))
> >> - folio_undo_large_rmappable(old);
> >> + folio_undo_large_rmappable(old);
> >> old->memcg_data = 0;
> >> }
> >>
> >> I'm resolving this by simply dropping the above hunk. So Kefeng's
> >> patch is now as below. Please check.
> >
> > Checked, and that is correct, thank you Andrew. Correct, but not quite
> > complete: because I'm sure that if Kefeng had written his patch after
> > mine, he would have made the equivalent change in mm/migrate.c:
> >
>
> Yes,
>
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -443,8 +443,7 @@ int folio_migrate_mapping(struct address_space *mapping,
> > }
> >
> > /* Take off deferred split queue while frozen and memcg set */
> > - if (folio_test_large(folio) && folio_test_large_rmappable(folio))
> > - folio_undo_large_rmappable(folio);
> > + folio_undo_large_rmappable(folio);
> >
> > /*
> > * Now we know that no one else is looking at the folio:
> >
> > But there's no harm done if you push out a tree without that additional
> > mod: we can add it as a fixup afterwards, it's no more than a cleanup.
> >
> Maybe we could convert to __folio_undo_large_rmappable() for !maping part,
> which will avoid unnecessary freeze/unfreeze for empty deferred
> list.
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index ca65f03acb31..e6af9c25c25b 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -412,10 +412,11 @@ static int __folio_migrate_mapping(struct address_space
> *mapping,
> if (!mapping) {
> /* Take off deferred split queue while frozen and memcg set */
> if (folio_test_large(folio) &&
> - folio_test_large_rmappable(folio)) {
> + folio_test_large_rmappable(folio) &&
> + !data_race(list_empty(&folio->_deferred_list))) {
> if (!folio_ref_freeze(folio, expected_count))
> return -EAGAIN;
> - folio_undo_large_rmappable(folio);
> + __folio_undo_large_rmappable(folio);
> folio_ref_unfreeze(folio, expected_count);
> }
>
What you show above is exactly what I had when I was originally testing
over the top of mm-everything (well, not quite exactly, I don't think I
bothered with the data_race()). But I grew to feel that probably everyone
else would be happier with less of those internals _deferred_list and
__folio_undo_large_rmappable() spread about.
There are many ways to play it. I had also considered doing it Zi Yan's
way, freezing always in the !mapping case as well as in the mapping case:
what overhead it adds would probably get lost amidst all the other overhead
of page migration. It will not be surprising if changes come later requiring
us always to freeze in the anon !swapcache case too, it always seemed a bit
surprising not to need freezing there. But for now I decided it's best to
keep the freezing to the case where it's known to be needed (but without
getting into __s).
Many ways to play it, and I've no objection if someone then changes it
around later; but we've no need to depart from what Andrew already has.
Except, he did ask one of us to send along the -fix removing the unnecessary
checks before its second folio_undo_large_rmappable() once your refactor
patch goes in: here it is below.
[I guess this is the wrong place to say so, but folio_undo_large_rmappable()
is a dreadful name: it completely obscures what the function actually does,
and gives the false impression that the folio would be !large_rmappable
afterwards. I hope that one day the name gets changed to something like
folio_unqueue_deferred_split() or folio_cancel_deferred_split().]
[PATCH] mm: refactor folio_undo_large_rmappable() fix
Now that folio_undo_large_rmappable() is an inline function checking
order and large_rmappable for itself (and __folio_undo_large_rmappable()
is now declared even when CONFIG_TRANASPARENT_HUGEPAGE is off) there is
no need for folio_migrate_mapping() to check large and large_rmappable
first (in the mapping case when it has had to freeze anyway).
Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>
---
For folding in to mm-unstable's "mm: refactor folio_undo_large_rmappable()",
unless I'm too late and it's already mm-stable (no problem, just a cleanup).
mm/migrate.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -443,8 +443,7 @@ int folio_migrate_mapping(struct address_space *mapping,
}
/* Take off deferred split queue while frozen and memcg set */
- if (folio_test_large(folio) && folio_test_large_rmappable(folio))
- folio_undo_large_rmappable(folio);
+ folio_undo_large_rmappable(folio);
/*
* Now we know that no one else is looking at the folio:
--
2.35.3
