[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 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". _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |