From: Daniel Vetter <daniel@ffwll.ch> To: Jason Gunthorpe <jgg@ziepe.ca> Cc: LKML <linux-kernel@vger.kernel.org>, "Linux MM" <linux-mm@kvack.org>, "DRI Development" <dri-devel@lists.freedesktop.org>, "Intel Graphics Development" <intel-gfx@lists.freedesktop.org>, "Andrew Morton" <akpm@linux-foundation.org>, "Michal Hocko" <mhocko@suse.com>, "David Rientjes" <rientjes@google.com>, "Christian König" <christian.koenig@amd.com>, "Jérôme Glisse" <jglisse@redhat.com>, "Daniel Vetter" <daniel.vetter@intel.com> Subject: Re: [PATCH 4/4] mm, notifier: Catch sleeping/blocking for !blockable Date: Wed, 21 Aug 2019 11:34:06 +0200 [thread overview] Message-ID: <CAKMK7uGuH_Lvzf+M3Vast-RFS6Dr70F+Q4U_aSHuR1TpJg02SQ@mail.gmail.com> (raw) In-Reply-To: <20190820152712.GH29246@ziepe.ca> On Wed, Aug 21, 2019 at 9:33 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Tue, Aug 20, 2019 at 05:18:10PM +0200, Daniel Vetter wrote: > > > > diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c > > > > index 538d3bb87f9b..856636d06ee0 100644 > > > > +++ b/mm/mmu_notifier.c > > > > @@ -181,7 +181,13 @@ int __mmu_notifier_invalidate_range_start(struct mmu_notifier_range *range) > > > > id = srcu_read_lock(&srcu); > > > > hlist_for_each_entry_rcu(mn, &range->mm->mmu_notifier_mm->list, hlist) { > > > > if (mn->ops->invalidate_range_start) { > > > > - int _ret = mn->ops->invalidate_range_start(mn, range); > > > > + int _ret; > > > > + > > > > + if (!mmu_notifier_range_blockable(range)) > > > > + non_block_start(); > > > > + _ret = mn->ops->invalidate_range_start(mn, range); > > > > + if (!mmu_notifier_range_blockable(range)) > > > > + non_block_end(); > > > > > > If someone Acks all the sched changes then I can pick this for > > > hmm.git, but I still think the existing pre-emption debugging is fine > > > for this use case. > > > > Ok, I'll ping Peter Z. for an ack, iirc he was involved. > > > > > Also, same comment as for the lockdep map, this needs to apply to the > > > non-blocking range_end also. > > > > Hm, I thought the page table locks we're holding there already prevent any > > sleeping, so would be redundant? > > AFAIK no. All callers of invalidate_range_start/end pairs do so a few > lines apart and don't change their locking in between - thus since > start can block so can end. > > Would love to know if that is not true?? Yeah I reviewed them, I think I mixed up a discussion I had a while ago with Jerome. It's a bit tricky to follow in the code since in some places ->invalidate_range and ->invalidate_range_end seem to be called from the same place, in others not at all. > Similarly I've also been idly wondering if we should add a > 'might_sleep()' to invalidate_rangestart/end() to make this constraint > clear & tested to the mm side? Hm, sounds like a useful idea. Since in general you wont test with mmu notifiers, but they could happen, and then they will block for at least some mutex usually. I'll throw that as an idea on top for the next round. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Jason Gunthorpe <jgg@ziepe.ca> Cc: "Michal Hocko" <mhocko@suse.com>, "Intel Graphics Development" <intel-gfx@lists.freedesktop.org>, LKML <linux-kernel@vger.kernel.org>, "DRI Development" <dri-devel@lists.freedesktop.org>, "Linux MM" <linux-mm@kvack.org>, "Jérôme Glisse" <jglisse@redhat.com>, "David Rientjes" <rientjes@google.com>, "Daniel Vetter" <daniel.vetter@intel.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Christian König" <christian.koenig@amd.com> Subject: Re: [PATCH 4/4] mm, notifier: Catch sleeping/blocking for !blockable Date: Wed, 21 Aug 2019 11:34:06 +0200 [thread overview] Message-ID: <CAKMK7uGuH_Lvzf+M3Vast-RFS6Dr70F+Q4U_aSHuR1TpJg02SQ@mail.gmail.com> (raw) In-Reply-To: <20190820152712.GH29246@ziepe.ca> On Wed, Aug 21, 2019 at 9:33 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Tue, Aug 20, 2019 at 05:18:10PM +0200, Daniel Vetter wrote: > > > > diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c > > > > index 538d3bb87f9b..856636d06ee0 100644 > > > > +++ b/mm/mmu_notifier.c > > > > @@ -181,7 +181,13 @@ int __mmu_notifier_invalidate_range_start(struct mmu_notifier_range *range) > > > > id = srcu_read_lock(&srcu); > > > > hlist_for_each_entry_rcu(mn, &range->mm->mmu_notifier_mm->list, hlist) { > > > > if (mn->ops->invalidate_range_start) { > > > > - int _ret = mn->ops->invalidate_range_start(mn, range); > > > > + int _ret; > > > > + > > > > + if (!mmu_notifier_range_blockable(range)) > > > > + non_block_start(); > > > > + _ret = mn->ops->invalidate_range_start(mn, range); > > > > + if (!mmu_notifier_range_blockable(range)) > > > > + non_block_end(); > > > > > > If someone Acks all the sched changes then I can pick this for > > > hmm.git, but I still think the existing pre-emption debugging is fine > > > for this use case. > > > > Ok, I'll ping Peter Z. for an ack, iirc he was involved. > > > > > Also, same comment as for the lockdep map, this needs to apply to the > > > non-blocking range_end also. > > > > Hm, I thought the page table locks we're holding there already prevent any > > sleeping, so would be redundant? > > AFAIK no. All callers of invalidate_range_start/end pairs do so a few > lines apart and don't change their locking in between - thus since > start can block so can end. > > Would love to know if that is not true?? Yeah I reviewed them, I think I mixed up a discussion I had a while ago with Jerome. It's a bit tricky to follow in the code since in some places ->invalidate_range and ->invalidate_range_end seem to be called from the same place, in others not at all. > Similarly I've also been idly wondering if we should add a > 'might_sleep()' to invalidate_rangestart/end() to make this constraint > clear & tested to the mm side? Hm, sounds like a useful idea. Since in general you wont test with mmu notifiers, but they could happen, and then they will block for at least some mutex usually. I'll throw that as an idea on top for the next round. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-08-21 9:34 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-20 8:18 [PATCH 0/4] mmu notifier debug annotations/checks Daniel Vetter 2019-08-20 8:18 ` [PATCH 1/4] mm, notifier: Add a lockdep map for invalidate_range_start/end Daniel Vetter 2019-08-20 13:31 ` Jason Gunthorpe 2019-08-20 8:19 ` [PATCH 2/4] mm, notifier: Prime lockdep Daniel Vetter 2019-08-20 13:31 ` Jason Gunthorpe 2019-08-20 8:19 ` [PATCH 3/4] kernel.h: Add non_block_start/end() Daniel Vetter 2019-08-20 8:19 ` Daniel Vetter 2019-08-20 20:24 ` Daniel Vetter 2019-08-20 20:24 ` Daniel Vetter 2019-08-22 23:14 ` Andrew Morton 2019-08-22 23:14 ` Andrew Morton 2019-08-23 8:34 ` Daniel Vetter 2019-08-23 8:34 ` Daniel Vetter 2019-08-23 12:12 ` Jason Gunthorpe 2019-08-23 12:12 ` Jason Gunthorpe 2019-08-23 12:22 ` Peter Zijlstra 2019-08-23 12:22 ` Peter Zijlstra 2019-08-23 13:42 ` Daniel Vetter 2019-08-23 13:42 ` Daniel Vetter 2019-08-23 14:06 ` Peter Zijlstra 2019-08-23 14:06 ` Peter Zijlstra 2019-08-23 15:15 ` Daniel Vetter 2019-08-23 15:15 ` Daniel Vetter 2019-08-23 8:48 ` Peter Zijlstra 2019-08-23 8:48 ` Peter Zijlstra 2019-08-20 8:19 ` [PATCH 4/4] mm, notifier: Catch sleeping/blocking for !blockable Daniel Vetter 2019-08-20 13:34 ` Jason Gunthorpe 2019-08-20 15:18 ` Daniel Vetter 2019-08-20 15:27 ` Jason Gunthorpe 2019-08-21 9:34 ` Daniel Vetter [this message] 2019-08-21 9:34 ` Daniel Vetter 2019-08-21 15:41 ` Daniel Vetter 2019-08-21 15:41 ` Daniel Vetter 2019-08-21 16:16 ` Jason Gunthorpe 2019-08-22 8:42 ` Daniel Vetter 2019-08-22 8:42 ` Daniel Vetter 2019-08-22 14:24 ` Jason Gunthorpe 2019-08-22 14:27 ` Daniel Vetter 2019-08-22 14:27 ` Daniel Vetter 2019-08-20 11:15 ` ✗ Fi.CI.CHECKPATCH: warning for mmu notifier debug annotations/checks Patchwork 2019-08-20 12:33 ` ✓ Fi.CI.BAT: success " Patchwork 2019-08-20 18:14 ` ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAKMK7uGuH_Lvzf+M3Vast-RFS6Dr70F+Q4U_aSHuR1TpJg02SQ@mail.gmail.com \ --to=daniel@ffwll.ch \ --cc=akpm@linux-foundation.org \ --cc=christian.koenig@amd.com \ --cc=daniel.vetter@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=jgg@ziepe.ca \ --cc=jglisse@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@suse.com \ --cc=rientjes@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.