| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 27 Jul 2017 20:58:38 +0000 (UTC)
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc: Avi Kivity <avi@...lladb.com>,
maged michael <maged.michael@...il.com>,
Andrew Hunter <ahh@...gle.com>, gromer@...gle.com,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: Udpated sys_membarrier() speedup patch, FYI
----- On Jul 27, 2017, at 4:37 PM, Paul E. McKenney paulmck@...ux.vnet.ibm.com wrote:
> On Thu, Jul 27, 2017 at 11:04:13PM +0300, Avi Kivity wrote:
>> On 07/27/2017 10:43 PM, Paul E. McKenney wrote:
>> >On Thu, Jul 27, 2017 at 10:20:14PM +0300, Avi Kivity wrote:
>> >>On 07/27/2017 09:12 PM, Paul E. McKenney wrote:
>> >>>Hello!
>> >>>
>> >>>Please see below for a prototype sys_membarrier() speedup patch.
>> >>>Please note that there is some controversy on this subject, so the final
>> >>>version will probably be quite a bit different than this prototype.
>> >>>
>> >>>But my main question is whether the throttling shown below is acceptable
>> >>>for your use cases, namely only one expedited sys_membarrier() permitted
>> >>>per scheduling-clock period (1 millisecond on many platforms), with any
>> >>>excess being silently converted to non-expedited form. The reason for
>> >>>the throttling is concerns about DoS attacks based on user code with a
>> >>>tight loop invoking this system call.
>> >>>
>> >>>Thoughts?
>> >>Silent throttling would render it useless for me. -EAGAIN is a
>> >>little better, but I'd be forced to spin until either I get kicked
>> >>out of my loop, or it succeeds.
>> >>
>> >>IPIing only running threads of my process would be perfect. In fact
>> >>I might even be able to make use of "membarrier these threads
>> >>please" to reduce IPIs, when I change the topology from fully
>> >>connected to something more sparse, on larger machines.
>> >>
>> >>My previous implementations were a signal (but that's horrible on
>> >>large machines) and trylock + mprotect (but that doesn't work on
>> >>ARM).
>> >OK, how about the following patch, which IPIs only the running
>> >threads of the process doing the sys_membarrier()?
>>
>> Works for me.
>
> Thank you for testing! I expect that Mathieu will have a v2 soon,
> hopefully CCing you guys. (If not, I will forward it.)
>
Will do!
> Mathieu, please note Avi's feedback below.
More below,
>
> Thanx, Paul
>
>> >------------------------------------------------------------------------
>> >
>> >From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
>> >To: Peter Zijlstra <peterz@...radead.org>
>> >Cc: linux-kernel@...r.kernel.org, Mathieu Desnoyers
>> > <mathieu.desnoyers@...icios.com>,
>> > "Paul E . McKenney" <paulmck@...ux.vnet.ibm.com>, Boqun Feng
>> > <boqun.feng@...il.com>
>> >Subject: [RFC PATCH] membarrier: expedited private command
>> >Date: Thu, 27 Jul 2017 14:59:43 -0400
>> >Message-Id: <20170727185943.11570-1-mathieu.desnoyers@...icios.com>
>> >
>> >Implement MEMBARRIER_CMD_PRIVATE_EXPEDITED with IPIs using cpumask built
>> >from all runqueues for which current thread's mm is the same as our own.
>> >
>> >Scheduler-wise, it requires that we add a memory barrier after context
>> >switching between processes (which have different mm).
>> >
>> >It would be interesting to benchmark the overhead of this added barrier
>> >on the performance of context switching between processes. If the
>> >preexisting overhead of switching between mm is high enough, the
>> >overhead of adding this extra barrier may be insignificant.
>> >
>> >[ Compile-tested only! ]
>> >
>> >CC: Peter Zijlstra <peterz@...radead.org>
>> >CC: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
>> >CC: Boqun Feng <boqun.feng@...il.com>
>> >Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
>> >---
>> > include/uapi/linux/membarrier.h | 8 +++--
>> > kernel/membarrier.c | 76 ++++++++++++++++++++++++++++++++++++++++-
>> > kernel/sched/core.c | 21 ++++++++++++
>> > 3 files changed, 102 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
>> >index e0b108bd2624..6a33c5852f6b 100644
>> >--- a/include/uapi/linux/membarrier.h
>> >+++ b/include/uapi/linux/membarrier.h
>> >@@ -40,14 +40,18 @@
>> > * (non-running threads are de facto in such a
>> > * state). This covers threads from all processes
>> > * running on the system. This command returns 0.
>> >+ * TODO: documentation.
>> > *
>> > * Command to be passed to the membarrier system call. The commands need to
>> > * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
>> > * the value 0.
>> > */
>> > enum membarrier_cmd {
>> >- MEMBARRIER_CMD_QUERY = 0,
>> >- MEMBARRIER_CMD_SHARED = (1 << 0),
>> >+ MEMBARRIER_CMD_QUERY = 0,
>> >+ MEMBARRIER_CMD_SHARED = (1 << 0),
>> >+ /* reserved for MEMBARRIER_CMD_SHARED_EXPEDITED (1 << 1) */
>> >+ /* reserved for MEMBARRIER_CMD_PRIVATE (1 << 2) */
>> >+ MEMBARRIER_CMD_PRIVATE_EXPEDITED = (1 << 3),
>> > };
>> >
>> > #endif /* _UAPI_LINUX_MEMBARRIER_H */
>> >diff --git a/kernel/membarrier.c b/kernel/membarrier.c
>> >index 9f9284f37f8d..8c6c0f96f617 100644
>> >--- a/kernel/membarrier.c
>> >+++ b/kernel/membarrier.c
>> >@@ -19,10 +19,81 @@
>> > #include <linux/tick.h>
>> >
>> > /*
>> >+ * XXX For cpu_rq(). Should we rather move
>> >+ * membarrier_private_expedited() to sched/core.c or create
>> >+ * sched/membarrier.c ?
>> >+ */
>> >+#include "sched/sched.h"
>> >+
>> >+/*
>> > * Bitmask made from a "or" of all commands within enum membarrier_cmd,
>> > * except MEMBARRIER_CMD_QUERY.
>> > */
>> >-#define MEMBARRIER_CMD_BITMASK (MEMBARRIER_CMD_SHARED)
>> >+#define MEMBARRIER_CMD_BITMASK \
>> >+ (MEMBARRIER_CMD_SHARED | MEMBARRIER_CMD_PRIVATE_EXPEDITED)
>> >+
>>
>> > rcu_read_unlock();
>> >+ }
>> >+}
>> >+
>> >+static void membarrier_private_expedited(void)
>> >+{
>> >+ int cpu, this_cpu;
>> >+ cpumask_var_t tmpmask;
>> >+
>> >+ if (num_online_cpus() == 1)
>> >+ return;
>> >+
>> >+ /*
>> >+ * Matches memory barriers around rq->curr modification in
>> >+ * scheduler.
>> >+ */
>> >+ smp_mb(); /* system call entry is not a mb. */
>> >+
>> >+ if (!alloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
>> >+ /* Fallback for OOM. */
>> >+ membarrier_private_expedited_ipi_each();
>> >+ goto end;
>> >+ }
>> >+
>> >+ this_cpu = raw_smp_processor_id();
>> >+ for_each_online_cpu(cpu) {
>> >+ struct task_struct *p;
>> >+
>> >+ if (cpu == this_cpu)
>> >+ continue;
>> >+ rcu_read_lock();
>> >+ p = task_rcu_dereference(&cpu_rq(cpu)->curr);
>> >+ if (p && p->mm == current->mm)
>> >+ __cpumask_set_cpu(cpu, tmpmask);
>>
>> This gets you some false positives, if the CPU idled then mm will
>> not have changed.
>
> Good point! The battery-powered embedded guys would probably prefer
> we not needlessly IPI idle CPUs. We cannot rely on RCU's dyntick-idle
> state in nohz_full cases. Not sure if is_idle_task() can be used
> safely, given things like play_idle().
Would changing the check in this loop to:
if (p && !is_idle_task(p) && p->mm == current->mm) {
work for you ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Powered by blists - more mailing lists