Messages in this thread | | | Date | Thu, 25 Dec 2014 10:29:44 +0200 | From | Haggai Eran <> | Subject | Re: [PATCH 2/7] mmu_notifier: keep track of active invalidation ranges v2 |
| |
On 22/12/2014 18:48, j.glisse@gmail.com wrote: > static inline void mmu_notifier_invalidate_range_start(struct mm_struct *mm, > - unsigned long start, > - unsigned long end, > - enum mmu_event event) > + struct mmu_notifier_range *range) > { > + /* > + * Initialize list no matter what in case a mmu_notifier register after > + * a range_start but before matching range_end. > + */ > + INIT_LIST_HEAD(&range->list);
I don't see how can an mmu_notifier register after a range_start but before a matching range_end. The mmu_notifier registration locks all mm locks, and that should prevent any invalidation from running, right?
> if (mm_has_notifiers(mm)) > - __mmu_notifier_invalidate_range_start(mm, start, end, event); > + __mmu_notifier_invalidate_range_start(mm, range); > }
...
> void __mmu_notifier_invalidate_range_start(struct mm_struct *mm, > - unsigned long start, > - unsigned long end, > - enum mmu_event event) > + struct mmu_notifier_range *range) > > { > struct mmu_notifier *mn; > @@ -185,21 +183,36 @@ void __mmu_notifier_invalidate_range_start(struct mm_struct *mm, > id = srcu_read_lock(&srcu); > hlist_for_each_entry_rcu(mn, &mm->mmu_notifier_mm->list, hlist) { > if (mn->ops->invalidate_range_start) > - mn->ops->invalidate_range_start(mn, mm, start, > - end, event); > + mn->ops->invalidate_range_start(mn, mm, range); > } > srcu_read_unlock(&srcu, id); > + > + /* > + * This must happen after the callback so that subsystem can block on > + * new invalidation range to synchronize itself. > + */ > + spin_lock(&mm->mmu_notifier_mm->lock); > + list_add_tail(&range->list, &mm->mmu_notifier_mm->ranges); > + mm->mmu_notifier_mm->nranges++; > + spin_unlock(&mm->mmu_notifier_mm->lock); > } > EXPORT_SYMBOL_GPL(__mmu_notifier_invalidate_range_start);
Don't you have a race here because you add the range struct after the callback?
------------------------------------------------------------------------- Thread A | Thread B ------------------------------------------------------------------------- call mmu notifier callback | clear SPTE | | device page fault | mmu_notifier_range_is_valid returns true | install new SPTE add event struct to list | mm clears/modifies the PTE | -------------------------------------------------------------------------
So we are left with different entries in the host page table and the secondary page table.
I would think you'd want the event struct to be added to the list before the callback is run.
Best regards, Haggai
| |