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

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

> +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@xxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel