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
