[1/3] mm: Check if mmu notifier callbacks are allowed to fail
diff mbox series

Message ID 20181122165106.18238-2-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • RFC: mmu notifier debug checks
Related show

Commit Message

Daniel Vetter Nov. 22, 2018, 4:51 p.m. UTC
Just a bit of paranoia, since if we start pushing this deep into
callchains it's hard to spot all places where an mmu notifier
implementation might fail when it's not allowed to.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: linux-mm@kvack.org
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 mm/mmu_notifier.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Chris Wilson Nov. 22, 2018, 4:53 p.m. UTC | #1
Quoting Daniel Vetter (2018-11-22 16:51:04)
> Just a bit of paranoia, since if we start pushing this deep into
> callchains it's hard to spot all places where an mmu notifier
> implementation might fail when it's not allowed to.

Most callers could handle the failure correctly. It looks like the
failure was not propagated for convenience.
-Chris
Christian König Nov. 22, 2018, 6:50 p.m. UTC | #2
Am 22.11.18 um 17:51 schrieb Daniel Vetter:
> Just a bit of paranoia, since if we start pushing this deep into
> callchains it's hard to spot all places where an mmu notifier
> implementation might fail when it's not allowed to.
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: linux-mm@kvack.org
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Christian König <christian.koenig@amd.com>

> ---
>   mm/mmu_notifier.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> index 5119ff846769..59e102589a25 100644
> --- a/mm/mmu_notifier.c
> +++ b/mm/mmu_notifier.c
> @@ -190,6 +190,8 @@ int __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
>   				pr_info("%pS callback failed with %d in %sblockable context.\n",
>   						mn->ops->invalidate_range_start, _ret,
>   						!blockable ? "non-" : "");
> +				WARN(blockable,"%pS callback failure not allowed\n",
> +				     mn->ops->invalidate_range_start);
>   				ret = _ret;
>   			}
>   		}
Daniel Vetter Nov. 23, 2018, 8:49 a.m. UTC | #3
On Thu, Nov 22, 2018 at 04:53:34PM +0000, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-11-22 16:51:04)
> > Just a bit of paranoia, since if we start pushing this deep into
> > callchains it's hard to spot all places where an mmu notifier
> > implementation might fail when it's not allowed to.
> 
> Most callers could handle the failure correctly. It looks like the
> failure was not propagated for convenience.

I have no idea whether the mm is semantically ok if pte shootdown doesn't
work for all sorts of strange reasons. From the commit that introduced the
error code it souded like this was very much only ok in the limited case
of an already killed process, in the oom killer path, where it's really
only about trying to free any kind of memory. And where the process is
gone already, so semantics of what exactly happens don't matter that much
anymore.

And even if a lot more paths could support some kind of error recovery
(they'd need to restart stuff, at least for your i915 patch to work I
think), as long as we have paths where that's not allowed I think it's
good to catch any bugs where a nonzero errno is errornously returned.
-Daniel
Michal Hocko Nov. 23, 2018, 11:14 a.m. UTC | #4
On Fri 23-11-18 09:49:34, Daniel Vetter wrote:
> On Thu, Nov 22, 2018 at 04:53:34PM +0000, Chris Wilson wrote:
> > Quoting Daniel Vetter (2018-11-22 16:51:04)
> > > Just a bit of paranoia, since if we start pushing this deep into
> > > callchains it's hard to spot all places where an mmu notifier
> > > implementation might fail when it's not allowed to.
> > 
> > Most callers could handle the failure correctly. It looks like the
> > failure was not propagated for convenience.
> 
> I have no idea whether the mm is semantically ok if pte shootdown doesn't
> work for all sorts of strange reasons. From the commit that introduced the
> error code it souded like this was very much only ok in the limited case
> of an already killed process, in the oom killer path, where it's really
> only about trying to free any kind of memory. And where the process is
> gone already, so semantics of what exactly happens don't matter that much
> anymore.

Yes this was indeed the case. There is still the exit path which would
do the rest of the work so we are not leaving anything behind.
Michal Hocko Nov. 23, 2018, 11:15 a.m. UTC | #5
On Thu 22-11-18 17:51:04, Daniel Vetter wrote:
> Just a bit of paranoia, since if we start pushing this deep into
> callchains it's hard to spot all places where an mmu notifier
> implementation might fail when it's not allowed to.

What does WARN give you more than the existing pr_info? Is really
backtrace that interesting?

> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: linux-mm@kvack.org
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  mm/mmu_notifier.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> index 5119ff846769..59e102589a25 100644
> --- a/mm/mmu_notifier.c
> +++ b/mm/mmu_notifier.c
> @@ -190,6 +190,8 @@ int __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
>  				pr_info("%pS callback failed with %d in %sblockable context.\n",
>  						mn->ops->invalidate_range_start, _ret,
>  						!blockable ? "non-" : "");
> +				WARN(blockable,"%pS callback failure not allowed\n",
> +				     mn->ops->invalidate_range_start);
>  				ret = _ret;
>  			}
>  		}
> -- 
> 2.19.1
>
Daniel Vetter Nov. 23, 2018, 12:30 p.m. UTC | #6
On Fri, Nov 23, 2018 at 12:15:57PM +0100, Michal Hocko wrote:
> On Thu 22-11-18 17:51:04, Daniel Vetter wrote:
> > Just a bit of paranoia, since if we start pushing this deep into
> > callchains it's hard to spot all places where an mmu notifier
> > implementation might fail when it's not allowed to.
> 
> What does WARN give you more than the existing pr_info? Is really
> backtrace that interesting?

Automated tools have to ignore everything at info level (there's too much
of that). I guess I could do something like

if (blockable)
	pr_warn(...)
else
	pr_info(...)

WARN() is simply my goto tool for getting something at warning level
dumped into dmesg. But I think the pr_warn with the callback function
should be enough indeed.

If you wonder where all the info level stuff happens that we have to
ignore: suspend/resume is a primary culprit (fairly important for
gfx/desktops), but there's a bunch of other places. Even if we ignore
everything at info and below we still need filters because some drivers
are a bit too trigger-happy (i915 definitely included I guess, so everyone
contributes to this problem).

Cheers, Daniel

> 
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: "Jérôme Glisse" <jglisse@redhat.com>
> > Cc: linux-mm@kvack.org
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  mm/mmu_notifier.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> > index 5119ff846769..59e102589a25 100644
> > --- a/mm/mmu_notifier.c
> > +++ b/mm/mmu_notifier.c
> > @@ -190,6 +190,8 @@ int __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
> >  				pr_info("%pS callback failed with %d in %sblockable context.\n",
> >  						mn->ops->invalidate_range_start, _ret,
> >  						!blockable ? "non-" : "");
> > +				WARN(blockable,"%pS callback failure not allowed\n",
> > +				     mn->ops->invalidate_range_start);
> >  				ret = _ret;
> >  			}
> >  		}
> > -- 
> > 2.19.1
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs
Michal Hocko Nov. 23, 2018, 12:43 p.m. UTC | #7
On Fri 23-11-18 13:30:57, Daniel Vetter wrote:
> On Fri, Nov 23, 2018 at 12:15:57PM +0100, Michal Hocko wrote:
> > On Thu 22-11-18 17:51:04, Daniel Vetter wrote:
> > > Just a bit of paranoia, since if we start pushing this deep into
> > > callchains it's hard to spot all places where an mmu notifier
> > > implementation might fail when it's not allowed to.
> > 
> > What does WARN give you more than the existing pr_info? Is really
> > backtrace that interesting?
> 
> Automated tools have to ignore everything at info level (there's too much
> of that). I guess I could do something like
> 
> if (blockable)
> 	pr_warn(...)
> else
> 	pr_info(...)
> 
> WARN() is simply my goto tool for getting something at warning level
> dumped into dmesg. But I think the pr_warn with the callback function
> should be enough indeed.

I wouldn't mind s@pr_info@pr_warn@
 
> If you wonder where all the info level stuff happens that we have to
> ignore: suspend/resume is a primary culprit (fairly important for
> gfx/desktops), but there's a bunch of other places. Even if we ignore
> everything at info and below we still need filters because some drivers
> are a bit too trigger-happy (i915 definitely included I guess, so everyone
> contributes to this problem).

Thanks for the clarification.
Daniel Vetter Nov. 23, 2018, 1:15 p.m. UTC | #8
On Fri, Nov 23, 2018 at 1:43 PM Michal Hocko <mhocko@kernel.org> wrote:
> On Fri 23-11-18 13:30:57, Daniel Vetter wrote:
> > On Fri, Nov 23, 2018 at 12:15:57PM +0100, Michal Hocko wrote:
> > > On Thu 22-11-18 17:51:04, Daniel Vetter wrote:
> > > > Just a bit of paranoia, since if we start pushing this deep into
> > > > callchains it's hard to spot all places where an mmu notifier
> > > > implementation might fail when it's not allowed to.
> > >
> > > What does WARN give you more than the existing pr_info? Is really
> > > backtrace that interesting?
> >
> > Automated tools have to ignore everything at info level (there's too much
> > of that). I guess I could do something like
> >
> > if (blockable)
> >       pr_warn(...)
> > else
> >       pr_info(...)
> >
> > WARN() is simply my goto tool for getting something at warning level
> > dumped into dmesg. But I think the pr_warn with the callback function
> > should be enough indeed.
>
> I wouldn't mind s@pr_info@pr_warn@

Well that's too much, because then it would misfire in the oom
testcase, where failing is ok (desireble even, we want to avoid
blocking after all). So needs to be  a switch (or else we need to
filter it in results, and that's a bit a maintenance headache from a
CI pov).
-Danile

> > If you wonder where all the info level stuff happens that we have to
> > ignore: suspend/resume is a primary culprit (fairly important for
> > gfx/desktops), but there's a bunch of other places. Even if we ignore
> > everything at info and below we still need filters because some drivers
> > are a bit too trigger-happy (i915 definitely included I guess, so everyone
> > contributes to this problem).
>
> Thanks for the clarification.
> --
> Michal Hocko
> SUSE Labs
Michal Hocko Nov. 23, 2018, 1:30 p.m. UTC | #9
On Fri 23-11-18 14:15:11, Daniel Vetter wrote:
> On Fri, Nov 23, 2018 at 1:43 PM Michal Hocko <mhocko@kernel.org> wrote:
> > On Fri 23-11-18 13:30:57, Daniel Vetter wrote:
> > > On Fri, Nov 23, 2018 at 12:15:57PM +0100, Michal Hocko wrote:
> > > > On Thu 22-11-18 17:51:04, Daniel Vetter wrote:
> > > > > Just a bit of paranoia, since if we start pushing this deep into
> > > > > callchains it's hard to spot all places where an mmu notifier
> > > > > implementation might fail when it's not allowed to.
> > > >
> > > > What does WARN give you more than the existing pr_info? Is really
> > > > backtrace that interesting?
> > >
> > > Automated tools have to ignore everything at info level (there's too much
> > > of that). I guess I could do something like
> > >
> > > if (blockable)
> > >       pr_warn(...)
> > > else
> > >       pr_info(...)
> > >
> > > WARN() is simply my goto tool for getting something at warning level
> > > dumped into dmesg. But I think the pr_warn with the callback function
> > > should be enough indeed.
> >
> > I wouldn't mind s@pr_info@pr_warn@
> 
> Well that's too much, because then it would misfire in the oom
> testcase, where failing is ok (desireble even, we want to avoid
> blocking after all). So needs to be  a switch (or else we need to
> filter it in results, and that's a bit a maintenance headache from a
> CI pov).

I thought the failure should be rare enough that warning about them can
be actually useful. E.g. in the oom case we can live with the failure
because we want to release _some_ memory but know about a callback that
prevents us to go the full way might be interesting.

But I do not really feel strongly about this. I find WARN a bit abuse
because the trace is unlikely going to help us much. If you want to make
a verbosity depending on the blockable context then I will surely not
stand in the way.

Patch
diff mbox series

diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
index 5119ff846769..59e102589a25 100644
--- a/mm/mmu_notifier.c
+++ b/mm/mmu_notifier.c
@@ -190,6 +190,8 @@  int __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
 				pr_info("%pS callback failed with %d in %sblockable context.\n",
 						mn->ops->invalidate_range_start, _ret,
 						!blockable ? "non-" : "");
+				WARN(blockable,"%pS callback failure not allowed\n",
+				     mn->ops->invalidate_range_start);
 				ret = _ret;
 			}
 		}