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@lst.de

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

Jason