[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.
 
> > +    * release semantics on the initialization of the mmu_notifier_mm's
> > +         * contents are provided for unlocked readers.  acquire can only be
> > +         * used while holding the mmgrab or mmget, and is safe because once
> > +         * created the mmu_notififer_mm is not freed until the mm is
> > +         * destroyed.  As above, users holding the mmap_sem or one of the
> > +         * mm_take_all_locks() do not need to use acquire semantics.
> >      */
> 
> Some spaces instead of tabs here.

Got it

> > +static int __mmu_interval_notifier_insert(
> > +   struct mmu_interval_notifier *mni, struct mm_struct *mm,
> > +   struct mmu_notifier_mm *mmn_mm, unsigned long start,
> > +   unsigned long length, const struct mmu_interval_notifier_ops *ops)
> 
> Odd indentation - we usuall do two tabs (my preference) or align after
> the opening brace.

This is one tab. I don't think one tab is odd, it seems pretty popular
even just in mm/.

But two tabs is considered usual? I didn't even know that.

> > + * This function must be paired with mmu_interval_notifier_insert(). It 
> > cannot be
> 
> line > 80 chars.

got it, was missed during the rename

> Otherwise this looks good and very similar to what I reviewed earlier:
> 
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>

Thanks a bunch, your comments have been very helpful on this series!

Jason

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.