| Message ID | 20190520213945.17046-4-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] mm: Check if mmu notifier callbacks are allowed to fail | expand |
On Mon, May 20, 2019 at 11:39:45PM +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 need to read more on lockdep but it is legal to have mmu notifier invalidation within each other. For instance when you munmap you might split a huge pmd and it will trigger a second invalidate range while the munmap one is not done yet. Would that trigger the lockdep here ? Worst case i can think of is 2 invalidate_range_start chain one after the other. I don't think you can triggers a 3 levels nesting but maybe. Cheers, Jérôme
On Tue, May 21, 2019 at 5:41 PM Jerome Glisse <jglisse@redhat.com> wrote: > > On Mon, May 20, 2019 at 11:39:45PM +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 need to read more on lockdep but it is legal to have mmu notifier > invalidation within each other. For instance when you munmap you > might split a huge pmd and it will trigger a second invalidate range > while the munmap one is not done yet. Would that trigger the lockdep > here ? Depends how it's nesting. I'm wrapping the annotation only just around the individual mmu notifier callback, so if the nesting is just - munmap starts - invalidate_range_start #1 - we noticed that there's a huge pmd we need to split - invalidate_range_start #2 - invalidate_reange_end #2 - invalidate_range_end #1 - munmap is done But if otoh it's ok to trigger the 2nd invalidate range from within an mmu_notifier->invalidate_range_start callback, then lockdep will be pissed about that. > Worst case i can think of is 2 invalidate_range_start chain one after > the other. I don't think you can triggers a 3 levels nesting but maybe. Lockdep has special nesting annotations. I think it'd be more an issue of getting those funneled through the entire call chain, assuming we really need that. -Daniel
On Tue, May 21, 2019 at 06:00:36PM +0200, Daniel Vetter wrote: > On Tue, May 21, 2019 at 5:41 PM Jerome Glisse <jglisse@redhat.com> wrote: > > > > On Mon, May 20, 2019 at 11:39:45PM +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 need to read more on lockdep but it is legal to have mmu notifier > > invalidation within each other. For instance when you munmap you > > might split a huge pmd and it will trigger a second invalidate range > > while the munmap one is not done yet. Would that trigger the lockdep > > here ? > > Depends how it's nesting. I'm wrapping the annotation only just around > the individual mmu notifier callback, so if the nesting is just > - munmap starts > - invalidate_range_start #1 > - we noticed that there's a huge pmd we need to split > - invalidate_range_start #2 > - invalidate_reange_end #2 > - invalidate_range_end #1 > - munmap is done Yeah this is how it looks. All the callback from range_start #1 would happens before range_start #2 happens so we should be fine. > > But if otoh it's ok to trigger the 2nd invalidate range from within an > mmu_notifier->invalidate_range_start callback, then lockdep will be > pissed about that. No that would be illegal for a callback to do that. There is no existing callback that would do that at least AFAIK. So we can just say that it is illegal. I would not see the point. > > > Worst case i can think of is 2 invalidate_range_start chain one after > > the other. I don't think you can triggers a 3 levels nesting but maybe. > > Lockdep has special nesting annotations. I think it'd be more an issue > of getting those funneled through the entire call chain, assuming we > really need that. I think we are fine. So this patch looks good. Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h index b6c004bd9f6a..9dd38c32fc53 100644 --- a/include/linux/mmu_notifier.h +++ 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 + /* * 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); } static inline int diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c index a09e737711d5..33bdaddfb9b1 100644 --- a/mm/mmu_notifier.c +++ b/mm/mmu_notifier.c @@ -23,6 +23,13 @@ /* global SRCU for all MMs */ DEFINE_STATIC_SRCU(srcu); +#ifdef CONFIG_LOCKDEP +struct lockdep_map __mmu_notifier_invalidate_range_start_map = { + .name = "mmu_notifier_invalidate_range_start" +}; +EXPORT_SYMBOL_GPL(__mmu_notifier_invalidate_range_start_map); +#endif + /* * This function allows mmu_notifier::release callback to delay a call to * a function that will free appropriate resources. The function must be
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. v2: Use lock_map_acquire/release() like fs_reclaim, to avoid confusion with this being a real mutex (Chris Wilson). v3: Rebase on top of Glisse's arg rework. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: David Rientjes <rientjes@google.com> Cc: "Jérôme Glisse" <jglisse@redhat.com> Cc: Michal Hocko <mhocko@suse.com> Cc: "Christian König" <christian.koenig@amd.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Mike Rapoport <rppt@linux.vnet.ibm.com> Cc: linux-mm@kvack.org Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- include/linux/mmu_notifier.h | 6 ++++++ mm/mmu_notifier.c | 7 +++++++ 2 files changed, 13 insertions(+)