Re: [PATCH 20/25] s390/fbatch: no lru_add_drain_all() in s390_wiggle_split_folio()

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Thu, 27 Aug 2026 01:49:12 -0700 (PDT)
Hugh Dickins <hughd@google.com> wrote:

> On Wed, 26 Aug 2026, Claudio Imbrenda wrote:
> > On Mon, 24 Aug 2026 07:39:12 -0700 (PDT)
> > Hugh Dickins <hughd@google.com> wrote:
> >   
> > > s390_wiggle_split_folio() has no good reason to lru_add_drain_all(),
> > > now that the per-cpu fbatch references are gone.
> > > 
> > > Signed-off-by: Hugh Dickins <hughd@google.com>
> > > ---
> > >  arch/s390/kernel/uv.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> > > index dc14ebc0105b..120a467026a5 100644
> > > --- a/arch/s390/kernel/uv.c
> > > +++ b/arch/s390/kernel/uv.c
> > > @@ -364,7 +364,6 @@ int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio)
> > >  
> > >  	lockdep_assert_not_held(&mm->mmap_lock);
> > >  	folio_wait_writeback(folio);
> > > -	lru_add_drain_all();
> > >  
> > >  	if (!folio_test_large(folio))
> > >  		return 0;  
> > 
> > This is black magic for me, I am not sure I fully understand all the
> > details, but what's the new purpose of lru_add_drain_all() ?
> > 
> > will we have a guarantee that no stray references to mapped folios will
> > ever remain?
> > 
> > Any unexpected reference (i.e. not due to mappings, see
> > expected_folio_refs()) will cause a protected guest to hang.  
> 
> I most certanly don't know s390 or that code well enough to guarantee
> you that no stray references to mapped folios can remain there. What I
> can guarantee is that no references, of the kind which lru_add_drain_all()
> used to be needed to remove, can exist there: so there will no longer
> be any point in s390 (or others) calling it for that reason, to help
> split_folio() to succeed.

we are not using it to help split_folio() succeed (although that's a
pleasant side effect). We need it even for small pages, to guarantee
that no extra reference from LRU is present on the page.

you just mentioned that, with this patch series, no such references
will be there, so that would be enough for me

> 
> You wonder then, what lru_add_drain_all()'s new purpose is, why it
> still exists at all? I did hope to remove it completely, but found
> two usages that I could not argue against: one is in user-forced page
> reclaim (two memcg interfaces and a sysfs interface), where it's
> still desirable to push folios on to the immediately reclaimable LRUs,
> rather than leave any on the per-cpu fbatches preceding those LRUs;
> the other is in memory hotremove, where it will be necessary to erase
> stray addresses, through which a subsequent folio_try_get() might have
> accessed a struct folio which (I imagine) might have been freed.
> 
> Yes, your split_folio() may still occasionally fail, because of
> transient references and folio_try_get()s on that folio; but that's
> so before and after the changes. And there is (in my mind anyway) an
> open question of whether "folio_try_get() blips" will be visible a
> little more than before.

yes, it's fine if there are transient fails, as long as this won't
block indefinitely (or for extended amounts of time)

> 
> Hmm, looking again at s390_wiggle_split_folio(), it seems rather
> odd that it was doing an lru_add_drain_all() at all: because any
> large (hence splittable) folios have themselves been immediately
> flushed from the per-cpu fbatches, not left queued up there. Maybe

yes, because as I mentioned above, we are not using it for
split_folio(), but to guarantee that no extra references are present.

We count how many references are present, how many we are expecting,
and if any extra are present, we do the lru drain. If after the drain
we still have extra references, then we try again, in the hope that the
extra references go away quickly (i.e. we expect the extra references
to be due to I/O)

when a page transitions from "normal" to "secure-guest owned", we must
make sure that no extra references are present.

> there was an earlier time when mm did not enforce that; and Barry
> is currently looking to relax that, so the limitation intended for
> pmd-sized folios is no longer forced on the smallest large folios.
> 
> If Barry's relaxation goes in before my drainage changes, then
> there is value in that s390 lru_add_drain_all() in the interim.

hmmm so, should it stay for now, then?

also: I'm working on completely reworking how the transition from
non-secure to secure is handled, with the explicit goal of getting rid
of that kludge we are currently using. That will also get rid of the
lru drain. But it will take some time (I hope to have something by the
end of the year)