Re: [Xen-devel] [PATCH v2 02/15] mm/mmu_notifier: add an interval tree notifier

[Jason Gunthorpe <jgg@xxxxxxxxxxxx>]

On Thu, Nov 07, 2019 at 12:53:56PM -0800, John Hubbard wrote:
> > > > +/**
> > > > + * struct mmu_range_notifier_ops
> > > > + * @invalidate: Upon return the caller must stop using any SPTEs 
> > > > within this
> > > > + *              range, this function can sleep. Return false if 
> > > > blocking was
> > > > + *              required but range is non-blocking
> > > > + */
> > > 
> > > How about this (I'm not sure I fully understand the return value, though):
> > > 
> > > /**
> > >   * struct mmu_range_notifier_ops
> > >   * @invalidate: Upon return the caller must stop using any SPTEs within 
> > > this
> > >   *               range.
> > >   *
> > >   *               This function is permitted to sleep.
> > >   *
> > >   *       @Return: false if blocking was required, but @range is
> > >   *                       non-blocking.
> > >   *
> > >   */
> > 
> > Is this kdoc format for function pointers?
> 
> heh, I'm sort of winging it, I'm not sure how function pointers are supposed
> to be documented in kdoc. Actually the only key take-away here is to write
> 
> "This function can sleep"
> 
> as a separate sentence..

Sure

> > This odd duality has already cause some confusion, but names here are
> > hard.  mmu_interval_notifier is the best alternative I've heard.
> > 
> > Changing this name is a lot of work - are we happy
> > 'mmu_interval_notifier' is the right choice? 
> 
> Yes, it's my favorite too. I'd vote for going with that.

Okay, lets give it a go

> Very nice, would you be open to putting that into (any) one of the comment
> headers? That's an unusually clear and concise description:

Yep, done

> > > > +int mmu_range_notifier_insert(struct mmu_range_notifier *mrn,
> > > > +                             unsigned long start, unsigned long length,
> > > > +                             struct mm_struct *mm)
> > > > +{
> > > > +       struct mmu_notifier_mm *mmn_mm;
> > > > +       int ret;
> > > 
> > > Hmmm, I think a later patch improperly changes the above to "int ret = 
> > > 0;".
> > > I'll check on that. It's correct here, though.
> > 
> > Looks OK in my tree?
> 
> Nope, that's how I found it. The top of your mmu_notifier branch has this:
> 
> int __mmu_notifier_invalidate_range_start(struct mmu_notifier_range *range)
> {
>         struct mmu_notifier_mm *mmn_mm = range->mm->mmu_notifier_mm;
>         int ret = 0;
> 
>         if (mmn_mm->has_interval) {
>                 ret = mn_itree_invalidate(mmn_mm, range);
>                 if (ret)
>                         return ret;
>         }
>         if (!hlist_empty(&mmn_mm->list))
>                 return mn_hlist_invalidate_range_start(mmn_mm, range);
>         return 0;
> }

Ah, that is a different function :) Fixed

> Looks good. We're just polishing up minor points now, so you can add:
> 
> Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>

Great, thanks, I'll post a v3 with the rename

Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel