Nouveau - Nov 2019 - [PATCH v3 02/14] mm/mmu_notifier: add an interval tree notifier

> +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.
> +	/*
> +	 * 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 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.
	 */
> +	/*
> +	 * 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?
> +	 * 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.
> +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 function must be paired with mmu_interval_notifier_insert(). It
cannot be
line > 80 chars.

Otherwise this looks good and very similar to what I reviewed earlier:

Reviewed-by: Christoph Hellwig <hch at lst.de>

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 at lst.de>
Thanks a bunch, your comments have been very helpful on this series!

Jason

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".