Linux-api Archive on lore.kernel.org
 help / color / Atom feed
From: Frederic Weisbecker <frederic@kernel.org>
To: Alex Belits <abelits@marvell.com>
Cc: "mingo@kernel.org" <mingo@kernel.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"rostedt@goodmis.org" <rostedt@goodmis.org>,
	Prasun Kapoor <pkapoor@marvell.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-api@vger.kernel.org" <linux-api@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"catalin.marinas@arm.com" <catalin.marinas@arm.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	"will@kernel.org" <will@kernel.org>
Subject: Re: [EXT] Re: [PATCH 11/12] task_isolation: kick_all_cpus_sync: don't kick isolated cpus
Date: Mon, 9 Mar 2020 03:28:30 +0100
Message-ID: <20200309022829.GB9615@lenoir> (raw)
In-Reply-To: <7e0ce8988f4811e7453859e22654d2618557d9ab.camel@marvell.com>

On Sun, Mar 08, 2020 at 06:48:43AM +0000, Alex Belits wrote:
> On Fri, 2020-03-06 at 16:34 +0100, Frederic Weisbecker wrote:
> > On Wed, Mar 04, 2020 at 04:15:24PM +0000, Alex Belits wrote:
> > > From: Yuri Norov <ynorov@marvell.com>
> > > 
> > > Make sure that kick_all_cpus_sync() does not call CPUs that are
> > > running
> > > isolated tasks.
> > > 
> > > Signed-off-by: Alex Belits <abelits@marvell.com>
> > > ---
> > >  kernel/smp.c | 14 +++++++++++++-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/smp.c b/kernel/smp.c
> > > index 3a8bcbdd4ce6..d9b4b2fedfed 100644
> > > --- a/kernel/smp.c
> > > +++ b/kernel/smp.c
> > > @@ -731,9 +731,21 @@ static void do_nothing(void *unused)
> > >   */
> > >  void kick_all_cpus_sync(void)
> > >  {
> > > +	struct cpumask mask;
> > > +
> > >  	/* Make sure the change is visible before we kick the cpus */
> > >  	smp_mb();
> > > -	smp_call_function(do_nothing, NULL, 1);
> > > +
> > > +	preempt_disable();
> > > +#ifdef CONFIG_TASK_ISOLATION
> > > +	cpumask_clear(&mask);
> > > +	task_isolation_cpumask(&mask);
> > > +	cpumask_complement(&mask, &mask);
> > > +#else
> > > +	cpumask_setall(&mask);
> > > +#endif
> > > +	smp_call_function_many(&mask, do_nothing, NULL, 1);
> > > +	preempt_enable();
> > >  }
> > 
> > That looks very dangerous, the callers of kick_all_cpus_sync() want
> > to
> > sync all CPUs for a reason. You will rather need to fix the callers.
> 
> All callers of this use this function to synchronize IPIs and icache,
> and they have no idea if there is anything special about the state of
> CPUs. If a task is isolated, this call would not be necessary because
> the task is in userspace, and it would have to enter kernel for any of
> that to become relevant but then it will have to switch from userspace
> to kernel. At worst it is returning to userspace after entering
> isolation or back in kernel running cleanup after isolation is broken
> but before tsk_thread_flags_cache is updated. There will be nothing to
> run on the same CPU because we have just left isolation, so task will
> either exit or go back to userspace.
> 
> Is there any reason for a race at that point?


I can imagine several races:

1) The isolated task has set the cpumask but hasn't exited the kernel
yet. If it still runs kernel code while kick_all_cpus_sync() has completed,
we fail.

2) The isolated task is running do_exit() but the caller of kick_all_cpus_sync()
still sees the target as part of the isolated mask.

3) The isolated task has just set the isolated cpumask and entered userspace
but the caller still don't see the new value in the isolated cpumask, so it sends
the IPI to the isolated CPU.

Besides, any caller of kick_all_cpus_sync() is in its right to expect that
everything preceding the call to that function is visible to all CPUs
after that call. If you spare that IPI to an isolated CPU, what ensures
it will see what it is supposed to once it calls do_exit() or prctl()?

Is there a way we could fix the callers instead? For example synchronize_rcu()
could be a replacement (it handles very well nohz_full CPUs), provided the
callsites can sleep. It seems to be the case for __do_tune_cpucache() at least.

flush_icache_range() is scarier I have to admit, doesn't look like it can
sleep.


> > Thanks.
> > 
> > >  EXPORT_SYMBOL_GPL(kick_all_cpus_sync);
> > >  
> > > -- 
> > > 2.20.1
> > > 
> 
> -- 
> Alex

  reply index

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-04 16:01 [PATCH 00/12] "Task_isolation" mode Alex Belits
2020-03-04 16:03 ` [PATCH 01/12] task_isolation: vmstat: add quiet_vmstat_sync function Alex Belits
2020-03-04 16:04 ` [PATCH 02/12] task_isolation: vmstat: add vmstat_idle function Alex Belits
2020-03-04 16:07 ` [PATCH 03/12] task_isolation: userspace hard isolation from kernel Alex Belits
2020-03-05 18:33   ` Frederic Weisbecker
2020-03-08  5:32     ` [EXT] " Alex Belits
2020-04-28 14:12     ` Marcelo Tosatti
2020-03-06 15:26   ` Frederic Weisbecker
2020-03-08  6:06     ` [EXT] " Alex Belits
2020-03-06 16:00   ` Frederic Weisbecker
2020-03-08  7:16     ` [EXT] " Alex Belits
2020-03-04 16:08 ` [PATCH 04/12] task_isolation: Add task isolation hooks to arch-independent code Alex Belits
2020-03-04 16:09 ` [PATCH 05/12] task_isolation: arch/x86: enable task isolation functionality Alex Belits
2020-03-04 16:10 ` [PATCH 06/12] task_isolation: arch/arm64: " Alex Belits
2020-03-04 16:31   ` Mark Rutland
2020-03-08  4:48     ` [EXT] " Alex Belits
2020-03-04 16:11 ` [PATCH 07/12] task_isolation: arch/arm: " Alex Belits
2020-03-04 16:12 ` [PATCH 08/12] task_isolation: don't interrupt CPUs with tick_nohz_full_kick_cpu() Alex Belits
2020-03-06 16:03   ` Frederic Weisbecker
2020-03-08  7:28     ` [EXT] " Alex Belits
2020-03-09  2:38       ` Frederic Weisbecker
2020-03-04 16:13 ` [PATCH 09/12] task_isolation: net: don't flush backlog on CPUs running isolated tasks Alex Belits
2020-03-04 16:14 ` [PATCH 10/12] task_isolation: ringbuffer: don't interrupt CPUs running isolated tasks on buffer resize Alex Belits
2020-03-04 16:15 ` [PATCH 11/12] task_isolation: kick_all_cpus_sync: don't kick isolated cpus Alex Belits
2020-03-06 15:34   ` Frederic Weisbecker
2020-03-08  6:48     ` [EXT] " Alex Belits
2020-03-09  2:28       ` Frederic Weisbecker [this message]
2020-03-04 16:16 ` [PATCH 12/12] task_isolation: CONFIG_TASK_ISOLATION prevents distribution of jobs to non-housekeeping CPUs Alex Belits
2020-03-08  3:42 ` [PATCH v2 00/12] "Task_isolation" mode Alex Belits
2020-03-08  3:44   ` [PATCH v2 01/12] task_isolation: vmstat: add quiet_vmstat_sync function Alex Belits
2020-03-08  3:46   ` [PATCH v2 02/12] task_isolation: vmstat: add vmstat_idle function Alex Belits
2020-03-08  3:47   ` [PATCH v2 03/12] task_isolation: userspace hard isolation from kernel Alex Belits
     [not found]     ` <20200307214254.7a8f6c22@hermes.lan>
2020-03-08  7:33       ` [EXT] " Alex Belits
2020-03-27  8:42     ` Marta Rybczynska
2020-04-06  4:31     ` Kevyn-Alexandre Paré
2020-04-06  4:43     ` Kevyn-Alexandre Paré
2020-03-08  3:48   ` [PATCH v2 04/12] task_isolation: Add task isolation hooks to arch-independent code Alex Belits
2020-03-08  3:49   ` [PATCH v2 05/12] task_isolation: arch/x86: enable task isolation functionality Alex Belits
2020-03-08  3:50   ` [PATCH v2 06/12] task_isolation: arch/arm64: " Alex Belits
2020-03-09 16:59     ` Mark Rutland
2020-03-08  3:52   ` [PATCH v2 07/12] task_isolation: arch/arm: " Alex Belits
2020-03-08  3:53   ` [PATCH v2 08/12] task_isolation: don't interrupt CPUs with tick_nohz_full_kick_cpu() Alex Belits
2020-03-08  3:54   ` [PATCH v2 09/12] task_isolation: net: don't flush backlog on CPUs running isolated tasks Alex Belits
2020-03-08  3:55   ` [PATCH v2 10/12] task_isolation: ringbuffer: don't interrupt CPUs running isolated tasks on buffer resize Alex Belits
2020-04-06  4:27     ` Kevyn-Alexandre Paré
2020-03-08  3:56   ` [PATCH v2 11/12] task_isolation: kick_all_cpus_sync: don't kick isolated cpus Alex Belits
2020-03-08  3:57   ` [PATCH v2 12/12] task_isolation: CONFIG_TASK_ISOLATION prevents distribution of jobs to non-housekeeping CPUs Alex Belits
2020-04-09 15:09   ` [PATCH v3 00/13] "Task_isolation" mode Alex Belits
2020-04-09 15:15     ` [PATCH 01/13] task_isolation: vmstat: add quiet_vmstat_sync function Alex Belits
2020-04-09 15:16     ` [PATCH 02/13] task_isolation: vmstat: add vmstat_idle function Alex Belits
2020-04-09 15:17     ` [PATCH v3 03/13] task_isolation: add instruction synchronization memory barrier Alex Belits
2020-04-15 12:44       ` Mark Rutland
2020-04-19  5:02         ` [EXT] " Alex Belits
2020-04-20 12:23           ` Will Deacon
2020-04-20 12:36             ` Mark Rutland
2020-04-20 13:55               ` Will Deacon
2020-04-21  7:41                 ` Will Deacon
2020-04-20 12:45           ` Mark Rutland
2020-04-09 15:20     ` [PATCH v3 04/13] task_isolation: userspace hard isolation from kernel Alex Belits
2020-04-09 18:00       ` Andy Lutomirski
2020-04-19  5:07         ` Alex Belits
2020-04-09 15:21     ` [PATCH 05/13] task_isolation: Add task isolation hooks to arch-independent code Alex Belits
2020-04-09 15:22     ` [PATCH 06/13] task_isolation: arch/x86: enable task isolation functionality Alex Belits
2020-04-09 15:23     ` [PATCH v3 07/13] task_isolation: arch/arm64: " Alex Belits
2020-04-22 12:08       ` Catalin Marinas
2020-04-09 15:24     ` [PATCH v3 08/13] task_isolation: arch/arm: " Alex Belits
2020-04-09 15:25     ` [PATCH v3 09/13] task_isolation: don't interrupt CPUs with tick_nohz_full_kick_cpu() Alex Belits
2020-04-09 15:26     ` [PATCH v3 10/13] task_isolation: net: don't flush backlog on CPUs running isolated tasks Alex Belits
2020-04-09 15:27     ` [PATCH v3 11/13] task_isolation: ringbuffer: don't interrupt CPUs running isolated tasks on buffer resize Alex Belits
2020-04-09 15:27     ` [PATCH v3 12/13] task_isolation: kick_all_cpus_sync: don't kick isolated cpus Alex Belits
2020-04-09 15:28     ` [PATCH v3 13/13] task_isolation: CONFIG_TASK_ISOLATION prevents distribution of jobs to non-housekeeping CPUs Alex Belits

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=20200309022829.GB9615@lenoir \
    --to=frederic@kernel.org \
    --cc=abelits@marvell.com \
    --cc=catalin.marinas@arm.com \
    --cc=davem@davemloft.net \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=pkapoor@marvell.com \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=will@kernel.org \
    /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

Linux-api Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-api/0 linux-api/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-api linux-api/ https://lore.kernel.org/linux-api \
		linux-api@vger.kernel.org
	public-inbox-index linux-api

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-api


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git