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 _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
