From: Ingo Molnar <mingo@kernel.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
linux-kernel@vger.kernel.org,
Peter Zijlstra <peterz@infradead.org>,
Mike Galbraith <efault@gmx.de>, Ingo Molnar <mingo@elte.hu>,
"Rafael J . Wysocki" <rjw@rjwysocki.net>
Subject: Re: [RFC PATCH] kernel: sched: Provide a pointer to the valid CPU mask
Date: Thu, 6 Apr 2017 13:02:15 +0200 [thread overview]
Message-ID: <20170406110215.GA1367@gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.20.1704061052370.1716@nanos>
* Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 6 Apr 2017, Ingo Molnar wrote:
>
> > Sorry if this is a back and forth - I was somehow convinced that we do need to
> > frob the cpus_allowed mask to get this functionality - but in hindsight I
> > think the counter should be enough.
> >
> > I.e. just have a counter and these two APIs:
> >
> > static inline void migrate_disable(void)
> > {
> > current->migration_disabled++;
> > }
> >
> > ...
> >
> > static inline void migrate_enable(void)
> > {
> > current->migration_disabled--;
> > }
> >
> > ... and make sure the scheduler migration code plus the CPU hotplug code considers
> > the counter.
>
> We tried that some time ago, but there are a lot of places in the scheduler
> which just rely on the cpus_allowed_mask, so we have to chase all of them and
> make sure that new users do not ignore that counter. That's why we chose the
> cpus_allowed_mask approach. And I still think that's the proper thing to do.
But but ...
The number of places in the scheduler where we actually end up migrating a task is
pretty limited:
try_to_wake_up():
- main wakeup code
migrate_swap():
- active NUMA-balancing feature
move_queued_task():
- hotplug CPU-down migration
- changing the affinity mask
The wakeup and NUMA balancing case is trivial to solve: it's an optimization and
we can skip the migration if migration is disabled.
CPU hotplug and changing the affinity mask are the more complex cases, because
there migrating or not migrating is a correctness issue:
- CPU hotplug has to be aware of this anyway, regardless of whether it's solved
via a counter of the affinity mask.
- Changing the affinity mask (set_cpus_allowed()) has two main cases:
the synchronous and asynchronous case:
- synchronous is when the current task changes its own affinity mask, this
should work fine mostly out of box, as we don't call set_cpus_allowed()
from inside migration disabled regions. (We can enforce this via a
debugging check.)
- The asynchronous case is when the affinity task of some other task is
changed - this would not have an immediate effect with migration-disabled
logic, the migration would be delayed to when migration is re-enabled
again.
As for general fragility, is there any reason why a simple debugging check in
set_task_cpu() would not catch most mishaps:
WARN_ON_ONCE(p->state != TASK_RUNNING && p->migration_disabled);
... or something like that?
I.e. my point is that I think using a counter would be much simpler, yet still as
robust and maintainable. We could in fact move migrate_disable()/enable() upstream
straight away and eliminate this small fork of functionality between mainline and
-rt.
Thanks,
Ingo
next prev parent reply other threads:[~2017-04-06 11:02 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-04 18:42 [RFC PATCH] kernel: sched: Provide a pointer to the valid CPU mask Sebastian Andrzej Siewior
2017-04-05 7:39 ` Ingo Molnar
2017-04-05 8:37 ` Sebastian Andrzej Siewior
2017-04-06 6:16 ` Ingo Molnar
2017-04-06 7:38 ` Sebastian Andrzej Siewior
2017-04-06 8:01 ` Ingo Molnar
2017-04-06 9:25 ` Sebastian Andrzej Siewior
2017-04-06 9:46 ` Peter Zijlstra
2017-04-06 10:58 ` Thomas Gleixner
2017-04-06 11:41 ` Peter Zijlstra
2017-04-06 9:35 ` Peter Zijlstra
2017-04-06 9:42 ` Peter Zijlstra
2017-04-06 10:36 ` Thomas Gleixner
2017-04-06 11:02 ` Ingo Molnar [this message]
2017-04-06 11:10 ` Thomas Gleixner
2017-04-07 7:13 ` Ingo Molnar
2017-04-06 9:34 ` Peter Zijlstra
2017-04-06 9:32 ` Peter Zijlstra
2017-04-06 9:46 ` Sebastian Andrzej Siewior
2017-04-06 10:35 ` Peter Zijlstra
2017-04-06 10:47 ` Thomas Gleixner
2017-04-06 10:57 ` Peter Zijlstra
2017-04-06 11:03 ` Thomas Gleixner
2017-04-06 11:50 ` Peter Zijlstra
2017-04-06 11:56 ` Thomas Gleixner
2017-04-06 12:31 ` Peter Zijlstra
2017-04-11 1:38 ` [lkp-robot] [kernel] c1f943ee40: kernel_BUG_at_kernel/smpboot.c kernel test robot
2017-04-11 1:38 ` kernel test robot
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=20170406110215.GA1367@gmail.com \
--to=mingo@kernel.org \
--cc=bigeasy@linutronix.de \
--cc=efault@gmx.de \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=peterz@infradead.org \
--cc=rjw@rjwysocki.net \
--cc=tglx@linutronix.de \
/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 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.