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

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

 




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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux