From: Michal Hocko <mhocko@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "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: Mon, 10 Dec 2018 17:20:10 +0100 [thread overview]
Message-ID: <20181210162010.GS1286@dhcp22.suse.cz> (raw)
In-Reply-To: <20181210152253.GP5289@hirez.programming.kicks-ass.net>
On Mon 10-12-18 16:22:53, Peter Zijlstra wrote:
> On Mon, Dec 10, 2018 at 04:01:59PM +0100, Michal Hocko wrote:
> > On Mon 10-12-18 15:47:11, Peter Zijlstra wrote:
> > > On Mon, Dec 10, 2018 at 03:13:37PM +0100, Michal Hocko wrote:
> > > > I do not see any scheduler guys Cced and it would be really great to get
> > > > their opinion here.
> > > >
> > > > On Mon 10-12-18 11:36:39, Daniel Vetter wrote:
> > > > > In some special cases we must not block, but there's not a
> > > > > spinlock, preempt-off, irqs-off or similar critical section already
> > > > > that arms the might_sleep() debug checks. Add a non_block_start/end()
> > > > > pair to annotate these.
> > > > >
> > > > > This will be used in the oom paths of mmu-notifiers, where blocking is
> > > > > not allowed to make sure there's forward progress.
> > > >
> > > > Considering the only alternative would be to abuse
> > > > preempt_{disable,enable}, and that really has a different semantic, I
> > > > think this makes some sense. The cotext is preemptible but we do not
> > > > want notifier to sleep on any locks, WQ etc.
> > >
> > > I'm confused... what is this supposed to do?
> > >
> > > And what does 'block' mean here? Without preempt_disable/IRQ-off we're
> > > subject to regular preemption and execution can stall for arbitrary
> > > amounts of time.
> >
> > The notifier is called from quite a restricted context - oom_reaper -
> > which shouldn't depend on any locks or sleepable conditionals.
>
> You want to exclude spinlocks too? We could maybe frob something with
> lockdep if you need that?
Spinlocks are less of a problem because you cannot have a (in)direct
dependency on the page allocator that would deadlock. Spinlocks, or
preemption disabled in general should be short enough to guarantee a
forward progress.
> > The code
> > should be swift as well but we mostly do care about it to make a forward
> > progress. Checking for sleepable context is the best thing we could come
> > up with that would describe these demands at least partially.
>
> 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.
> I suppose you could extend the check to include schedule_debug() as
> well, maybe something like:
Do you mean to make the check cheaper?
> 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
next prev parent reply other threads:[~2018-12-10 16:20 UTC|newest]
Thread overview: 15+ 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:44 ` Koenig, Christian
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 14:13 ` Michal Hocko
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 [this message]
2018-12-10 16:30 ` Peter Zijlstra
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
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
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=20181210162010.GS1286@dhcp22.suse.cz \
--to=mhocko@kernel.org \
--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=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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).