Hi all (but I guess mostly Jason),
Finally gotten around to rebasing the previous version, fixing the rebase
fail in there, update the commit message a bit and give this a spin with
some tests. Nicely caught a lockdep splat that we're now discussing in
i915, and seems to not have misfired anywhere else (including a few oom).
Review, comments and everything very much appreciated. Plus I'd really
like to land this, there's more hmm_mirror users in-flight, and I've seen
some that get things wrong which this patchset should catch.
Thanks, Daniel
Daniel Vetter (5):
mm: Check if mmu notifier callbacks are allowed to fail
kernel.h: Add non_block_start/end()
mm, notifier: Catch sleeping/blocking for !blockable
mm, notifier: Add a lockdep map for invalidate_range_start
mm/hmm: WARN on illegal ->sync_cpu_device_pagetables errors
include/linux/kernel.h | 10 +++++++++-
include/linux/mmu_notifier.h | 6 ++++++
include/linux/sched.h | 4 ++++
kernel/sched/core.c | 19 ++++++++++++++-----
mm/hmm.c | 3 +++
mm/mmu_notifier.c | 17 ++++++++++++++++-
6 files changed, 52 insertions(+), 7 deletions(-)
--
2.22.0
Similar to the warning in the mmu notifer, warning if an hmm mirror
callback gets it's blocking vs. nonblocking handling wrong, or if it
fails with anything else than -EAGAIN.
Cc: Jason Gunthorpe <[email protected]>
Cc: Ralph Campbell <[email protected]>
Cc: John Hubbard <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Dan Carpenter <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Balbir Singh <[email protected]>
Cc: Ira Weiny <[email protected]>
Cc: Souptick Joarder <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: "Jérôme Glisse" <[email protected]>
Cc: [email protected]
Signed-off-by: Daniel Vetter <[email protected]>
---
mm/hmm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mm/hmm.c b/mm/hmm.c
index 16b6731a34db..52ac59384268 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -205,6 +205,9 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
ret = -EAGAIN;
break;
}
+ WARN(ret, "%pS callback failed with %d in %sblockable context\n",
+ mirror->ops->sync_cpu_device_pagetables, ret,
+ update.blockable ? "" : "non-");
}
up_read(&hmm->mirrors_sem);
--
2.22.0
On Wed, Aug 14, 2019 at 10:20:27PM +0200, Daniel Vetter wrote:
> Similar to the warning in the mmu notifer, warning if an hmm mirror
> callback gets it's blocking vs. nonblocking handling wrong, or if it
> fails with anything else than -EAGAIN.
>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Ralph Campbell <[email protected]>
> Cc: John Hubbard <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Dan Carpenter <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Balbir Singh <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Cc: Souptick Joarder <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: "Jérôme Glisse" <[email protected]>
> Cc: [email protected]
> Signed-off-by: Daniel Vetter <[email protected]>
> mm/hmm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 16b6731a34db..52ac59384268 100644
> +++ b/mm/hmm.c
> @@ -205,6 +205,9 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> ret = -EAGAIN;
> break;
> }
> + WARN(ret, "%pS callback failed with %d in %sblockable context\n",
> + mirror->ops->sync_cpu_device_pagetables, ret,
> + update.blockable ? "" : "non-");
> }
> up_read(&hmm->mirrors_sem);
Didn't I beat you to this?
list_for_each_entry(mirror, &hmm->mirrors, list) {
int rc;
rc = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
if (rc) {
if (WARN_ON(update.blockable || rc != -EAGAIN))
continue;
ret = -EAGAIN;
break;
}
}
Thanks,
Jason
On Wed, Aug 14, 2019 at 09:11:37PM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 14, 2019 at 10:20:27PM +0200, Daniel Vetter wrote:
> > Similar to the warning in the mmu notifer, warning if an hmm mirror
> > callback gets it's blocking vs. nonblocking handling wrong, or if it
> > fails with anything else than -EAGAIN.
> >
> > Cc: Jason Gunthorpe <[email protected]>
> > Cc: Ralph Campbell <[email protected]>
> > Cc: John Hubbard <[email protected]>
> > Cc: Dan Williams <[email protected]>
> > Cc: Dan Carpenter <[email protected]>
> > Cc: Matthew Wilcox <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Balbir Singh <[email protected]>
> > Cc: Ira Weiny <[email protected]>
> > Cc: Souptick Joarder <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: "J?r?me Glisse" <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Daniel Vetter <[email protected]>
> > mm/hmm.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index 16b6731a34db..52ac59384268 100644
> > +++ b/mm/hmm.c
> > @@ -205,6 +205,9 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> > ret = -EAGAIN;
> > break;
> > }
> > + WARN(ret, "%pS callback failed with %d in %sblockable context\n",
> > + mirror->ops->sync_cpu_device_pagetables, ret,
> > + update.blockable ? "" : "non-");
> > }
> > up_read(&hmm->mirrors_sem);
>
> Didn't I beat you to this?
Very much possible, I think I didn't rebase this to linux-next before
resending ... have an
Reviewed-by: Daniel Vetter <[email protected]>
in case you need.
Cheers, Daniel
>
> list_for_each_entry(mirror, &hmm->mirrors, list) {
> int rc;
>
> rc = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
> if (rc) {
> if (WARN_ON(update.blockable || rc != -EAGAIN))
> continue;
> ret = -EAGAIN;
> break;
> }
> }
>
> Thanks,
> Jason
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch