From: Daniel Vetter <daniel@ffwll.ch> To: Peter Zijlstra <peterz@infradead.org> Cc: "Michal Hocko" <mhocko@kernel.org>, "Daniel Vetter" <daniel.vetter@ffwll.ch>, "Intel Graphics Development" <intel-gfx@lists.freedesktop.org>, "DRI Development" <dri-devel@lists.freedesktop.org>, LKML <linux-kernel@vger.kernel.org>, linux-mm@kvack.org, "Andrew Morton" <akpm@linux-foundation.org>, "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 2/4] kernel.h: Add non_block_start/end() Date: Wed, 12 Dec 2018 11:26:56 +0100 [thread overview] Message-ID: <20181212102656.GS21184@phenom.ffwll.local> (raw) In-Reply-To: <20181210163009.GR5289@hirez.programming.kicks-ass.net> On Mon, Dec 10, 2018 at 05:30:09PM +0100, Peter Zijlstra wrote: > On Mon, Dec 10, 2018 at 05:20:10PM +0100, Michal Hocko wrote: > > > OK, no real objections to the thing. Just so long we're all on the same > > > page as to what it does and doesn't do ;-) > > > > I am not really sure whether there are other potential users besides > > this one and whether the check as such is justified. > > It's a debug option... > > > > I suppose you could extend the check to include schedule_debug() as > > > well, maybe something like: > > > > Do you mean to make the check cheaper? > > Nah, so the patch only touched might_sleep(), the below touches > schedule(). > > If there were a patch that hits schedule() without going through a > might_sleep() (rare in practise I think, but entirely possible) then you > won't get a splat without something like the below on top. We have a bunch of schedule() calls in i915, for e.g. waiting for multiple events at the same time (when we want to unblock if any of them fire). And there's no might_sleep in these cases afaict. Adding the check in schedule() sounds useful, I'll include your snippet in v2. Plus try a bit better to explain in the commit message why Michal suggested these. Thanks, Daniel > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > > index f66920173370..b1aaa278f1af 100644 > > > --- a/kernel/sched/core.c > > > +++ b/kernel/sched/core.c > > > @@ -3278,13 +3278,18 @@ static noinline void __schedule_bug(struct task_struct *prev) > > > /* > > > * Various schedule()-time debugging checks and statistics: > > > */ > > > -static inline void schedule_debug(struct task_struct *prev) > > > +static inline void schedule_debug(struct task_struct *prev, bool preempt) > > > { > > > #ifdef CONFIG_SCHED_STACK_END_CHECK > > > if (task_stack_end_corrupted(prev)) > > > panic("corrupted stack end detected inside scheduler\n"); > > > #endif > > > > > > +#ifdef CONFIG_DEBUG_ATOMIC_SLEEP > > > + if (!preempt && prev->state && prev->non_block_count) > > > + // splat > > > +#endif > > > + > > > if (unlikely(in_atomic_preempt_off())) { > > > __schedule_bug(prev); > > > preempt_count_set(PREEMPT_DISABLED); > > > @@ -3391,7 +3396,7 @@ static void __sched notrace __schedule(bool preempt) > > > rq = cpu_rq(cpu); > > > prev = rq->curr; > > > > > > - schedule_debug(prev); > > > + schedule_debug(prev, preempt); > > > > > > if (sched_feat(HRTICK)) > > > hrtick_clear(rq); > > > > -- > > Michal Hocko > > SUSE Labs -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Peter Zijlstra <peterz@infradead.org> Cc: "Daniel Vetter" <daniel.vetter@ffwll.ch>, "Intel Graphics Development" <intel-gfx@lists.freedesktop.org>, LKML <linux-kernel@vger.kernel.org>, "DRI Development" <dri-devel@lists.freedesktop.org>, "Michal Hocko" <mhocko@kernel.org>, 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 2/4] kernel.h: Add non_block_start/end() Date: Wed, 12 Dec 2018 11:26:56 +0100 [thread overview] Message-ID: <20181212102656.GS21184@phenom.ffwll.local> (raw) In-Reply-To: <20181210163009.GR5289@hirez.programming.kicks-ass.net> On Mon, Dec 10, 2018 at 05:30:09PM +0100, Peter Zijlstra wrote: > On Mon, Dec 10, 2018 at 05:20:10PM +0100, Michal Hocko wrote: > > > OK, no real objections to the thing. Just so long we're all on the same > > > page as to what it does and doesn't do ;-) > > > > I am not really sure whether there are other potential users besides > > this one and whether the check as such is justified. > > It's a debug option... > > > > I suppose you could extend the check to include schedule_debug() as > > > well, maybe something like: > > > > Do you mean to make the check cheaper? > > Nah, so the patch only touched might_sleep(), the below touches > schedule(). > > If there were a patch that hits schedule() without going through a > might_sleep() (rare in practise I think, but entirely possible) then you > won't get a splat without something like the below on top. We have a bunch of schedule() calls in i915, for e.g. waiting for multiple events at the same time (when we want to unblock if any of them fire). And there's no might_sleep in these cases afaict. Adding the check in schedule() sounds useful, I'll include your snippet in v2. Plus try a bit better to explain in the commit message why Michal suggested these. Thanks, Daniel > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > > index f66920173370..b1aaa278f1af 100644 > > > --- a/kernel/sched/core.c > > > +++ b/kernel/sched/core.c > > > @@ -3278,13 +3278,18 @@ static noinline void __schedule_bug(struct task_struct *prev) > > > /* > > > * Various schedule()-time debugging checks and statistics: > > > */ > > > -static inline void schedule_debug(struct task_struct *prev) > > > +static inline void schedule_debug(struct task_struct *prev, bool preempt) > > > { > > > #ifdef CONFIG_SCHED_STACK_END_CHECK > > > if (task_stack_end_corrupted(prev)) > > > panic("corrupted stack end detected inside scheduler\n"); > > > #endif > > > > > > +#ifdef CONFIG_DEBUG_ATOMIC_SLEEP > > > + if (!preempt && prev->state && prev->non_block_count) > > > + // splat > > > +#endif > > > + > > > if (unlikely(in_atomic_preempt_off())) { > > > __schedule_bug(prev); > > > preempt_count_set(PREEMPT_DISABLED); > > > @@ -3391,7 +3396,7 @@ static void __sched notrace __schedule(bool preempt) > > > rq = cpu_rq(cpu); > > > prev = rq->curr; > > > > > > - schedule_debug(prev); > > > + schedule_debug(prev, preempt); > > > > > > if (sched_feat(HRTICK)) > > > hrtick_clear(rq); > > > > -- > > 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
next prev parent reply other threads:[~2018-12-12 10:27 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-10 10:36 [PATCH 0/4] mmu notifier debug checks v2 Daniel Vetter 2018-12-10 10:36 ` [PATCH 1/4] mm: Check if mmu notifier callbacks are allowed to fail Daniel Vetter 2018-12-10 10:36 ` Daniel Vetter 2018-12-10 10:44 ` Koenig, Christian 2018-12-10 10:44 ` Koenig, Christian 2018-12-10 13:27 ` Michal Hocko 2018-12-10 13:27 ` Michal Hocko 2018-12-10 10:36 ` [PATCH 2/4] kernel.h: Add non_block_start/end() Daniel Vetter 2018-12-10 10:36 ` Daniel Vetter 2018-12-10 14:13 ` Michal Hocko 2018-12-10 14:13 ` Michal Hocko 2018-12-10 14:47 ` Peter Zijlstra 2018-12-10 14:47 ` Peter Zijlstra 2018-12-10 15:01 ` Michal Hocko 2018-12-10 15:22 ` Peter Zijlstra 2018-12-10 16:20 ` Michal Hocko 2018-12-10 16:30 ` Peter Zijlstra 2018-12-10 16:30 ` Peter Zijlstra 2018-12-12 10:26 ` Daniel Vetter [this message] 2018-12-12 10:26 ` Daniel Vetter 2018-12-10 10:36 ` [PATCH 3/4] mm, notifier: Catch sleeping/blocking for !blockable Daniel Vetter 2018-12-10 10:36 ` [PATCH 4/4] mm, notifier: Add a lockdep map for invalidate_range_start Daniel Vetter 2018-12-10 12:07 ` ✗ Fi.CI.CHECKPATCH: warning for mmu notifier debug checks v2 Patchwork 2018-12-10 12:28 ` ✓ Fi.CI.BAT: success " Patchwork 2018-12-10 15:54 ` ✗ Fi.CI.IGT: failure " Patchwork 2018-12-10 16:47 ` ✗ Fi.CI.BAT: failure for mmu notifier debug checks v2 (rev2) Patchwork 2019-05-20 21:39 [PATCH 1/4] mm: Check if mmu notifier callbacks are allowed to fail Daniel Vetter 2019-05-20 21:39 ` [PATCH 2/4] kernel.h: Add non_block_start/end() Daniel Vetter 2019-05-20 21:39 ` Daniel Vetter
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=20181212102656.GS21184@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=akpm@linux-foundation.org \ --cc=christian.koenig@amd.com \ --cc=daniel.vetter@ffwll.ch \ --cc=daniel.vetter@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=jglisse@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@kernel.org \ --cc=peterz@infradead.org \ --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.