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

[Ralph Campbell <rcampbell@xxxxxxxxxx>]

On 11/13/19 8:46 AM, Jason Gunthorpe wrote:
> On Wed, Nov 13, 2019 at 05:59:52AM -0800, Christoph Hellwig wrote:
> > > +int mmu_interval_notifier_insert(struct mmu_interval_notifier *mni,
> > > +                                     struct mm_struct *mm, unsigned long start,
> > > +                                     unsigned long length,
> > > +                                     const struct mmu_interval_notifier_ops 
> > > *ops);
> > > +int mmu_interval_notifier_insert_locked(
> > > +       struct mmu_interval_notifier *mni, struct mm_struct *mm,
> > > +       unsigned long start, unsigned long length,
> > > +       const struct mmu_interval_notifier_ops *ops);
> > Very inconsistent indentation between these two related functions.
> clang-format.. The kernel config is set to prefer a line up under the
> ( if all the arguments will fit within the 80 cols otherwise it does a
> 1 tab continuation indent.
>
> > > +       /*
> > > +        * The inv_end incorporates a deferred mechanism like
> > > +        * rtnl_unlock(). Adds and removes are queued until the final inv_end
> > > +        * happens then they are progressed. This arrangement for tree updates
> > > +        * is used to avoid using a blocking lock during
> > > +        * invalidate_range_start.
> > Nitpick:  That comment can be condensed into one less line:
> The rtnl_unlock can move up a line too. My editor is failing me on
> this.
>
> > > +       /*
> > > +        * TODO: Since we already have a spinlock above, this would be faster
> > > +        * as wake_up_q
> > > +        */
> > > +       if (need_wake)
> > > +               wake_up_all(&mmn_mm->wq);
> > So why is this important enough for a TODO comment, but not important
> > enough to do right away?
> Lets drop the comment, I'm noto sure wake_up_q is even a function this
> layer should be calling.
Actually, I think you can remove the "need_wake" variable since it is
unconditionally set to "true".

Also, the comment in__mmu_interval_notifier_insert() says
"mni->mr_invalidate_seq" and I think that should be
"mni->invalidate_seq".

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