On Wed, Aug 14, 2019 at 09:09:59PM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 14, 2019 at 10:20:26PM +0200, Daniel Vetter wrote:
> > This is a similar idea to the fs_reclaim fake lockdep lock. It's
> > fairly easy to provoke a specific notifier to be run on a specific
> > range: Just prep it, and then munmap() it.
> >
> > A bit harder, but still doable, is to provoke the mmu notifiers for
> > all the various callchains that might lead to them. But both at the
> > same time is really hard to reliable hit, especially when you want to
> > exercise paths like direct reclaim or compaction, where it's not
> > easy to control what exactly will be unmapped.
> >
> > By introducing a lockdep map to tie them all together we allow lockdep
> > to see a lot more dependencies, without having to actually hit them
> > in a single challchain while testing.
> >
> > Aside: Since I typed this to test i915 mmu notifiers I've only rolled
> > this out for the invaliate_range_start callback. If there's
> > interest, we should probably roll this out to all of them. But my
> > undestanding of core mm is seriously lacking, and I'm not clear on
> > whether we need a lockdep map for each callback, or whether some can
> > be shared.
>
> I was thinking about doing something like this..
>
> IMHO only range_end needs annotation, the other ops are either already
> non-sleeping or only used by KVM.
This isnt' about sleeping, this is about locking loops. And the biggest
risk for that is from driver code, and at least hmm_mirror only has the
driver code callback on invalidate_range_start. Once thing I discovered
using this (and it would be really hard to spot, it's deeply neested) is
that i915 userptr.
Even if i915 userptr would use hmm_mirror (to fix the issue you mention
below), if we then switch the annotation to invalidate_range_end nothing
interesting would ever come from this. Well, the only thing it'd catch is
issues in hmm_mirror, but I think core mm review will catch that before it
reaches us :-)
> BTW, I have found it strange that i915 only uses
> invalidate_range_start. Not really sure how it is able to do
> that. Would love to know the answer :)
I suspect it's broken :-/ Our userptr is ... not the best. Part of the
motivation here.
> > Reviewed-by: Jérôme Glisse <jglisse@xxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > include/linux/mmu_notifier.h | 6 ++++++
> > mm/mmu_notifier.c | 7 +++++++
> > 2 files changed, 13 insertions(+)
> >
> > diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
> > index b6c004bd9f6a..9dd38c32fc53 100644
> > +++ b/include/linux/mmu_notifier.h
> > @@ -42,6 +42,10 @@ enum mmu_notifier_event {
> >
> > #ifdef CONFIG_MMU_NOTIFIER
> >
> > +#ifdef CONFIG_LOCKDEP
> > +extern struct lockdep_map __mmu_notifier_invalidate_range_start_map;
> > +#endif
>
> I wonder what the trade off is having a global map vs a map in each
> mmu_notifier_mm ?
Less reports, specifically no reports involving multiple different mmu
notifiers to build the entire chain. But I'm assuming it's possible to
combine them in one mm (kvm+gpu+infiniband in one process sounds like
something someone could reasonably do), and it will help to make sure
everyone follows the same rules.
>
> > /*
> > * The mmu notifier_mm structure is allocated and installed in
> > * mm->mmu_notifier_mm inside the mm_take_all_locks() protected
> > @@ -310,10 +314,12 @@ static inline void mmu_notifier_change_pte(struct mm_struct *mm,
> > static inline void
> > mmu_notifier_invalidate_range_start(struct mmu_notifier_range *range)
> > {
> > + lock_map_acquire(&__mmu_notifier_invalidate_range_start_map);
> > if (mm_has_notifiers(range->mm)) {
> > range->flags |= MMU_NOTIFIER_RANGE_BLOCKABLE;
> > __mmu_notifier_invalidate_range_start(range);
> > }
> > + lock_map_release(&__mmu_notifier_invalidate_range_start_map);
> > }
>
> Also range_end should have this too - it has all the same
> constraints. I think it can share the map. So 'range_start_map' is
> probably not the right name.
>
> It may also make some sense to do a dummy acquire/release under the
> mm_take_all_locks() to forcibly increase map coverage and reduce the
> scenario complexity required to hit bugs.
>
> And if we do decide on the reclaim thing in my other email then the
> reclaim dependency can be reliably injected by doing:
>
> fs_reclaim_acquire();
> lock_map_acquire(&__mmu_notifier_invalidate_range_start_map);
> lock_map_release(&__mmu_notifier_invalidate_range_start_map);
> fs_reclaim_release();
>
> If I understand lockdep properly..
Ime fs_reclaim injects the mmu_notifier map here reliably as soon as
you've thrown out the first pagecache mmap on any process. That "make sure
we inject it quickly" is why the lockdep is _outside_ of the
mm_has_notifiers() check. So no further injection needed imo.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
