Re: [Intel-gfx] [PATCH 2/3] mm, notifier: Catch sleeping/blocking for !blockable
On 23/11/2018 13:12, Daniel Vetter wrote: On Fri, Nov 23, 2018 at 1:46 PM Michal Hocko wrote: On Fri 23-11-18 13:38:38, Daniel Vetter wrote: On Fri, Nov 23, 2018 at 12:12:37PM +0100, Michal Hocko wrote: On Thu 22-11-18 17:51:05, Daniel Vetter wrote: We need to make sure implementations don't cheat and don't have a possible schedule/blocking point deeply burried where review can't catch it. I'm not sure whether this is the best way to make sure all the might_sleep() callsites trigger, and it's a bit ugly in the code flow. But it gets the job done. Yeah, it is quite ugly. Especially because it makes DEBUG config bahavior much different. So is this really worth it? Has this already discovered any existing bug? Given that we need an oom trigger to hit this we're not hitting this in CI (oom is just way to unpredictable to even try). I'd kinda like to also add some debug interface so I can provoke an oom kill of a specially prepared process, to make sure we can reliably exercise this path without killing the kernel accidentally. We do similar tricks for our shrinker already. Create a task with oom_score_adj = 1000 and trigger the oom killer via sysrq and you should get a predictable oom invocation and execution. Ah right. We kinda do that already in an attempt to get the tests killed without the runner, for accidental oom. Just didn't think about this in the context of intentionally firing the oom. I'll try whether I can bake up some new subtest in our userptr/mmu-notifier testcases. Very handy trick - I think I will think of applying it in the shrinker area as well. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] mm, notifier: Catch sleeping/blocking for !blockable
On Fri, Nov 23, 2018 at 1:46 PM Michal Hocko wrote:
>
> On Fri 23-11-18 13:38:38, Daniel Vetter wrote:
> > On Fri, Nov 23, 2018 at 12:12:37PM +0100, Michal Hocko wrote:
> > > On Thu 22-11-18 17:51:05, Daniel Vetter wrote:
> > > > We need to make sure implementations don't cheat and don't have a
> > > > possible schedule/blocking point deeply burried where review can't
> > > > catch it.
> > > >
> > > > I'm not sure whether this is the best way to make sure all the
> > > > might_sleep() callsites trigger, and it's a bit ugly in the code flow.
> > > > But it gets the job done.
> > >
> > > Yeah, it is quite ugly. Especially because it makes DEBUG config
> > > bahavior much different. So is this really worth it? Has this already
> > > discovered any existing bug?
> >
> > Given that we need an oom trigger to hit this we're not hitting this in CI
> > (oom is just way to unpredictable to even try). I'd kinda like to also add
> > some debug interface so I can provoke an oom kill of a specially prepared
> > process, to make sure we can reliably exercise this path without killing
> > the kernel accidentally. We do similar tricks for our shrinker already.
>
> Create a task with oom_score_adj = 1000 and trigger the oom killer via
> sysrq and you should get a predictable oom invocation and execution.
Ah right. We kinda do that already in an attempt to get the tests
killed without the runner, for accidental oom. Just didn't think about
this in the context of intentionally firing the oom. I'll try whether
I can bake up some new subtest in our userptr/mmu-notifier testcases.
> [...]
> > Wrt the behavior difference: I guess we could put another counter into the
> > task struct, and change might_sleep() to check it. All under
> > CONFIG_DEBUG_ATOMIC_SLEEP only ofc. That would avoid the preempt-disable
> > sideeffect. My worry with that is that people will spot it, and abuse it
> > in creative ways that do affect semantics. See horrors like
> > drm_can_sleep() (and I'm sure gfx folks are not the only ones who
> > seriously lacked taste here).
> >
> > Up to the experts really how to best paint this shed I think.
>
> Actually I like a way to say non_block_{begin,end} and might_sleep
> firing inside that context.
Ok, I'll respin with these (introduced in a separate patch).
-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
Re: [Intel-gfx] [PATCH 2/3] mm, notifier: Catch sleeping/blocking for !blockable
On Fri 23-11-18 13:38:38, Daniel Vetter wrote:
> On Fri, Nov 23, 2018 at 12:12:37PM +0100, Michal Hocko wrote:
> > On Thu 22-11-18 17:51:05, Daniel Vetter wrote:
> > > We need to make sure implementations don't cheat and don't have a
> > > possible schedule/blocking point deeply burried where review can't
> > > catch it.
> > >
> > > I'm not sure whether this is the best way to make sure all the
> > > might_sleep() callsites trigger, and it's a bit ugly in the code flow.
> > > But it gets the job done.
> >
> > Yeah, it is quite ugly. Especially because it makes DEBUG config
> > bahavior much different. So is this really worth it? Has this already
> > discovered any existing bug?
>
> Given that we need an oom trigger to hit this we're not hitting this in CI
> (oom is just way to unpredictable to even try). I'd kinda like to also add
> some debug interface so I can provoke an oom kill of a specially prepared
> process, to make sure we can reliably exercise this path without killing
> the kernel accidentally. We do similar tricks for our shrinker already.
Create a task with oom_score_adj = 1000 and trigger the oom killer via
sysrq and you should get a predictable oom invocation and execution.
[...]
> Wrt the behavior difference: I guess we could put another counter into the
> task struct, and change might_sleep() to check it. All under
> CONFIG_DEBUG_ATOMIC_SLEEP only ofc. That would avoid the preempt-disable
> sideeffect. My worry with that is that people will spot it, and abuse it
> in creative ways that do affect semantics. See horrors like
> drm_can_sleep() (and I'm sure gfx folks are not the only ones who
> seriously lacked taste here).
>
> Up to the experts really how to best paint this shed I think.
Actually I like a way to say non_block_{begin,end} and might_sleep
firing inside that context.
--
Michal Hocko
SUSE Labs
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] mm, notifier: Catch sleeping/blocking for !blockable
On Fri, Nov 23, 2018 at 12:12:37PM +0100, Michal Hocko wrote:
> On Thu 22-11-18 17:51:05, Daniel Vetter wrote:
> > We need to make sure implementations don't cheat and don't have a
> > possible schedule/blocking point deeply burried where review can't
> > catch it.
> >
> > I'm not sure whether this is the best way to make sure all the
> > might_sleep() callsites trigger, and it's a bit ugly in the code flow.
> > But it gets the job done.
>
> Yeah, it is quite ugly. Especially because it makes DEBUG config
> bahavior much different. So is this really worth it? Has this already
> discovered any existing bug?
Given that we need an oom trigger to hit this we're not hitting this in CI
(oom is just way to unpredictable to even try). I'd kinda like to also add
some debug interface so I can provoke an oom kill of a specially prepared
process, to make sure we can reliably exercise this path without killing
the kernel accidentally. We do similar tricks for our shrinker already.
There's been patches floating with this kind of bug I think, and the call
chains we're dealing with a fairly deep. I don't trust review to reliably
catch this kind of fail, that's why I'm looking into tools to better
validat this stuff to augment review.
And yes it's ugly :-/
Wrt the behavior difference: I guess we could put another counter into the
task struct, and change might_sleep() to check it. All under
CONFIG_DEBUG_ATOMIC_SLEEP only ofc. That would avoid the preempt-disable
sideeffect. My worry with that is that people will spot it, and abuse it
in creative ways that do affect semantics. See horrors like
drm_can_sleep() (and I'm sure gfx folks are not the only ones who
seriously lacked taste here).
Up to the experts really how to best paint this shed I think.
Thanks, Daniel
>
> > Cc: Andrew Morton
> > Cc: Michal Hocko
> > Cc: David Rientjes
> > Cc: "Christian König"
> > Cc: Daniel Vetter
> > Cc: "Jérôme Glisse"
> > Cc: linux...@kvack.org
> > Signed-off-by: Daniel Vetter
> > ---
> > mm/mmu_notifier.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> > index 59e102589a25..4d282cfb296e 100644
> > --- a/mm/mmu_notifier.c
> > +++ b/mm/mmu_notifier.c
> > @@ -185,7 +185,13 @@ int __mmu_notifier_invalidate_range_start(struct
> > mm_struct *mm,
> > id = srcu_read_lock();
> > hlist_for_each_entry_rcu(mn, >mmu_notifier_mm->list, hlist) {
> > if (mn->ops->invalidate_range_start) {
> > - int _ret = mn->ops->invalidate_range_start(mn, mm,
> > start, end, blockable);
> > + int _ret;
> > +
> > + if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
> > + preempt_disable();
> > + _ret = mn->ops->invalidate_range_start(mn, mm, start,
> > end, blockable);
> > + if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
> > + preempt_enable();
> > if (_ret) {
> > pr_info("%pS callback failed with %d in
> > %sblockable context.\n",
> >
> > mn->ops->invalidate_range_start, _ret,
> > --
> > 2.19.1
> >
>
> --
> Michal Hocko
> SUSE Labs
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] mm, notifier: Catch sleeping/blocking for !blockable
On Thu 22-11-18 17:51:05, Daniel Vetter wrote:
> We need to make sure implementations don't cheat and don't have a
> possible schedule/blocking point deeply burried where review can't
> catch it.
>
> I'm not sure whether this is the best way to make sure all the
> might_sleep() callsites trigger, and it's a bit ugly in the code flow.
> But it gets the job done.
Yeah, it is quite ugly. Especially because it makes DEBUG config
bahavior much different. So is this really worth it? Has this already
discovered any existing bug?
> Cc: Andrew Morton
> Cc: Michal Hocko
> Cc: David Rientjes
> Cc: "Christian König"
> Cc: Daniel Vetter
> Cc: "Jérôme Glisse"
> Cc: linux...@kvack.org
> Signed-off-by: Daniel Vetter
> ---
> mm/mmu_notifier.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> index 59e102589a25..4d282cfb296e 100644
> --- a/mm/mmu_notifier.c
> +++ b/mm/mmu_notifier.c
> @@ -185,7 +185,13 @@ int __mmu_notifier_invalidate_range_start(struct
> mm_struct *mm,
> id = srcu_read_lock();
> hlist_for_each_entry_rcu(mn, >mmu_notifier_mm->list, hlist) {
> if (mn->ops->invalidate_range_start) {
> - int _ret = mn->ops->invalidate_range_start(mn, mm,
> start, end, blockable);
> + int _ret;
> +
> + if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
> + preempt_disable();
> + _ret = mn->ops->invalidate_range_start(mn, mm, start,
> end, blockable);
> + if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
> + preempt_enable();
> if (_ret) {
> pr_info("%pS callback failed with %d in
> %sblockable context.\n",
>
> mn->ops->invalidate_range_start, _ret,
> --
> 2.19.1
>
--
Michal Hocko
SUSE Labs
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] mm, notifier: Catch sleeping/blocking for !blockable
Am 23.11.18 um 09:46 schrieb Daniel Vetter:
On Thu, Nov 22, 2018 at 06:55:17PM +, Koenig, Christian wrote:
Am 22.11.18 um 17:51 schrieb Daniel Vetter:
We need to make sure implementations don't cheat and don't have a
possible schedule/blocking point deeply burried where review can't
catch it.
I'm not sure whether this is the best way to make sure all the
might_sleep() callsites trigger, and it's a bit ugly in the code flow.
But it gets the job done.
Cc: Andrew Morton
Cc: Michal Hocko
Cc: David Rientjes
Cc: "Christian König"
Cc: Daniel Vetter
Cc: "Jérôme Glisse"
Cc: linux...@kvack.org
Signed-off-by: Daniel Vetter
---
mm/mmu_notifier.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
index 59e102589a25..4d282cfb296e 100644
--- a/mm/mmu_notifier.c
+++ b/mm/mmu_notifier.c
@@ -185,7 +185,13 @@ int __mmu_notifier_invalidate_range_start(struct mm_struct
*mm,
id = srcu_read_lock();
hlist_for_each_entry_rcu(mn, >mmu_notifier_mm->list, hlist) {
if (mn->ops->invalidate_range_start) {
- int _ret = mn->ops->invalidate_range_start(mn, mm,
start, end, blockable);
+ int _ret;
+
+ if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
+ preempt_disable();
+ _ret = mn->ops->invalidate_range_start(mn, mm, start,
end, blockable);
+ if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
+ preempt_enable();
Just for the sake of better documenting this how about adding this to
include/linux/kernel.h right next to might_sleep():
#define disallow_sleeping_if(cond) for((cond) ? preempt_disable() :
(void)0; (cond); preempt_disable())
(Just from the back of my head, might contain peanuts and/or hints of
errors).
I think these magic for blocks aren't used in the kernel. goto breaks
them, and we use goto a lot.
Yeah, good argument.
I think a disallow/allow_sleep() pair with
the conditional preept_disable/enable() calls would be nice though. I can
do that if the overall idea sticks.
Sounds like a good idea to me as well.
Christian.
-Daniel
Christian.
if (_ret) {
pr_info("%pS callback failed with %d in %sblockable
context.\n",
mn->ops->invalidate_range_start, _ret,
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] mm, notifier: Catch sleeping/blocking for !blockable
On Thu, Nov 22, 2018 at 06:55:17PM +, Koenig, Christian wrote:
> Am 22.11.18 um 17:51 schrieb Daniel Vetter:
> > We need to make sure implementations don't cheat and don't have a
> > possible schedule/blocking point deeply burried where review can't
> > catch it.
> >
> > I'm not sure whether this is the best way to make sure all the
> > might_sleep() callsites trigger, and it's a bit ugly in the code flow.
> > But it gets the job done.
> >
> > Cc: Andrew Morton
> > Cc: Michal Hocko
> > Cc: David Rientjes
> > Cc: "Christian König"
> > Cc: Daniel Vetter
> > Cc: "Jérôme Glisse"
> > Cc: linux...@kvack.org
> > Signed-off-by: Daniel Vetter
> > ---
> > mm/mmu_notifier.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> > index 59e102589a25..4d282cfb296e 100644
> > --- a/mm/mmu_notifier.c
> > +++ b/mm/mmu_notifier.c
> > @@ -185,7 +185,13 @@ int __mmu_notifier_invalidate_range_start(struct
> > mm_struct *mm,
> > id = srcu_read_lock();
> > hlist_for_each_entry_rcu(mn, >mmu_notifier_mm->list, hlist) {
> > if (mn->ops->invalidate_range_start) {
> > - int _ret = mn->ops->invalidate_range_start(mn, mm,
> > start, end, blockable);
> > + int _ret;
> > +
> > + if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
> > + preempt_disable();
> > + _ret = mn->ops->invalidate_range_start(mn, mm, start,
> > end, blockable);
> > + if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
> > + preempt_enable();
>
> Just for the sake of better documenting this how about adding this to
> include/linux/kernel.h right next to might_sleep():
>
> #define disallow_sleeping_if(cond) for((cond) ? preempt_disable() :
> (void)0; (cond); preempt_disable())
>
> (Just from the back of my head, might contain peanuts and/or hints of
> errors).
I think these magic for blocks aren't used in the kernel. goto breaks
them, and we use goto a lot. I think a disallow/allow_sleep() pair with
the conditional preept_disable/enable() calls would be nice though. I can
do that if the overall idea sticks.
-Daniel
>
> Christian.
>
> > if (_ret) {
> > pr_info("%pS callback failed with %d in
> > %sblockable context.\n",
> >
> > mn->ops->invalidate_range_start, _ret,
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] mm, notifier: Catch sleeping/blocking for !blockable
Am 22.11.18 um 17:51 schrieb Daniel Vetter:
> We need to make sure implementations don't cheat and don't have a
> possible schedule/blocking point deeply burried where review can't
> catch it.
>
> I'm not sure whether this is the best way to make sure all the
> might_sleep() callsites trigger, and it's a bit ugly in the code flow.
> But it gets the job done.
>
> Cc: Andrew Morton
> Cc: Michal Hocko
> Cc: David Rientjes
> Cc: "Christian König"
> Cc: Daniel Vetter
> Cc: "Jérôme Glisse"
> Cc: linux...@kvack.org
> Signed-off-by: Daniel Vetter
> ---
> mm/mmu_notifier.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> index 59e102589a25..4d282cfb296e 100644
> --- a/mm/mmu_notifier.c
> +++ b/mm/mmu_notifier.c
> @@ -185,7 +185,13 @@ int __mmu_notifier_invalidate_range_start(struct
> mm_struct *mm,
> id = srcu_read_lock();
> hlist_for_each_entry_rcu(mn, >mmu_notifier_mm->list, hlist) {
> if (mn->ops->invalidate_range_start) {
> - int _ret = mn->ops->invalidate_range_start(mn, mm,
> start, end, blockable);
> + int _ret;
> +
> + if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
> + preempt_disable();
> + _ret = mn->ops->invalidate_range_start(mn, mm, start,
> end, blockable);
> + if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
> + preempt_enable();
Just for the sake of better documenting this how about adding this to
include/linux/kernel.h right next to might_sleep():
#define disallow_sleeping_if(cond) for((cond) ? preempt_disable() :
(void)0; (cond); preempt_disable())
(Just from the back of my head, might contain peanuts and/or hints of
errors).
Christian.
> if (_ret) {
> pr_info("%pS callback failed with %d in
> %sblockable context.\n",
>
> mn->ops->invalidate_range_start, _ret,
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] mm, notifier: Catch sleeping/blocking for !blockable
We need to make sure implementations don't cheat and don't have a
possible schedule/blocking point deeply burried where review can't
catch it.
I'm not sure whether this is the best way to make sure all the
might_sleep() callsites trigger, and it's a bit ugly in the code flow.
But it gets the job done.
Cc: Andrew Morton
Cc: Michal Hocko
Cc: David Rientjes
Cc: "Christian König"
Cc: Daniel Vetter
Cc: "Jérôme Glisse"
Cc: linux...@kvack.org
Signed-off-by: Daniel Vetter
---
mm/mmu_notifier.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
index 59e102589a25..4d282cfb296e 100644
--- a/mm/mmu_notifier.c
+++ b/mm/mmu_notifier.c
@@ -185,7 +185,13 @@ int __mmu_notifier_invalidate_range_start(struct mm_struct
*mm,
id = srcu_read_lock();
hlist_for_each_entry_rcu(mn, >mmu_notifier_mm->list, hlist) {
if (mn->ops->invalidate_range_start) {
- int _ret = mn->ops->invalidate_range_start(mn, mm,
start, end, blockable);
+ int _ret;
+
+ if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
+ preempt_disable();
+ _ret = mn->ops->invalidate_range_start(mn, mm, start,
end, blockable);
+ if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_SLEEP) && !blockable)
+ preempt_enable();
if (_ret) {
pr_info("%pS callback failed with %d in
%sblockable context.\n",
mn->ops->invalidate_range_start, _ret,
--
2.19.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
