Re: [PATCH v6 05/11] mm: Add fast_only bool to test_young and clear_young MMU notifiers

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Aug 07, 2024 at 08:02:26AM -0700, James Houghton wrote:
> On Tue, Aug 6, 2024 at 10:23 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Thu, Aug 01, 2024 at 04:13:40PM -0700, James Houghton wrote:
> > > --- a/include/linux/mmu_notifier.h
> > > +++ b/include/linux/mmu_notifier.h
> > > @@ -106,6 +106,18 @@ struct mmu_notifier_ops {
> > >          * clear_young is a lightweight version of clear_flush_young. Like the
> > >          * latter, it is supposed to test-and-clear the young/accessed bitflag
> > >          * in the secondary pte, but it may omit flushing the secondary tlb.
> > > +        *
> > > +        * The fast_only parameter indicates that this call should not block,
> > > +        * and this function should not cause other MMU notifier calls to
> > > +        * block. Usually this means that the implementation should be
> > > +        * lockless.
> > > +        *
> > > +        * When called with fast_only, this notifier will be a no-op unless
> > > +        * has_fast_aging is set on the struct mmu_notifier.
> >
> > If you add a has_fast_aging I wonder if it is better to introduce new
> > ops instead? The semantics are a bit easier to explain that way
> 
> v5 implemented these with a new op[1]. *Just* having the new op is
> kind of problematic -- we have yet another op to do something very
> similar to what already exists. We are left with two options:
> consolidate everything into a single notifier[2] or add a new
> parameter to test/clear_young()[3]. The latter, implemented in this
> v6, is somewhat simpler to implement (fewer LoC, reduces some
> duplication in KVM), though it does indeed make the explanation for
> test/clear_young() slightly more complex. I don't feel very strongly,
> but unless you do, I think I just ought to stick with how the v6 does
> it. :)

If it does makes the code simpler then it is probably the better choice

Jason