* Re: [PATCH v6 6/9] nvmem: qfprom: Add Qualcomm QFPROM support.
From: Stephen Boyd @ 2015-06-24 22:07 UTC (permalink / raw)
To: Srinivas Kandagatla,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Greg Kroah-Hartman
Cc: Maxime Ripard, Rob Herring, Kumar Gala, Mark Brown,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, arnd-r2nGTMty4D4,
pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w,
mporter-OWPKS81ov/FWk0Htik3J/w, stefan.wahren-eS4NqCHxEME,
wxt-TNX95d0MmH7DzftRWevZcw
In-Reply-To: <1435014556-26392-1-git-send-email-srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On 06/22/2015 04:09 PM, Srinivas Kandagatla wrote:
> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> new file mode 100644
> index 0000000..7f7a82f
> --- /dev/null
> +++ b/drivers/nvmem/qfprom.c
> @@ -0,0 +1,89 @@
>
> +
> +#include <linux/platform_device.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
Is this include used?
> +#include <linux/module.h>
> +#include <linux/delay.h>
Is this include used?
> +#include <linux/of.h>
> +
> +static struct regmap_config qfprom_regmap_config = {
Can this be const?
> + .reg_bits = 32,
> + .val_bits = 8,
> + .reg_stride = 1,
> +};
> +
> +static struct nvmem_config econfig = {
And this one too?
> + .name = "qfprom",
> + .owner = THIS_MODULE,
> +};
> +
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* [RFC PATCH 1/3] restartable sequences: user-space per-cpu critical sections
From: Paul Turner @ 2015-06-24 22:26 UTC (permalink / raw)
To: Peter Zijlstra, Paul E. McKenney, Mathieu Desnoyers
Cc: Andrew Hunter, Andi Kleen, Lai Jiangshan,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steven Rostedt,
Josh Triplett, Ingo Molnar, Andrew Morton, Andy Lutomirski,
Linus Torvalds, Chris Lameter
In-Reply-To: <20150624222609.6116.86035.stgit-tdHu5vqousHHt/MElyovVYaSKrA+ACpX0E9HWUfgJXw@public.gmane.org>
Introduce the notion of 'restartable sequence'. This is a user-defined range
within which we guarantee user-execution will occur serially with respect
to scheduling events such as migration or competition with other threads.
Preemption, or other interruption within this region, results in control being
transferred to a user-defined restart handler when rescheduled. This handler
may arrange for the original operation to be retried, including potentially
resynchronizing with dependent state that may have been updated in the interim.
This may be used in combination with an in-memory cpu-id to allow user programs
to implement cpu-local data-structures and primitives, without the use/overhead
of any atomics.
The kernel ABI generally consists of:
- A single (per-address space) critical region
- A restart handler which pairs with the region above
- A (per-thread) memory location which will be kept current with its cpu
The definition of the above is performed via a new syscall,
SYSCALL_DEFINE5(restartable_sequences,
int, op, int, flags, long, val1, long, val2, long, val3)
There are currently 2 possible operations,
1) Configure the critical region (and restart handler)
2) Configure the per-thread cpu pointer
[ See kernel/restartable_sequences.c for full documentation ]
A thread that has not configured (2) will not be restarted when executing in
(1).
Note that while the kernel only sees a single critical region, arbitrarily many
sequences can be composed via multiplexing of the user-space restart handler.
This patch introduces the general framework for configuration, as well as
exposing the syscall. We minimally expose x86 as having support (even though
the actual ABI is added by a subsequent patch) so that this can be compile
tested in isolation.
Signed-off-by: Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
arch/Kconfig | 7 +
arch/x86/Kconfig | 1
arch/x86/syscalls/syscall_64.tbl | 1
fs/exec.c | 1
include/linux/sched.h | 28 ++++++
include/uapi/asm-generic/unistd.h | 5 +
init/Kconfig | 9 ++
kernel/Makefile | 1
kernel/restartable_sequences.c | 185 +++++++++++++++++++++++++++++++++++++
kernel/sched/core.c | 4 +
kernel/sched/sched.h | 3 +
kernel/sys_ni.c | 3 +
12 files changed, 246 insertions(+), 2 deletions(-)
create mode 100644 kernel/restartable_sequences.c
diff --git a/arch/Kconfig b/arch/Kconfig
index a65eafb..fb31981 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -229,6 +229,13 @@ config HAVE_REGS_AND_STACK_ACCESS_API
declared in asm/ptrace.h
For example the kprobes-based event tracer needs this API.
+config HAVE_RESTARTABLE_SEQUENCE_SUPPORT
+ bool
+ depends on HAVE_REGS_AND_STACK_ACCESS_API
+ help
+ This symbol should be selected by an architecture if it supports an
+ implementation of restartable sequences.
+
config HAVE_CLK
bool
help
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 8fec044..9c9c92f 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -67,6 +67,7 @@ config X86
select HAVE_EFFICIENT_UNALIGNED_ACCESS
select USER_STACKTRACE_SUPPORT
select HAVE_REGS_AND_STACK_ACCESS_API
+ select HAVE_RESTARTABLE_SEQUENCE_SUPPORT
select HAVE_DMA_API_DEBUG
select HAVE_KERNEL_GZIP
select HAVE_KERNEL_BZIP2
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index 9ef32d5..1de5cbc 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -329,6 +329,7 @@
320 common kexec_file_load sys_kexec_file_load
321 common bpf sys_bpf
322 64 execveat stub_execveat
+323 common restartable_sequences sys_restartable_sequences
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/fs/exec.c b/fs/exec.c
index 1977c2a..acd38f6 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1590,6 +1590,7 @@ static int do_execveat_common(int fd, struct filename *filename,
current->in_execve = 0;
acct_update_integrals(current);
task_numa_free(current);
+ rseq_clear_state_exec();
free_bprm(bprm);
kfree(pathbuf);
putname(filename);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index af0eeba..0540735 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1178,6 +1178,22 @@ struct mempolicy;
struct pipe_inode_info;
struct uts_namespace;
+#ifdef CONFIG_RESTARTABLE_SEQUENCES
+struct restartable_sequence_state {
+ /* Start and end of an address space's critical section. */
+ void __user *crit_start, __user *crit_end;
+ /* Where preempted threads will be restarted. */
+ void __user *crit_restart;
+ /* Thread's current CPU, typically in TLS. */
+ int __user *cpu_pointer;
+ struct preempt_notifier notifier;
+};
+
+void rseq_clear_state_exec(void);
+#else
+static inline void rseq_clear_state_exec(void) {}
+#endif
+
struct load_weight {
unsigned long weight;
u32 inv_weight;
@@ -1793,6 +1809,9 @@ struct task_struct {
#ifdef CONFIG_DEBUG_ATOMIC_SLEEP
unsigned long task_state_change;
#endif
+#ifdef CONFIG_RESTARTABLE_SEQUENCES
+ struct restartable_sequence_state rseq_state;
+#endif
int pagefault_disabled;
};
@@ -3167,4 +3186,13 @@ static inline unsigned long rlimit_max(unsigned int limit)
return task_rlimit_max(current, limit);
}
+#ifdef CONFIG_RESTARTABLE_SEQUENCES
+static inline int rseq_active(struct task_struct *p)
+{
+ return p->rseq_state.cpu_pointer != NULL;
+}
+#else
+static inline int rseq_active(struct task_struct *p) { return 0; }
+#endif
+
#endif
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index e016bd9..6173f56 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -709,9 +709,10 @@ __SYSCALL(__NR_memfd_create, sys_memfd_create)
__SYSCALL(__NR_bpf, sys_bpf)
#define __NR_execveat 281
__SC_COMP(__NR_execveat, sys_execveat, compat_sys_execveat)
-
+#define __NR_restartable_sequences 282
+__SYSCALL(__NR_restartable_sequences, sys_restartable_sequences)
#undef __NR_syscalls
-#define __NR_syscalls 282
+#define __NR_syscalls 283
/*
* All syscalls below here should go away really,
diff --git a/init/Kconfig b/init/Kconfig
index 81050e4..a597e30 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2010,6 +2010,15 @@ source "block/Kconfig"
config PREEMPT_NOTIFIERS
bool
+config RESTARTABLE_SEQUENCES
+ bool "Userspace Restartable Sequences (RSEQ)"
+ default n
+ depends on HAVE_RESTARTABLE_SEQUENCE_SUPPORT && PREEMPT_NOTIFIERS
+ help
+ Allows binaries to define a region of user-text within which
+ execution will be restarted in the event of signal delivery or
+ preemption.
+
config PADATA
depends on SMP
bool
diff --git a/kernel/Makefile b/kernel/Makefile
index 60c302c..01eea12 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
obj-$(CONFIG_JUMP_LABEL) += jump_label.o
obj-$(CONFIG_CONTEXT_TRACKING) += context_tracking.o
obj-$(CONFIG_TORTURE_TEST) += torture.o
+obj-$(CONFIG_RESTARTABLE_SEQUENCES) += restartable_sequences.o
$(obj)/configs.o: $(obj)/config_data.h
diff --git a/kernel/restartable_sequences.c b/kernel/restartable_sequences.c
new file mode 100644
index 0000000..72945f2
--- /dev/null
+++ b/kernel/restartable_sequences.c
@@ -0,0 +1,185 @@
+/*
+ * Restartable Sequences are a lightweight interface that allows user-level
+ * code to be executed atomically relative to scheduler preemption. Typically
+ * used for implementing per-cpu operations.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ *
+ * Copyright (C) 2015, Google, Inc.,
+ * Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> and Andrew Hunter <ahh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
+ *
+ */
+
+#ifdef CONFIG_RESTARTABLE_SEQUENCES
+
+#include <linux/uaccess.h>
+#include <linux/preempt.h>
+#include <linux/syscalls.h>
+
+static void rseq_sched_in_nop(struct preempt_notifier *pn, int cpu) {}
+static void rseq_sched_out_nop(struct preempt_notifier *pn,
+ struct task_struct *next) {}
+
+static __read_mostly struct preempt_ops rseq_preempt_ops = {
+ .sched_in = rseq_sched_in_nop,
+ .sched_out = rseq_sched_out_nop,
+};
+
+int rseq_register_cpu_pointer_current(int __user *cpu_pointer)
+{
+ struct restartable_sequence_state *rseq_state =
+ ¤t->rseq_state;
+ int registered = 0, rc = 0;
+
+ if (cpu_pointer == rseq_state->cpu_pointer)
+ return 0;
+
+ if (cpu_pointer && !access_ok(VERIFY_WRITE, cpu_pointer, sizeof(int)))
+ return -EINVAL;
+
+ rcu_read_lock();
+ /* Group leader always holds critical section definition. */
+ if (cpu_pointer && !current->group_leader->rseq_state.crit_restart) {
+ rc = -EINVAL;
+ goto out_unlock;
+ }
+ smp_rmb(); /* Pairs with setting crit_restart. */
+
+ if (rseq_state->cpu_pointer)
+ registered = 1;
+ rseq_state->cpu_pointer = cpu_pointer;
+
+ if (cpu_pointer && !registered) {
+ preempt_notifier_init(&rseq_state->notifier,
+ &rseq_preempt_ops);
+ preempt_notifier_register(&rseq_state->notifier);
+ } else if (!cpu_pointer && registered) {
+ preempt_notifier_unregister(&rseq_state->notifier);
+ }
+
+ /* Will update *cpu_pointer on return. */
+ if (cpu_pointer)
+ set_thread_flag(TIF_NOTIFY_RESUME);
+out_unlock:
+ rcu_read_unlock();
+
+ return 0;
+}
+
+void rseq_clear_state_exec()
+{
+ /* Ensure notifier is disabled. */
+ rseq_register_cpu_pointer_current(NULL);
+ memset(¤t->rseq_state, 0, sizeof(current->rseq_state));
+}
+
+static DEFINE_MUTEX(rseq_state_mutex);
+
+int rseq_register_critical_current(__user void *start, __user void *end,
+ __user void *restart)
+{
+ struct restartable_sequence_state *rseq_state;
+ int rc = 0;
+
+ rcu_read_lock();
+ /* The critical section is shared by all threads in a process. */
+ rseq_state = ¤t->group_leader->rseq_state;
+
+ /* [start,end) must not overlap with the restart handler. */
+ if (start >= end || (restart >= start && restart < end)) {
+ rc = -EINVAL;
+ goto out_rcu;
+ }
+
+ if (!access_ok(VERIFY_READ, start, end - start) ||
+ !access_ok(VERIFY_READ, restart, 1)) {
+ rc = -EINVAL;
+ goto out_rcu;
+ }
+
+ mutex_lock(&rseq_state_mutex);
+ /*
+ * We (currently) only allow RSEQ to be configured once. This
+ * simplifies synchronization with updates and reduces the risk of
+ * colliding critical sections.
+ */
+ if (rseq_state->crit_restart) {
+ rc = -EBUSY;
+ } else {
+ rseq_state->crit_start = start;
+ rseq_state->crit_end = end;
+ smp_wmb(); /* synchronize on visibility of crit_restart. */
+ rseq_state->crit_restart = restart;
+ }
+ mutex_unlock(&rseq_state_mutex);
+out_rcu:
+ rcu_read_unlock();
+ return rc;
+}
+
+#define SYS_RSEQ_SET_CRITICAL 0
+#define SYS_RSEQ_SET_CPU_POINTER 1
+
+/*
+ * RSEQ syscall interface.
+ *
+ * Usage:
+ * SYS_RSEQ_SET_CRITICAL, flags, crit_start, crit_end, crit_restart)
+ * A thread with user rip in (crit_start, crit_end] that has called
+ * RSEQ_SET_CPU_POINTER will have its execution resumed at crit_restart
+ * when interrupted by preemption or signal.
+ *
+ * SYS_RSEQ_SET_CPU_POINTER, flags, cpu_pointer_address
+ * Configures a (typically per-thread) value, containing the cpu which that
+ * thread is currently executing on.
+ * REQUIRES: SYS_RSEQ_SET_CRITICAL must have previously been called.
+ *
+ * flags is currently unused.
+ *
+ * Note: RSEQ_SET_CRITICAL may currently only be called once within an address
+ * space. This more general (e.g. by using RCU to synchronize region updates).
+ * However, that also introduces the risk of corruption in the case that more
+ * than one caller compete for control of the critical section.
+ */
+SYSCALL_DEFINE5(restartable_sequences,
+ int, op, int, flags, long, val1, long, val2, long, val3)
+{
+ int rc = -EINVAL;
+
+ if (op == SYS_RSEQ_SET_CRITICAL) {
+ /* Defines (process-wide) critical section. */
+ __user void *crit_start = (__user void *)val1;
+ __user void *crit_end = (__user void *)val2;
+ __user void *crit_restart = (__user void *)val3;
+ rc = rseq_register_critical_current(
+ crit_start, crit_end, crit_restart);
+ } else if (op == SYS_RSEQ_SET_CPU_POINTER) {
+ /*
+ * Enables RSEQ for this thread; sets location for CPU update
+ * to val1.
+ */
+ int __user *cpu = (int __user *)val1;
+ rc = rseq_register_cpu_pointer_current(cpu);
+ }
+
+ return rc;
+}
+#else
+SYSCALL_DEFINE0(restartable_sequences)
+{
+ return -ENOSYS;
+}
+#endif
+
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 921a754..1113565 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1868,6 +1868,10 @@ static void __sched_fork(unsigned long clone_flags, struct task_struct *p)
p->numa_group = NULL;
#endif /* CONFIG_NUMA_BALANCING */
+
+#ifdef CONFIG_RESTARTABLE_SEQUENCES
+ memset(&p->rseq_state, 0, sizeof(p->rseq_state));
+#endif
}
#ifdef CONFIG_NUMA_BALANCING
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index f10a445..24d4fac 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -947,6 +947,9 @@ static inline void __set_task_cpu(struct task_struct *p, unsigned int cpu)
{
set_task_rq(p, cpu);
#ifdef CONFIG_SMP
+ if (rseq_active(p))
+ set_tsk_thread_flag(p, TIF_NOTIFY_RESUME);
+
/*
* After ->cpu is set up to a new value, task_rq_lock(p, ...) can be
* successfuly executed on another CPU. We must ensure that updates of
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 7995ef5..4b109d9 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -243,3 +243,6 @@ cond_syscall(sys_bpf);
/* execveat */
cond_syscall(sys_execveat);
+
+/* restartable sequences */
+cond_syscall(sys_restartable_sequences);
^ permalink raw reply related
* [RFC PATCH 2/3] restartable sequences: x86 ABI
From: Paul Turner @ 2015-06-24 22:26 UTC (permalink / raw)
To: Peter Zijlstra, Paul E. McKenney, Mathieu Desnoyers
Cc: Andrew Hunter, Andi Kleen, Lai Jiangshan,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steven Rostedt,
Josh Triplett, Ingo Molnar, Andrew Morton, Andy Lutomirski,
Linus Torvalds, Chris Lameter
In-Reply-To: <20150624222609.6116.86035.stgit-tdHu5vqousHHt/MElyovVYaSKrA+ACpX0E9HWUfgJXw@public.gmane.org>
Implements the x86 (i386 & x86-64) ABIs for interrupting and restarting
execution within restartable sequence sections.
With respect to the x86-specific ABI:
On 32-bit: Upon restart, the interrupted rip is placed in %ecx
On 64-bit (or x32): Upon restart, the interrupted rip is placed in %r10
While potentially surprising at first glance, this choice is strongly motivated
by the fact that the available scratch registers under the i386 function call
ABI overlap with those used as argument registers under x86_64.
Given that sequences are already personality specific and that we always want
the arguments to be available for sequence restart, it's much more natural to
ultimately differentiate the ABI in these two cases.
Signed-off-by: Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
arch/x86/include/asm/restartable_sequences.h | 50 +++++++++++++++++++
arch/x86/kernel/Makefile | 2 +
arch/x86/kernel/restartable_sequences.c | 69 ++++++++++++++++++++++++++
arch/x86/kernel/signal.c | 12 +++++
kernel/restartable_sequences.c | 11 +++-
5 files changed, 141 insertions(+), 3 deletions(-)
create mode 100644 arch/x86/include/asm/restartable_sequences.h
create mode 100644 arch/x86/kernel/restartable_sequences.c
diff --git a/arch/x86/include/asm/restartable_sequences.h b/arch/x86/include/asm/restartable_sequences.h
new file mode 100644
index 0000000..0ceb024
--- /dev/null
+++ b/arch/x86/include/asm/restartable_sequences.h
@@ -0,0 +1,50 @@
+#ifndef _ASM_X86_RESTARTABLE_SEQUENCES_H
+#define _ASM_X86_RESTARTABLE_SEQUENCES_H
+
+#include <asm/processor.h>
+#include <asm/ptrace.h>
+#include <linux/sched.h>
+
+#ifdef CONFIG_RESTARTABLE_SEQUENCES
+
+static inline bool arch_rseq_in_crit_section(struct task_struct *p,
+ struct pt_regs *regs)
+{
+ struct task_struct *leader = p->group_leader;
+ struct restartable_sequence_state *rseq_state = &leader->rseq_state;
+
+ unsigned long ip = (unsigned long)regs->ip;
+ if (unlikely(ip < (unsigned long)rseq_state->crit_end &&
+ ip >= (unsigned long)rseq_state->crit_start))
+ return true;
+
+ return false;
+}
+
+static inline bool arch_rseq_needs_notify_resume(struct task_struct *p)
+{
+#ifdef CONFIG_PREEMPT
+ /*
+ * Under CONFIG_PREEMPT it's possible for regs to be incoherent in the
+ * case that we took an interrupt during syscall entry. Avoid this by
+ * always deferring to our notify-resume handler.
+ */
+ return true;
+#else
+ return arch_rseq_in_crit_section(p, task_pt_regs(p));
+#endif
+}
+
+void arch_rseq_handle_notify_resume(struct pt_regs *regs);
+void arch_rseq_check_critical_section(struct task_struct *p,
+ struct pt_regs *regs);
+
+#else /* !CONFIG_RESTARTABLE_SEQUENCES */
+
+static inline void arch_rseq_handle_notify_resume(struct pt_regs *regs) {}
+static inline void arch_rseq_check_critical_section(struct task_struct *p,
+ struct pt_regs *regs) {}
+
+#endif
+
+#endif /* _ASM_X86_RESTARTABLE_SEQUENCES_H */
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index febaf18..bd7827d 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -113,6 +113,8 @@ obj-$(CONFIG_TRACING) += tracepoint.o
obj-$(CONFIG_IOSF_MBI) += iosf_mbi.o
obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
+obj-$(CONFIG_RESTARTABLE_SEQUENCES) += restartable_sequences.o
+
###
# 64 bit specific files
ifeq ($(CONFIG_X86_64),y)
diff --git a/arch/x86/kernel/restartable_sequences.c b/arch/x86/kernel/restartable_sequences.c
new file mode 100644
index 0000000..3b38013
--- /dev/null
+++ b/arch/x86/kernel/restartable_sequences.c
@@ -0,0 +1,69 @@
+/*
+ * Restartable Sequences: x86 ABI.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ *
+ * Copyright (C) 2015, Google, Inc.,
+ * Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> and Andrew Hunter <ahh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
+ *
+ */
+
+#include <linux/sched.h>
+#include <linux/uaccess.h>
+#include <asm/restartable_sequences.h>
+
+void arch_rseq_check_critical_section(struct task_struct *p,
+ struct pt_regs *regs)
+{
+ if (!arch_rseq_in_crit_section(p, regs))
+ return;
+
+ /* RSEQ only applies to user-mode execution */
+ BUG_ON(!user_mode(regs));
+
+ /*
+ * The ABI is slightly different for {32,64}-bit threads on x86
+ *
+ * Short version:
+ * x86-64 (or x32): interrupted rip => %r10
+ * i386: interrupted rip => %ecx
+ *
+ * Longer version:
+ * The scratch registers available under the i386 function call ABI
+ * overlap with those used by argument registers under the x86_64 ABI.
+ *
+ * Given that the sequence block is already personality specific in
+ * that it must be entered by 'call' and that we always want the
+ * arguments available for a sequence restart; it's more natural to
+ * differentiate the ABI in these two cases.
+ */
+ if (unlikely(test_tsk_thread_flag(p, TIF_IA32)))
+ regs->cx = regs->ip; /* i386 */
+ else
+ regs->r10 = regs->ip; /* x86-64/x32 */
+
+ regs->ip = (unsigned long)p->group_leader->rseq_state.crit_restart;
+}
+
+void arch_rseq_handle_notify_resume(struct pt_regs *regs)
+{
+ struct restartable_sequence_state *rseq_state = ¤t->rseq_state;
+
+ /* If this update fails our user-state is incoherent. */
+ if (put_user(task_cpu(current), rseq_state->cpu_pointer))
+ force_sig(SIGSEGV, current);
+
+ arch_rseq_check_critical_section(current, regs);
+}
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 206996c..987c50b 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -31,6 +31,7 @@
#include <asm/vdso.h>
#include <asm/mce.h>
#include <asm/sighandling.h>
+#include <asm/restartable_sequences.h>
#ifdef CONFIG_X86_64
#include <asm/proto.h>
@@ -617,6 +618,15 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
sigset_t *set = sigmask_to_save();
compat_sigset_t *cset = (compat_sigset_t *) set;
+ /*
+ * If we are executing in the critical section of a restartable
+ * sequence we need to fix up the user's stack saved ip at this point
+ * so that signal handler return does not allow us to jump back into
+ * the block across a context switch boundary.
+ */
+ if (rseq_active(current))
+ arch_rseq_check_critical_section(current, regs);
+
/* Set up the stack frame */
if (is_ia32_frame()) {
if (ksig->ka.sa.sa_flags & SA_SIGINFO)
@@ -755,6 +765,8 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags)
if (thread_info_flags & _TIF_NOTIFY_RESUME) {
clear_thread_flag(TIF_NOTIFY_RESUME);
tracehook_notify_resume(regs);
+ if (rseq_active(current))
+ arch_rseq_handle_notify_resume(regs);
}
if (thread_info_flags & _TIF_USER_RETURN_NOTIFY)
fire_user_return_notifiers();
diff --git a/kernel/restartable_sequences.c b/kernel/restartable_sequences.c
index 72945f2..9102241 100644
--- a/kernel/restartable_sequences.c
+++ b/kernel/restartable_sequences.c
@@ -24,17 +24,22 @@
#ifdef CONFIG_RESTARTABLE_SEQUENCES
+#include <asm/restartable_sequences.h>
#include <linux/uaccess.h>
#include <linux/preempt.h>
#include <linux/syscalls.h>
static void rseq_sched_in_nop(struct preempt_notifier *pn, int cpu) {}
-static void rseq_sched_out_nop(struct preempt_notifier *pn,
- struct task_struct *next) {}
+static void rseq_sched_out(struct preempt_notifier *pn,
+ struct task_struct *next)
+{
+ if (arch_rseq_needs_notify_resume(current))
+ set_thread_flag(TIF_NOTIFY_RESUME);
+}
static __read_mostly struct preempt_ops rseq_preempt_ops = {
.sched_in = rseq_sched_in_nop,
- .sched_out = rseq_sched_out_nop,
+ .sched_out = rseq_sched_out,
};
int rseq_register_cpu_pointer_current(int __user *cpu_pointer)
^ permalink raw reply related
* [RFC PATCH 0/3] restartable sequences: fast user-space percpu critical sections
From: Paul Turner @ 2015-06-24 22:26 UTC (permalink / raw)
To: Peter Zijlstra, Paul E. McKenney, Mathieu Desnoyers
Cc: Andrew Hunter, Andi Kleen, Lai Jiangshan, linux-api, linux-kernel,
Steven Rostedt, Josh Triplett, Ingo Molnar, Andrew Morton,
Andy Lutomirski, Linus Torvalds, Chris Lameter
This is a fairly small series demonstrating a feature we've found to be quite
powerful in practice, "restartable sequences".
Most simply: these sequences comprise small snippets of user-code that are
guaranteed to be (effectively) executed serially, with support for restart (or
other handling) in the event of preemption or interruption.
This (when combined with an in-memory cpu-id; potentially optional on some
architectures) can be used to implement extremely fast per-cpu operations that
do not rely on the use of actual processor atomics. We've used this to back
performance critical code such as our malloc implementation with good results.
We previously discussed this feature at LPC:
http://www.linuxplumbersconf.org/2013/ocw/system/presentations/1695/original/LPC%20-%20PerCpu%20Atomics.pdf
Mathieu Desnoyers posted an alternate implementation based on the ideas above
at:
https://lkml.org/lkml/2015/5/21/472
This RFC is based on the approach we currently use internally. However, I'll
likely posit that neither this approach, nor the one posted above, is what we
should ultimately adopt (provided sufficient interest exists).
The implementation in this series can be summarized as follows:
- We allow a single (per-address) space critical section (and associated
handler) to be defined.
- When a thread with RSEQ configured (via new syscall) is interrupted within
a critical section, we modify its return address. Either within signal
delivery, or the notify-resume path. The scheduler touchpoint is only a
preempt_notifier which (potentially, dependent on config) examines the
kernel copy of pt_regs.
There are a few core requirements which motivate the approach taken here:
1) We must not interfere with regular scheduling. Unlike preemption protection
(or similar), there are no special considerations for code running within a
critical region beyond that we must move to the restart handler if
interrupted.
2) The code executing in scheduler context must be tightly constrained, both in
terms of overhead and that it must not require access to user memory.
3) The fast-path must be fast. That is, user entry/execution/exit of a
non-interrupted critical section is the most important case. The restart
handler is a 'slow' path that should only happen comparatively infrequently.
We're strongly motivated here by high-performance, low-level primitives:
think malloc, or rcu_read_lock.
While the contained implementation performs well under these constraints, it
has some notable limitations which we should consider for more general usage:
1) The definition of a single region works well for statically linked binaries
but can be challenging when shared-libraries want to use this feature. This
is partially mitigated in our experience that a library of primitives is
generally more useful than a myriad of custom sequences, but it's still a
major concern.
2) Due to the nature of restart and explicit location requirements it's only
really reasonable to write this critical section in assembly; which makes
porting and maintenance less convenient. (One of the included tests shows a
reasonable example of what this looks like.)
3) TLS spreads the entrails of its implementation all over compile _and_ link.
This makes properly handling it within the critical section cumbersome in
the shared binary case.
We've been thinking about how to address these issues and are considering some
alternate ABIs that still satisfy (1)-(3), but have a more convenient
programming model. I'll send this as a follow-up, but wanted to first share
the approach we've used to date.
Thanks,
- Paul
^ permalink raw reply
* [RFC PATCH 3/3] restartable sequences: basic user-space self-tests
From: Paul Turner @ 2015-06-24 22:26 UTC (permalink / raw)
To: Peter Zijlstra, Paul E. McKenney, Mathieu Desnoyers
Cc: Andrew Hunter, Andi Kleen, Lai Jiangshan,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steven Rostedt,
Josh Triplett, Ingo Molnar, Andrew Morton, Andy Lutomirski,
Linus Torvalds, Chris Lameter
In-Reply-To: <20150624222609.6116.86035.stgit-tdHu5vqousHHt/MElyovVYaSKrA+ACpX0E9HWUfgJXw@public.gmane.org>
Implements two basic tests of RSEQ functionality.
The first, "basic_test" only asserts that RSEQ works moderately correctly.
E.g. that:
- The CPUID pointer works
- Code infinitely looping within a critical section will eventually be
interrupted.
"basic_percpu_ops_test" is a slightly more "realistic" variant, implementing a
few simple per-cpu operations and testing their correctness. It also includes
a trivial example of user-space may multiplexing the critical section via the
restart handler.
Signed-off-by: Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
tools/testing/selftests/rseq/Makefile | 15 +
.../testing/selftests/rseq/basic_percpu_ops_test.S | 131 ++++++++++
.../testing/selftests/rseq/basic_percpu_ops_test.c | 250 ++++++++++++++++++++
tools/testing/selftests/rseq/basic_test.c | 76 ++++++
tools/testing/selftests/rseq/rseq.c | 48 ++++
tools/testing/selftests/rseq/rseq.h | 28 ++
6 files changed, 548 insertions(+)
create mode 100644 tools/testing/selftests/rseq/Makefile
create mode 100644 tools/testing/selftests/rseq/basic_percpu_ops_test.S
create mode 100644 tools/testing/selftests/rseq/basic_percpu_ops_test.c
create mode 100644 tools/testing/selftests/rseq/basic_test.c
create mode 100644 tools/testing/selftests/rseq/rseq.c
create mode 100644 tools/testing/selftests/rseq/rseq.h
diff --git a/tools/testing/selftests/rseq/Makefile b/tools/testing/selftests/rseq/Makefile
new file mode 100644
index 0000000..c5a2b47
--- /dev/null
+++ b/tools/testing/selftests/rseq/Makefile
@@ -0,0 +1,15 @@
+CFLAGS += -Wall
+LDFLAGS += -lpthread
+
+TESTS = basic_test basic_percpu_ops_test
+
+basic_percpu_ops_test: basic_percpu_ops_test.c basic_percpu_ops_test.S
+
+all: $(TESTS)
+%: %.c
+ $(CC) $(CFLAGS) -o $@ $^ rseq.c $(LDFLAGS)
+
+include ../lib.mk
+
+clean:
+ $(RM) $(TESTS)
diff --git a/tools/testing/selftests/rseq/basic_percpu_ops_test.S b/tools/testing/selftests/rseq/basic_percpu_ops_test.S
new file mode 100644
index 0000000..7da7781
--- /dev/null
+++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.S
@@ -0,0 +1,131 @@
+#include "rseq.h"
+
+#ifdef __x86_64__
+ .text
+ .code64
+
+#define FETCH_CPU(dest) movl %fs:__rseq_current_cpu@TPOFF, dest
+#define CRITICAL_SECTION_OFFSET(label) $label
+
+/* If start <= %RESTART_ADDR_REG < %end, jump to jump_to */
+#define HANDLE_REGION(start, end, jump_to) \
+ cmpq CRITICAL_SECTION_OFFSET(end), %RESTART_ADDR_REG; \
+ jge 1f; \
+ cmpq CRITICAL_SECTION_OFFSET(start), %RESTART_ADDR_REG; \
+ jge jump_to; \
+ 1:;
+
+#define HANDLE_REGION_PREFIX(prefix, start, end, jump_to) \
+ HANDLE_REGION(prefix##start, prefix##end, prefix##jump_to)
+
+/*-----------------------------------------------------------------------------
+ * Start of actual restartable sequences.
+ *---------------------------------------------------------------------------*/
+ .align 8
+ .globl RSEQ_CRITICAL_SECTION_START
+RSEQ_CRITICAL_SECTION_START:
+/* int rseq_percpu_lock() */
+ .globl rseq_percpu_lock
+ .type rseq_percpu_lock, @function
+rseq_percpu_lock:
+ .cfi_startproc
+rseq_percpu_lock_region0:
+ FETCH_CPU(%eax)
+ leaq (,%eax,8), %RESTART_ADDR_REG
+ leaq (%rdi,%RESTART_ADDR_REG,8), %RESTART_ADDR_REG
+rseq_percpu_lock_retry:
+ cmpw $0, (%RESTART_ADDR_REG)
+ jne rseq_percpu_lock_retry
+ movw $1, (%RESTART_ADDR_REG) /* 1 => lock owned */
+rseq_percpu_lock_region1:
+ ret
+rseq_percpu_lock_region2:
+ .cfi_endproc
+
+/*
+ * int rseq_cmpxchg(int cpu, intptr_t *p, intptr_t old, intptr_t new)
+ * int rseq_percpu_cmpxchgcheck(int cpu, intptr_t *p,
+ * intptr_t old, intptr_t new,
+ * intptr_t *check_ptr, intptr_t check_val)
+ *
+ * NOTE: We don't use cmpxchg in the implementation below as that would make
+ * checking the success of our commit operation was dependent on flags (which
+ * are in turn clobbered by the restart region) -- furthermore we can't just
+ * retry to fill in the flags since the restarted cmpxchg may have actually
+ * succeeded; spuriously failing subsequent attempts.
+ */
+
+ .globl rseq_percpu_cmpxchg
+ .type rseq_percpu_cmpxchg, @function
+rseq_percpu_cmpxchg:
+ .cfi_startproc
+rseq_percpu_cmpxchg_region0:
+ FETCH_CPU(%eax)
+ cmp %eax, %edi /* check cpu vs current_cpu */
+ jne rseq_percpu_cmpxchg_region1
+ cmp %rdx, (%rsi) /* verify *p == old */
+ jne rseq_percpu_cmpxchg_region2
+ mov %rcx, (%rsi)
+rseq_percpu_cmpxchg_region1:
+ ret /* return current cpu, indicating mismatch OR success */
+rseq_percpu_cmpxchg_region2:
+ mov $-1, %eax /* mismatch versus "old" or "check", return -1 */
+ ret
+rseq_percpu_cmpxchg_region3:
+ .cfi_endproc
+
+ .globl rseq_percpu_cmpxchgcheck
+ .type rseq_percpu_cmpxchgcheck, @function
+rseq_percpu_cmpxchgcheck:
+ .cfi_startproc
+rseq_percpu_cmpxchgcheck_region0:
+ FETCH_CPU(%eax)
+ cmp %eax, %edi /* check cpu vs current_cpu */
+ jne rseq_percpu_cmpxchgcheck_region1
+ cmp %rdx, (%rsi) /* verify *p == old */
+ jne rseq_percpu_cmpxchgcheck_region2
+ cmp %r9, (%r8) /* verify *check_ptr == check_val */
+ jne rseq_percpu_cmpxchgcheck_region2
+ mov %rcx, (%rsi)
+rseq_percpu_cmpxchgcheck_region1:
+ ret /* return current cpu, indicating mismatch OR success */
+rseq_percpu_cmpxchgcheck_region2:
+ mov $-1, %eax /* mismatch versus "old" or "check", return -1 */
+ ret
+rseq_percpu_cmpxchgcheck_region3:
+ .cfi_endproc
+
+ .align 8
+ .globl RSEQ_CRITICAL_SECTION_END
+RSEQ_CRITICAL_SECTION_END:
+
+/*-----------------------------------------------------------------------------
+ * Restart handler
+ * NOTE: per ABI, %RESTART_ADDR_REG is the program-counter we were restarted at.
+ *----------------------------------------------------------------------------
+ */
+
+ .align 8
+ .globl RSEQ_RESTART_HANDLER
+ .type RSEQ_RESTART_HANDLER, @function
+RSEQ_RESTART_HANDLER:
+ .cfi_startproc
+ /* There are several ways to implement this more efficiently. */
+ HANDLE_REGION_PREFIX(rseq_percpu_lock_region, 0, 1, 0)
+ HANDLE_REGION_PREFIX(rseq_percpu_lock_region, 1, 2, 1)
+
+ HANDLE_REGION_PREFIX(rseq_percpu_cmpxchg_region, 0, 1, 0)
+ HANDLE_REGION_PREFIX(rseq_percpu_cmpxchg_region, 1, 2, 1)
+ HANDLE_REGION_PREFIX(rseq_percpu_cmpxchg_region, 2, 3, 2)
+
+ HANDLE_REGION_PREFIX(rseq_percpu_cmpxchgcheck_region, 0, 1, 0)
+ HANDLE_REGION_PREFIX(rseq_percpu_cmpxchgcheck_region, 1, 2, 1)
+ HANDLE_REGION_PREFIX(rseq_percpu_cmpxchgcheck_region, 2, 3, 2)
+rseq_unknown_restart_addr:
+ mov %RESTART_ADDR_REG, %rdi
+ call rseq_unknown_restart_addr@PLT
+ .cfi_endproc
+
+/* Don't need/want an executable stack. */
+.section .note.GNU-stack,"",@progbits
+#endif
diff --git a/tools/testing/selftests/rseq/basic_percpu_ops_test.c b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
new file mode 100644
index 0000000..c6d7e4e
--- /dev/null
+++ b/tools/testing/selftests/rseq/basic_percpu_ops_test.c
@@ -0,0 +1,250 @@
+#define _GNU_SOURCE
+#include <assert.h>
+#include <pthread.h>
+#include <sched.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "rseq.h"
+
+/* We restrict on !__PIC__ as it greatly simplifies handling of TLS. */
+#if defined(__x86_64__) && !defined(__PIC__)
+
+#define barrier() __asm__ __volatile__("": : :"memory")
+
+/* Implemented by percpu_ops.S */
+struct percpu_lock {
+ int word[CPU_SETSIZE][16]; /* cache aligned; lock-word is [cpu][0] */
+};
+
+/* A simple percpu spinlock. Returns the cpu lock was acquired on. */
+int rseq_percpu_lock(struct percpu_lock *lock);
+
+/*
+ * cmpxchg [with an additional check value].
+ *
+ * Returns:
+ * -1 if *p != old [ || check_ptr != check_val, ] otherwise
+ * cpu that rseq_percpu_cmpxchgcheck was executed.
+ * - If this is different from the passed cpu, no modifications were made.
+ *
+ * Note: When specified, check_ptr is dereferenced iff *p == old
+ */
+int rseq_percpu_cmpxchg(int cpu, intptr_t *p, intptr_t old, intptr_t new);
+int rseq_percpu_cmpxchgcheck(int cpu, intptr_t *p, intptr_t old, intptr_t new,
+ intptr_t *check_ptr, intptr_t check_val);
+
+
+void rseq_percpu_unlock(struct percpu_lock *lock, int cpu)
+{
+ barrier(); /* need a release-store here, this suffices on x86. */
+ assert(lock->word[cpu][0] == 1);
+ lock->word[cpu][0] = 0;
+}
+
+void rseq_unknown_restart_addr(void *addr)
+{
+ fprintf(stderr, "rseq: unrecognized restart address %p\n", addr);
+ exit(1);
+}
+
+struct spinlock_test_data {
+ struct percpu_lock lock;
+ int counts[CPU_SETSIZE];
+ int reps;
+};
+
+void *test_percpu_spinlock_thread(void *arg)
+{
+ struct spinlock_test_data *data = arg;
+
+ int i, cpu;
+ rseq_configure_cpu_pointer();
+ for (i = 0; i < data->reps; i++) {
+ cpu = rseq_percpu_lock(&data->lock);
+ data->counts[cpu]++;
+ rseq_percpu_unlock(&data->lock, cpu);
+ }
+
+ return 0;
+}
+
+/*
+ * A simple test which implements a sharded counter using a per-cpu lock.
+ * Obviously real applications might prefer to simply use a per-cpu increment;
+ * however, this is reasonable for a test and the lock can be extended to
+ * synchronize more complicated operations.
+ */
+void test_percpu_spinlock()
+{
+ int i, sum;
+ pthread_t test_threads[200];
+ struct spinlock_test_data data;
+
+ memset(&data, 0, sizeof(data));
+ data.reps = 5000;
+
+ for (i = 0; i < 200; i++)
+ pthread_create(&test_threads[i], NULL,
+ test_percpu_spinlock_thread, &data);
+
+ for (i = 0; i < 200; i++)
+ pthread_join(test_threads[i], NULL);
+
+ sum = 0;
+ for (i = 0; i < CPU_SETSIZE; i++)
+ sum += data.counts[i];
+
+ assert(sum == data.reps * 200);
+}
+
+struct percpu_list_node {
+ intptr_t data;
+ struct percpu_list_node *next;
+};
+
+struct percpu_list {
+ struct percpu_list_node *heads[CPU_SETSIZE];
+};
+
+int percpu_list_push(struct percpu_list *list, struct percpu_list_node *node)
+{
+ int cpu;
+
+ do {
+ cpu = rseq_current_cpu();
+ node->next = list->heads[cpu];
+ } while (cpu != rseq_percpu_cmpxchg(cpu,
+ (intptr_t *)&list->heads[cpu], (intptr_t)node->next,
+ (intptr_t)node));
+
+ return cpu;
+}
+
+struct percpu_list_node *percpu_list_pop(struct percpu_list *list)
+{
+ int cpu;
+ struct percpu_list_node *head, *next;
+
+ do {
+ cpu = rseq_current_cpu();
+ head = list->heads[cpu];
+ /*
+ * Unlike a traditional lock-less linked list; the availability
+ * of a cmpxchg-check primitive allows us to implement pop
+ * without concerns over ABA-type races.
+ */
+ if (!head) return 0;
+ next = head->next;
+ } while (cpu != rseq_percpu_cmpxchgcheck(cpu,
+ (intptr_t *)&list->heads[cpu], (intptr_t)head, (intptr_t)next,
+ (intptr_t *)&head->next, (intptr_t)next));
+
+ return head;
+}
+
+
+void *test_percpu_list_thread(void *arg)
+{
+ int i;
+ struct percpu_list *list = (struct percpu_list *)arg;
+
+ rseq_configure_cpu_pointer();
+ for (i = 0; i < 100000; i++) {
+ struct percpu_list_node *node = percpu_list_pop(list);
+ sched_yield(); /* encourage shuffling */
+ if (node) percpu_list_push(list, node);
+ }
+
+ return 0;
+}
+
+/*
+ * Implements a per-cpu linked list then shuffles it via popping and pushing
+ * from many threads.
+ */
+void test_percpu_list()
+{
+ int i, j;
+ long sum = 0, expected_sum = 0;
+ struct percpu_list list;
+ pthread_t test_threads[200];
+ cpu_set_t allowed_cpus;
+
+ memset(&list, 0, sizeof(list));
+
+ /* Generate list entries for every usable cpu. */
+ sched_getaffinity(0, sizeof(allowed_cpus), &allowed_cpus);
+ for (i = 0; i < CPU_SETSIZE; i++) {
+ if (!CPU_ISSET(i, &allowed_cpus)) continue;
+ for (j = 1; j <= 100; j++) {
+ struct percpu_list_node *node;
+
+ expected_sum += j;
+
+ node = malloc(sizeof(*node));
+ assert(node);
+ node->data = j;
+ node->next = list.heads[i];
+ list.heads[i] = node;
+ }
+ }
+
+ for (i = 0; i < 200; i++)
+ assert(pthread_create(&test_threads[i], NULL,
+ test_percpu_list_thread, &list) == 0);
+
+ for (i = 0; i < 200; i++)
+ pthread_join(test_threads[i], NULL);
+
+ for (i = 0; i < CPU_SETSIZE; i++) {
+ cpu_set_t pin_mask;
+ struct percpu_list_node *node;
+
+ if (!CPU_ISSET(i, &allowed_cpus)) continue;
+
+ CPU_ZERO(&pin_mask);
+ CPU_SET(i, &pin_mask);
+ sched_setaffinity(0, sizeof(pin_mask), &pin_mask);
+
+ while ((node = percpu_list_pop(&list))) {
+ sum += node->data;
+ free(node);
+ }
+ }
+
+ /*
+ * All entries should now be accounted for (unless some external actor
+ * is interfering with our allowed affinity while this test is
+ * running).
+ */
+ assert(sum == expected_sum);
+}
+
+/* defined by basic_percpu_ops_test.S */
+extern void *RSEQ_CRITICAL_SECTION_START;
+extern void *RSEQ_CRITICAL_SECTION_END;
+extern void *RSEQ_RESTART_HANDLER;
+
+int main(int argc, char **argv)
+{
+ rseq_configure_region(&RSEQ_CRITICAL_SECTION_START,
+ &RSEQ_CRITICAL_SECTION_END,
+ &RSEQ_RESTART_HANDLER);
+ rseq_configure_cpu_pointer();
+
+ test_percpu_spinlock();
+ test_percpu_list();
+
+ return 0;
+}
+
+#else
+int main(int argc, char **argv)
+{
+ fprintf(stderr, "architecture not supported\n");
+ return 0;
+}
+#endif
diff --git a/tools/testing/selftests/rseq/basic_test.c b/tools/testing/selftests/rseq/basic_test.c
new file mode 100644
index 0000000..cca8edb
--- /dev/null
+++ b/tools/testing/selftests/rseq/basic_test.c
@@ -0,0 +1,76 @@
+/*
+ * Basic test coverage for critical regions and rseq_current_cpu().
+ */
+
+#define _GNU_SOURCE
+#include <assert.h>
+#include <sched.h>
+
+#include "rseq.h"
+
+#define _STRINGIFY(x) #x
+#define STRINGIFY(x) _STRINGIFY(x)
+
+extern void *RSEQ_CRITICAL_SECTION_START;
+extern void *RSEQ_CRITICAL_SECTION_END;
+extern void *RSEQ_RESTART_HANDLER;
+
+/*
+ * Asserts simply that we eventually see *some* event which interrupts our
+ * critical section (which otherwise loops infinitely). This could be
+ * preemption or signal delivery.
+ */
+int test_critical_section()
+{
+ void* restart_address = 0;
+#if defined(__i386__) || defined(__x86_64__)
+ __asm__(
+ ".globl RSEQ_CRITICAL_SECTION_START\n"
+ "RSEQ_CRITICAL_SECTION_START:\n"
+ " jmp RSEQ_CRITICAL_SECTION_START\n" /* while(1) */
+ ".globl RSEQ_CRITICAL_SECTION_END\n"
+ "RSEQ_CRITICAL_SECTION_END:\n"
+ ".globl RSEQ_RESTART_HANDLER\n"
+ "RSEQ_RESTART_HANDLER:\n"
+ " movq %%" STRINGIFY(RESTART_ADDR_REG) ", %0\n"
+ : "=a"(restart_address) ::);
+ assert(restart_address == &RSEQ_CRITICAL_SECTION_START);
+#else
+ fprintf(stderr, "architecture not supported\n");
+#endif
+ return 0;
+}
+
+void test_cpu_pointer()
+{
+ cpu_set_t affinity, test_affinity;
+ int i;
+
+ sched_getaffinity(0, sizeof(affinity), &affinity);
+ CPU_ZERO(&test_affinity);
+ for (i = 0; i < CPU_SETSIZE; i++) {
+ if (CPU_ISSET(i, &affinity)) {
+ CPU_SET(i, &test_affinity);
+ sched_setaffinity(0, sizeof(test_affinity),
+ &test_affinity);
+ assert(rseq_current_cpu() == sched_getcpu());
+ assert(rseq_current_cpu() == i);
+ CPU_CLR(i, &test_affinity);
+ }
+ }
+ sched_setaffinity(0, sizeof(affinity), &affinity);
+}
+
+int main(int argc, char **argv)
+{
+ rseq_configure_region(&RSEQ_CRITICAL_SECTION_START,
+ &RSEQ_CRITICAL_SECTION_END,
+ &RSEQ_RESTART_HANDLER);
+ rseq_configure_cpu_pointer();
+
+ test_critical_section();
+ test_cpu_pointer();
+
+ return 0;
+}
+
diff --git a/tools/testing/selftests/rseq/rseq.c b/tools/testing/selftests/rseq/rseq.c
new file mode 100644
index 0000000..c1ea5d8
--- /dev/null
+++ b/tools/testing/selftests/rseq/rseq.c
@@ -0,0 +1,48 @@
+#define _GNU_SOURCE
+#include <assert.h>
+#include <errno.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "rseq.h"
+
+__thread volatile const int __rseq_current_cpu = -1;
+
+#define __NR_rseq 323
+#define SYS_RSEQ_SET_CRITICAL 0
+#define SYS_RSEQ_SET_CPU_POINTER 1
+
+int sys_rseq(int op, int flags, void* val1, void* val2, void* val3)
+{
+ return syscall(__NR_rseq, op, flags,
+ (intptr_t)val1, (intptr_t)val2, (intptr_t)val3);
+}
+
+static void sys_rseq_checked(int op, int flags,
+ void* val1, void* val2, void* val3)
+{
+ int rc = sys_rseq(op, flags, val1, val2, val3);
+ if (rc) {
+ fprintf(stderr,"sys_rseq(%d, %d, %p, %p, %p) failed(%d): %s\n",
+ op, flags, val1, val2, val3, errno, strerror(errno));
+ exit(1);
+ }
+}
+
+void rseq_configure_region(void *rseq_text_start, void *rseq_text_end,
+ void *rseq_restart_handler)
+{
+ sys_rseq_checked(SYS_RSEQ_SET_CRITICAL, 0,
+ rseq_text_start, rseq_text_end, rseq_restart_handler);
+}
+
+void rseq_configure_cpu_pointer(void)
+{
+ sys_rseq_checked(SYS_RSEQ_SET_CPU_POINTER, 0,
+ (void*)&__rseq_current_cpu, 0, 0);
+ assert(rseq_current_cpu() != -1); /* always updated prior to return. */
+}
+
diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h
new file mode 100644
index 0000000..91bb655
--- /dev/null
+++ b/tools/testing/selftests/rseq/rseq.h
@@ -0,0 +1,28 @@
+#ifndef RSEQ_TEST_H
+#define RSEQ_TEST_H
+
+#if defined(__i386__)
+#define RESTART_ADDR_REG ecx
+#elif defined(__x86_64__)
+#define RESTART_ADDR_REG r10
+#else
+#define RESTART_ADDR_REG unknown
+#endif
+
+#ifndef __ASSEMBLER__
+int sys_rseq(int op, int flags, void* val1, void* val2, void* val3);
+/* RSEQ provided thread-local current_cpu */
+
+void rseq_configure_cpu_pointer(void);
+
+void rseq_configure_region(void *rseq_text_start, void *rseq_text_end,
+ void *rseq_restart_handler);
+
+
+extern __thread volatile const int __rseq_current_cpu;
+static inline int rseq_current_cpu(void) { return __rseq_current_cpu; }
+
+void run_tests();
+#endif
+
+#endif
^ permalink raw reply related
* Re: [PATCH] st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO
From: Sergey Senozhatsky @ 2015-06-24 23:27 UTC (permalink / raw)
To: Greg KH <gregkh@linuxfoundation.org> (gregkh@linuxfoundation.org)
Cc: Sergey Senozhatsky, Seymour, Shane M, linux-scsi@vger.kernel.org,
linux-api@vger.kernel.org, Kai.Makisara@kolumbus.fi
In-Reply-To: <20150624151011.GC7925@kroah.com>
On (06/24/15 08:10), Greg KH <gregkh@linuxfoundation.org> (gregkh@linuxfoundation.org) wrote:
> On Wed, Jun 24, 2015 at 03:25:57PM +0900, Sergey Senozhatsky wrote:
> > On (06/24/15 06:10), Seymour, Shane M wrote:
> > [..]
> > >
> > > /* The sysfs driver interface. Read-only at the moment */
> > > -static ssize_t st_try_direct_io_show(struct device_driver *ddp, char *buf)
> > > +static ssize_t try_direct_io_show(struct device_driver *ddp, char *buf)
> > > {
> > > - return snprintf(buf, PAGE_SIZE, "%d\n", try_direct_io);
> > > + return sprintf(buf, "%d\n", try_direct_io);
> > > }
> >
> > a nitpick,
> >
> > per Documentation/filesystems/sysfs.txt
> >
> > :
> > : - show() should always use scnprintf().
> > :
>
> That should be rewritten to say, "don't use snprintf(), but scnprintf(),
> if you want to. Otherwise sprintf() should be fine as you obviously are
> only returning a single value to userspace"
>
Sure, that was just a nitpick. For '%d' it's totally fine, I agree.
It was more of a 'do we strictly obey the rules' thing.
-ss
^ permalink raw reply
* Re: [RFC PATCH 0/3] restartable sequences: fast user-space percpu critical sections
From: Andy Lutomirski @ 2015-06-25 0:07 UTC (permalink / raw)
To: Paul Turner
Cc: Peter Zijlstra, Paul E. McKenney, Mathieu Desnoyers,
Andrew Hunter, Andi Kleen, Lai Jiangshan, Linux API,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Steven Rostedt, Josh Triplett, Ingo Molnar, Andrew Morton,
Linus Torvalds, Chris Lameter
In-Reply-To: <20150624222609.6116.86035.stgit-tdHu5vqousHHt/MElyovVYaSKrA+ACpX0E9HWUfgJXw@public.gmane.org>
On Wed, Jun 24, 2015 at 3:26 PM, Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> This is a fairly small series demonstrating a feature we've found to be quite
> powerful in practice, "restartable sequences".
>
On an extremely short glance, I'm starting to think that the right
approach, at least for x86, is to implement per-cpu gsbase. Then you
could do cmpxchg with a gs prefix to atomically take a percpu lock and
atomically release a percpu lock and check whether someone else stole
the lock from you. (Note: cmpxchg, unlike lock cmpxchg, is very
fast.)
This is totally useless for other architectures, but I think it would
be reasonable clean on x86. Thoughts?
I can elaborate if necessary.
--Andy
^ permalink raw reply
* [PATCH] sysfs.txt: update show method notes about sprintf/snprintf/scnprintf usage
From: Seymour, Shane M @ 2015-06-25 0:55 UTC (permalink / raw)
To: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> (gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org)
Changed the documentation to allow sprintf() for small
single values and explicitly say snprintf() must never be used in
a show function to format data to be returned to user space.
Change based on a discussion about the patch
st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO
Suggested-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Signed-off-by: Shane Seymour <shane.seymour-VXdhtT5mjnY@public.gmane.org>
---
--- a/Documentation/filesystems/sysfs.txt 2015-06-22 14:18:40.278620871 -0500
+++ b/Documentation/filesystems/sysfs.txt 2015-06-24 13:42:21.344446532 -0500
@@ -212,7 +212,9 @@ Other notes:
- show() methods should return the number of bytes printed into the
buffer. This is the return value of scnprintf().
-- show() should always use scnprintf().
+- show() must not use snprintf() when formatting a value to be
+ returned to user space. For small single values you can use
+ sprintf() otherwise you must use scnprintf().
- store() should return the number of bytes used from the buffer. If the
entire buffer has been used, just return the count argument.
^ permalink raw reply
* Re: [PATCH v7 00/16] libnvdimm: non-volatile memory devices
From: Toshi Kani @ 2015-06-25 0:59 UTC (permalink / raw)
To: Dan Williams
Cc: linux-nvdimm, rafael, neilb, Lv Zheng, H. Peter Anvin, hch, sfr,
Rafael J. Wysocki, Robert Moore, mingo, linux-acpi, Jens Axboe,
Borislav Petkov, Thomas Gleixner, axboe, gregkh, linux-kernel,
Andy Lutomirski, linux-api, akpm, Linus Torvalds
In-Reply-To: <20150617215027.11597.61414.stgit@dwillia2-desk3.amr.corp.intel.com>
On Wed, 2015-06-17 at 19:13 -0400, Dan Williams wrote:
> A new sub-system in support of non-volatile memory storage devices.
>
> Stephen, please add libnvdimm-for-next to -next:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/djbw/nvdimm libnvdimm-for-next
>
> Changes since v6 [1]:
>
> 1/ Deferred the patches dependent on ->rw_bytes() (BTT - stacked block
> driver, BLK - mmio aperture windows driver, NFIT_TEST - unit test
> infrastructure for all libnvdimm + nfit components) to their own
> patchset. Make the ->rw_bytes() implementation the first patch in
> that series (Christoph)
>
> 2/ Collected acks from Christoph and Rafael!
>
> 3/ Add a HAS_IOMEM dependency to CONFIG_BLK_DEV_PMEM following commit
> b6f2098fb708 "block: pmem: Add dependency on HAS_IOMEM" in 4.1-rc8.
>
> 4/ Move libnvdimm to subsys_initcall() and move arch/x86/kernel/pmem.c
> back to device_initcall(). This allows ACPI_NFIT to be built-in.
> (Linda)
>
> 5/ Drop the ACPI_DRIVER_ALL_NOTIFY_EVENTS flag in the nfit driver.
> (Rafael)
>
> 6/ Reference count the nvdimm_drvdata object. This fixes a bug that was
> found when the unit tests were extended to test disabling an nvdimm
> while a region device still had references to label data.
>
:
> Dan Williams (16):
> e820, efi: add ACPI 6.0 persistent memory types
> libnvdimm, nfit: initial libnvdimm infrastructure and NFIT support
> libnvdimm: control character device and nvdimm_bus sysfs attributes
> libnvdimm, nfit: dimm/memory-devices
> libnvdimm: control (ioctl) messages for nvdimm_bus and nvdimm devices
> libnvdimm, nvdimm: dimm driver and base libnvdimm device-driver infrastructure
> libnvdimm, nfit: regions (block-data-window, persistent memory, volatile memory)
> libnvdimm: support for legacy (non-aliasing) nvdimms
> libnvdimm, pmem: move pmem to drivers/nvdimm/
> libnvdimm, pmem: add libnvdimm support to the pmem driver
> libnvdimm, nfit: add interleave-set state-tracking infrastructure
> libnvdimm: namespace indices: read and validate
> libnvdimm: pmem label sets and namespace instantiation.
> libnvdimm: blk labels and namespace instantiation
> libnvdimm: write pmem label set
> libnvdimm: write blk label set
We have been successfully running this patchset on our NFIT-enabled
prototype systems with pmem. (Intel example _DSM, label, blk are not
available for testing.)
So for patch 1/16 to 4/15, and 6/16 to 10/16.
Tested-by: Toshi Kani <toshi.kani@hp.com>
Thanks,
-Toshi
^ permalink raw reply
* Re: [PATCH] sysfs.txt: update show method notes about sprintf/snprintf/scnprintf usage
From: Sergey Senozhatsky @ 2015-06-25 1:59 UTC (permalink / raw)
To: Seymour, Shane M
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> (gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org)
In-Reply-To: <DDB9C85B850785449757F9914A034FCB3F8EA364-MCKW7lC+H9ISZAcGdq5asR6epYMZPwEe5NbjCUgZEJk@public.gmane.org>
On (06/25/15 00:55), Seymour, Shane M wrote:
> Changed the documentation to allow sprintf() for small
> single values and explicitly say snprintf() must never be used in
> a show function to format data to be returned to user space.
>
> Change based on a discussion about the patch
> st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO
>
> Suggested-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Signed-off-by: Shane Seymour <shane.seymour-VXdhtT5mjnY@public.gmane.org>
> ---
> --- a/Documentation/filesystems/sysfs.txt 2015-06-22 14:18:40.278620871 -0500
> +++ b/Documentation/filesystems/sysfs.txt 2015-06-24 13:42:21.344446532 -0500
> @@ -212,7 +212,9 @@ Other notes:
> - show() methods should return the number of bytes printed into the
> buffer. This is the return value of scnprintf().
>
> -- show() should always use scnprintf().
> +- show() must not use snprintf() when formatting a value to be
> + returned to user space. For small single values you can use
> + sprintf() otherwise you must use scnprintf().
Well, a single value can easily overflow
sprintf(buf, "%s", dev->large_value);
Probably the wording better be "if you guarantee that overflow will
never happen, then you can use ...".
-ss
^ permalink raw reply
* [PATCH v2] sysfs.txt: update show method notes about sprintf/snprintf/scnprintf usage
From: Seymour, Shane M @ 2015-06-25 2:33 UTC (permalink / raw)
To: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> (gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org)
Cc: Sergey Senozhatsky
Changed the documentation to allow sprintf() when the buffer
provided by sysfs cannot be overflowed. Explicitly say
snprintf() must never be used in a show function to format
data to be returned to user space.
Change based on a discussion about the patch
st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO
Suggested-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Signed-off-by: Shane Seymour <shane.seymour-VXdhtT5mjnY@public.gmane.org>
---
Changes from v1:
- Changed the text after feedback from Sergey Senozhatsky
need to be more explicit that sprintf() can only be used when
you know that you will never overflow the buffer provided
by sysfs.
--- a/Documentation/filesystems/sysfs.txt 2015-06-22 14:18:40.278620871 -0500
+++ b/Documentation/filesystems/sysfs.txt 2015-06-24 15:42:32.617955400 -0500
@@ -212,7 +212,10 @@ Other notes:
- show() methods should return the number of bytes printed into the
buffer. This is the return value of scnprintf().
-- show() should always use scnprintf().
+- show() must not use snprintf() when formatting the value to be
+ returned to user space. If you can guarantee that an overflow
+ will never happen you can use sprintf() otherwise you must use
+ scnprintf().
- store() should return the number of bytes used from the buffer. If the
entire buffer has been used, just return the count argument.
^ permalink raw reply
* Re: [RFC PATCH 0/3] restartable sequences: fast user-space percpu critical sections
From: Paul Turner @ 2015-06-25 2:54 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Peter Zijlstra, Paul E. McKenney, Mathieu Desnoyers,
Andrew Hunter, Andi Kleen, Lai Jiangshan, Linux API,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Steven Rostedt, Josh Triplett, Ingo Molnar, Andrew Morton,
Linus Torvalds, Chris Lameter
In-Reply-To: <CALCETrXAtYDZBbpwZceFyhLOnqFmTDqTxhGfbrrVrY+34cxSFg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jun 24, 2015 at 5:07 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Wed, Jun 24, 2015 at 3:26 PM, Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>> This is a fairly small series demonstrating a feature we've found to be quite
>> powerful in practice, "restartable sequences".
>>
>
> On an extremely short glance, I'm starting to think that the right
> approach, at least for x86, is to implement per-cpu gsbase. Then you
> could do cmpxchg with a gs prefix to atomically take a percpu lock and
> atomically release a percpu lock and check whether someone else stole
> the lock from you. (Note: cmpxchg, unlike lock cmpxchg, is very
> fast.)
>
> This is totally useless for other architectures, but I think it would
> be reasonable clean on x86. Thoughts?
So this gives semantics that are obviously similar to this_cpu().
This provides allows reasonable per-cpu counters (which is alone
almost sufficient for a strong user-space RCU implementation giving
this some legs).
However, unless there's a nice implementation trick I'm missing, the
thing that stands out to me for locks (or other primitives) is that
this forces a two-phase commit. There's no way (short of say,
cmpxchg16b) to perform a write conditional on the lock not having been
stolen from us (and subsequently release the lock).
e.g.
1) We take the operation in some sort of speculative mode, that
another thread on the same cpu is stilled allowed to steal from us
2) We prepare what we want to commit
3) At this point we have to promote the lock taken in (1) to perform
our actual commit, or see that someone else has stolen (1)
4) Release the promoted lock in (3)
However, this means that if we're preempted at (3) then no other
thread on that cpu can make progress until we've been rescheduled and
released the lock; a nice property of the model we have today is that
threads sharing a cpu can not impede each other beyond what the
scheduler allows.
A lesser concern, but worth mentioning, is that there are also
potential pitfalls in the interaction with signal handlers,
particularly if a 2-phase commit is used.
- Paul
^ permalink raw reply
* Re: [PATCH v2] ipc: Modify message queue accounting to reflect both total user data and auxiliary kernel data
From: Davidlohr Bueso @ 2015-06-25 5:47 UTC (permalink / raw)
To: Marcus Gelderie
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
dhowells-H+wXaHxf7aLQT0dZR+AlfA,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
dledford-H+wXaHxf7aLQT0dZR+AlfA, John Duffy, Arto Bendiken,
linux-api-u79uwXL29TY76Z2rM5mHXA, redmnic-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <20150622222546.GA32432-W7fNxlbxG8VSq9BJjBFyUp/QNRX+jHPU@public.gmane.org>
On Tue, 2015-06-23 at 00:25 +0200, Marcus Gelderie wrote:
> A while back, the message queue implementation in the kernel was
> improved to use btrees to speed up retrieval of messages (commit
> d6629859b36). The patch introducing the improved kernel handling of
> message queues (using btrees) has, as a by-product, changed the
> meaning of the QSIZE field in the pseudo-file created for the queue.
> Before, this field reflected the size of the user-data in the queue.
> Since, it also takes kernel data structures into account. For
> example, if 13 bytes of user data are in the queue, on my machine the
> file reports a size of 61 bytes.
Good catch, and a nice opportunity to make the mq manpage more specific
wrt to queue sizes.
[...]
> Reporting the size of the message queue in kernel has its merits, but
> doing so in the QSIZE field of the pseudo file corresponding to the
> queue is a breaking change, as mentioned above. This patch therefore
> returns the QSIZE field to its original meaning. At the same time,
> it introduces a new field QKERSIZE that reflects the size of the queue
> in kernel (user data + kernel data).
Hmmm I'm not sure about this. What are the specific benefits of having
QKERSIZE? We don't export in-kernel data like this in any other ipc
(posix or sysv) mechanism, afaik. Plus, we do not compromise kernel data
structures like this, as we would break userspace if later we change
posix_msg_tree_node. So NAK to this.
I would just remove the extra
+ info->qsize += sizeof(struct posix_msg_tree_node);
bits from d6629859b36 (along with -stable v3.5), plus a patch updating
the manpage that this field only reflects user data.
Thanks,
Davidlohr
^ permalink raw reply
* Re: [PATCH] sysfs.txt: update show method notes about sprintf/snprintf/scnprintf usage
From: Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> (gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org) @ 2015-06-25 6:03 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Seymour, Shane M,
linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20150625015957.GB3365@swordfish>
On Thu, Jun 25, 2015 at 10:59:57AM +0900, Sergey Senozhatsky wrote:
> On (06/25/15 00:55), Seymour, Shane M wrote:
> > Changed the documentation to allow sprintf() for small
> > single values and explicitly say snprintf() must never be used in
> > a show function to format data to be returned to user space.
> >
> > Change based on a discussion about the patch
> > st: convert DRIVER_ATTR macros to DRIVER_ATTR_RO
> >
> > Suggested-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> > Signed-off-by: Shane Seymour <shane.seymour-VXdhtT5mjnY@public.gmane.org>
> > ---
> > --- a/Documentation/filesystems/sysfs.txt 2015-06-22 14:18:40.278620871 -0500
> > +++ b/Documentation/filesystems/sysfs.txt 2015-06-24 13:42:21.344446532 -0500
> > @@ -212,7 +212,9 @@ Other notes:
> > - show() methods should return the number of bytes printed into the
> > buffer. This is the return value of scnprintf().
> >
> > -- show() should always use scnprintf().
> > +- show() must not use snprintf() when formatting a value to be
> > + returned to user space. For small single values you can use
> > + sprintf() otherwise you must use scnprintf().
>
> Well, a single value can easily overflow
>
> sprintf(buf, "%s", dev->large_value);
That's an obviously foolish sysfs attribute, if you do that, you deserve
the kernel crash :)
> Probably the wording better be "if you guarantee that overflow will
> never happen, then you can use ...".
For a document that no one has obviously read in the past 5 years, I
really doubt we need to work too hard on the exact specific wording of
it.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v6 10/12] KVM: arm64: guest debug, HW assisted debug support
From: Alex Bennée @ 2015-06-25 6:38 UTC (permalink / raw)
To: Christoffer Dall
Cc: kvm, open list:DOCUMENTATION, Peter Zijlstra, jan.kiszka,
Will Deacon, Ingo Molnar, Lorenzo Pieralisi, Russell King,
Jonathan Corbet, kvmarm, Gleb Natapov, zhichao.huang,
Catalin Marinas, bp, marc.zyngier, r65777, linux-arm-kernel,
open list:ABI/API, open list, dahi, pbonzini
In-Reply-To: <20150624202216.GE22785@cbox>
Christoffer Dall <christoffer.dall@linaro.org> writes:
> On Fri, Jun 19, 2015 at 01:23:48PM +0100, Alex Bennée wrote:
>> This adds support for userspace to control the HW debug registers for
>> guest debug. In the debug ioctl we copy the IMPDEF defined number of
>
> s/defined//
>
>> registers into a new register set called host_debug_state. There is now
>> a new vcpu parameter called debug_ptr which selects which register set
>> is to copied into the real registers when world switch occurs.
>
> But this patch doesn't seem to add the debug_ptr field?
Oops, yes the comment belongs to the previous patch.
>
> s/to//
>
>>
>> I've moved some helper functions into the hw_breakpoint.h header for
>> re-use.
>>
>> As with single step we need to tweak the guest registers to enable the
>> exceptions so we need to save and restore those bits.
>>
>> Two new capabilities have been added to the KVM_EXTENSION ioctl to allow
>> userspace to query the number of hardware break and watch points
>> available on the host hardware.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>> - switched to C setup
>> - replace host debug registers directly into context
>> - minor tweak to api docs
>> - setup right register for debug
>> - add FAR_EL2 to debug exit structure
>> - add support for trapping debug register access
>> v3
>> - remove stray trace statement
>> - fix spacing around operators (various)
>> - clean-up usage of trap_debug
>> - introduce debug_ptr, replace excessive memcpy stuff
>> - don't use memcpy in ioctl, just assign
>> - update cap ioctl documentation
>> - reword a number comments
>> - rename host_debug_state->external_debug_state
>> v4
>> - use the new u32/u64 split debug_ptr approach
>> - fix some wording/comments
>> v5
>> - don't set MDSCR_EL1.KDE (not needed)
>> v6
>> - update wording given change in commentary
>> - KVM_GUESTDBG_USE_HW_BP->KVM_GUESTDBG_USE_HW
>> ---
>> Documentation/virtual/kvm/api.txt | 7 ++++++-
>> arch/arm/kvm/arm.c | 7 +++++++
>> arch/arm64/include/asm/hw_breakpoint.h | 12 +++++++++++
>> arch/arm64/include/asm/kvm_host.h | 6 +++++-
>> arch/arm64/kernel/hw_breakpoint.c | 12 -----------
>> arch/arm64/kvm/debug.c | 37 +++++++++++++++++++++++++++++-----
>> arch/arm64/kvm/handle_exit.c | 6 ++++++
>> arch/arm64/kvm/reset.c | 12 +++++++++++
>> include/uapi/linux/kvm.h | 2 ++
>> 9 files changed, 82 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index 33c8143..ada57df 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -2668,7 +2668,7 @@ The top 16 bits of the control field are architecture specific control
>> flags which can include the following:
>>
>> - KVM_GUESTDBG_USE_SW_BP: using software breakpoints [x86, arm64]
>> - - KVM_GUESTDBG_USE_HW_BP: using hardware breakpoints [x86, s390]
>> + - KVM_GUESTDBG_USE_HW_BP: using hardware breakpoints [x86, s390, arm64]
>> - KVM_GUESTDBG_INJECT_DB: inject DB type exception [x86]
>> - KVM_GUESTDBG_INJECT_BP: inject BP type exception [x86]
>> - KVM_GUESTDBG_EXIT_PENDING: trigger an immediate guest exit [s390]
>> @@ -2683,6 +2683,11 @@ updated to the correct (supplied) values.
>> The second part of the structure is architecture specific and
>> typically contains a set of debug registers.
>>
>> +For arm64 the number of debug registers is implementation defined and
>> +can be determined by querying the KVM_CAP_GUEST_DEBUG_HW_BPS and
>> +KVM_CAP_GUEST_DEBUG_HW_WPS capabilities which return a positive number
>> +indicating the number of supported registers.
>> +
>> When debug events exit the main run loop with the reason
>> KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run
>> structure containing architecture specific debug information.
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 0d17c7b..60c4045 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -307,6 +307,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>
>> #define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE | \
>> KVM_GUESTDBG_USE_SW_BP | \
>> + KVM_GUESTDBG_USE_HW | \
>> KVM_GUESTDBG_SINGLESTEP)
>>
>> /**
>> @@ -327,6 +328,12 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>
>> if (dbg->control & KVM_GUESTDBG_ENABLE) {
>> vcpu->guest_debug = dbg->control;
>> +
>> + /* Hardware assisted Break and Watch points */
>> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
>> + vcpu->arch.external_debug_state = dbg->arch;
>> + }
>> +
>> } else {
>> /* If not enabled clear all flags */
>> vcpu->guest_debug = 0;
>> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
>> index 52b484b..c450552 100644
>> --- a/arch/arm64/include/asm/hw_breakpoint.h
>> +++ b/arch/arm64/include/asm/hw_breakpoint.h
>> @@ -130,6 +130,18 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
>> }
>> #endif
>>
>> +/* Determine number of BRP registers available. */
>> +static inline int get_num_brps(void)
>> +{
>> + return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
>> +}
>> +
>> +/* Determine number of WRP registers available. */
>> +static inline int get_num_wrps(void)
>> +{
>> + return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
>> +}
>> +
>> extern struct pmu perf_ops_bp;
>>
>> #endif /* __KERNEL__ */
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 9697daf..0a3ee7b 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -116,13 +116,17 @@ struct kvm_vcpu_arch {
>> * debugging the guest from the host and to maintain separate host and
>> * guest state during world switches. vcpu_debug_state are the debug
>> * registers of the vcpu as the guest sees them. host_debug_state are
>> - * the host registers which are saved and restored during world switches.
>> + * the host registers which are saved and restored during
>> + * world switches. external_debug_state contains the debug
>> + * values we want to debugging the guest. This is set via the
>
> nit: s/debugging/debug/
>
>> + * KVM_SET_GUEST_DEBUG ioctl.
>> *
>> * debug_ptr points to the set of debug registers that should be loaded
>> * onto the hardware when running the guest.
>> */
>> struct kvm_guest_debug_arch *debug_ptr;
>> struct kvm_guest_debug_arch vcpu_debug_state;
>> + struct kvm_guest_debug_arch external_debug_state;
>>
>> /* Pointer to host CPU context */
>> kvm_cpu_context_t *host_cpu_context;
>> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
>> index e7d934d..3a41bbf 100644
>> --- a/arch/arm64/kernel/hw_breakpoint.c
>> +++ b/arch/arm64/kernel/hw_breakpoint.c
>> @@ -49,18 +49,6 @@ static DEFINE_PER_CPU(int, stepping_kernel_bp);
>> static int core_num_brps;
>> static int core_num_wrps;
>>
>> -/* Determine number of BRP registers available. */
>> -static int get_num_brps(void)
>> -{
>> - return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
>> -}
>> -
>> -/* Determine number of WRP registers available. */
>> -static int get_num_wrps(void)
>> -{
>> - return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
>> -}
>> -
>> int hw_breakpoint_slots(int type)
>> {
>> /*
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index d439eb8..b287bbc 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -96,10 +96,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>> MDCR_EL2_TDRA |
>> MDCR_EL2_TDOSA);
>>
>> - /* Trap on access to debug registers? */
>> - if (trap_debug)
>> - vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>> -
>> /* Is Guest debugging in effect? */
>> if (vcpu->guest_debug) {
>> /* Route all software debug exceptions to EL2 */
>> @@ -134,11 +130,42 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>> } else {
>> vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
>> }
>> +
>> + /*
>> + * HW Breakpoints and watchpoints
>> + *
>> + * We simply switch the debug_ptr to point to our new
>> + * external_debug_state which has been populated by the
>> + * debug ioctl. The existing KVM_ARM64_DEBUG_DIRTY
>> + * mechanism ensures the registers are updated on the
>> + * world switch.
>> + */
>> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
>> + /* Enable breakpoints/watchpoints */
>> + vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_MDE;
>> +
>> + vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
>> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
>> + trap_debug = true;
>> + }
>> }
>> +
>> + /* Trap debug register access */
>> + if (trap_debug)
>> + vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>> }
>>
>> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>> {
>> - if (vcpu->guest_debug)
>> + if (vcpu->guest_debug) {
>> restore_guest_debug_regs(vcpu);
>> +
>> + /*
>> + * If we were using HW debug we need to restore the
>> + * debug_ptr to the guest debug state.
>> + */
>> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW)
>> + vcpu->arch.debug_ptr = &vcpu->arch.vcpu_debug_state;
>
> I still think this would be more cleanly done in the setup_debug
> function, but ok:
I don't follow, setup_debug is called before we enter KVM. It's pretty
light when no debugging is being done so this ensure we leave state how
we would like it when we stop debugging.
I can move it to an else leg in setup if you really want.
>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
--
Alex Bennée
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply
* Re: [PATCH] sysfs.txt: update show method notes about sprintf/snprintf/scnprintf usage
From: Sergey Senozhatsky @ 2015-06-25 7:05 UTC (permalink / raw)
To: Greg KH <gregkh@linuxfoundation.org> (gregkh@linuxfoundation.org)
Cc: Sergey Senozhatsky, Seymour, Shane M, linux-api@vger.kernel.org,
Jonathan Corbet, linux-doc
In-Reply-To: <20150625060305.GA11905@kroah.com>
Cc Jonathan and linux-doc
On (06/24/15 23:03), Greg KH <gregkh@linuxfoundation.org> (gregkh@linuxfoundation.org) wrote:
[..]
> > > -- show() should always use scnprintf().
> > > +- show() must not use snprintf() when formatting a value to be
> > > + returned to user space. For small single values you can use
> > > + sprintf() otherwise you must use scnprintf().
> >
> > Well, a single value can easily overflow
> >
> > sprintf(buf, "%s", dev->large_value);
>
> That's an obviously foolish sysfs attribute, if you do that, you deserve
> the kernel crash :)
:)
And the 'always use scnprintf()' rule keeps all of us on the safe
side (almost for free).
> > Probably the wording better be "if you guarantee that overflow will
> > never happen, then you can use ...".
>
> For a document that no one has obviously read in the past 5 years, I
> really doubt we need to work too hard on the exact specific wording of
> it.
Oh, it's especially pleasant and satisfactory to ignore
a well-written and scrupulous documentation :) just kidding.
-ss
^ permalink raw reply
* Re: [PATCH v2] ipc: Modify message queue accounting to reflect both total user data and auxiliary kernel data
From: Michael Kerrisk (man-pages) @ 2015-06-25 7:23 UTC (permalink / raw)
To: Davidlohr Bueso, Doug Ledford
Cc: Marcus Gelderie, lkml, David Howells, Alexander Viro, John Duffy,
Arto Bendiken, Linux API, Marcus Gelderie
In-Reply-To: <1435211229.11852.23.camel-h16yJtLeMjHk1uMJSBkQmQ@public.gmane.org>
On 25 June 2015 at 07:47, Davidlohr Bueso <dave-h16yJtLeMjHk1uMJSBkQmQ@public.gmane.org> wrote:
> On Tue, 2015-06-23 at 00:25 +0200, Marcus Gelderie wrote:
>> A while back, the message queue implementation in the kernel was
>> improved to use btrees to speed up retrieval of messages (commit
>> d6629859b36). The patch introducing the improved kernel handling of
>> message queues (using btrees) has, as a by-product, changed the
>> meaning of the QSIZE field in the pseudo-file created for the queue.
>> Before, this field reflected the size of the user-data in the queue.
>> Since, it also takes kernel data structures into account. For
>> example, if 13 bytes of user data are in the queue, on my machine the
>> file reports a size of 61 bytes.
>
> Good catch, and a nice opportunity to make the mq manpage more specific
> wrt to queue sizes.
>
> [...]
>
>> Reporting the size of the message queue in kernel has its merits, but
>> doing so in the QSIZE field of the pseudo file corresponding to the
>> queue is a breaking change, as mentioned above. This patch therefore
>> returns the QSIZE field to its original meaning. At the same time,
>> it introduces a new field QKERSIZE that reflects the size of the queue
>> in kernel (user data + kernel data).
>
> Hmmm I'm not sure about this. What are the specific benefits of having
> QKERSIZE? We don't export in-kernel data like this in any other ipc
> (posix or sysv) mechanism, afaik. Plus, we do not compromise kernel data
> structures like this, as we would break userspace if later we change
> posix_msg_tree_node. So NAK to this.
>
> I would just remove the extra
> + info->qsize += sizeof(struct posix_msg_tree_node);
>
> bits from d6629859b36 (along with -stable v3.5), plus a patch updating
> the manpage that this field only reflects user data.
I've been hoping that Doug would jump into this discussion...
If I recall/understand Doug correctly (see
http://thread.gmane.org/gmane.linux.man/7050/focus=1797645 ), his
rationale for the QSIZE change was that it then revealed a value that
was closer to what was being used to account against the
RLIMIT_MSGQUEUE resource limit. (Even with these changes, the QSIZE
value was not 100% accurate for accounting against RLIMIT_MSGQUEUE,
since some pieces of kernel overhead were still not being accounted
for. Nevertheless, it's much closer than the old (pre 3.5) QSIZE for
some corner cases.) Thus, Marcus's rationale for preserving this info
as QKERSIZE.
Now whether QKERSIZE is actually useful or used by anyone is another
question. As far as I know, there was no user request that drove the
change. But Doug can perhaps say something to this. QSIZE should I
think definitely be fixed (reverted to pre-3.5 behavior). I'm agnostic
about QKERSIZE.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH v6 10/12] KVM: arm64: guest debug, HW assisted debug support
From: Christoffer Dall @ 2015-06-25 7:49 UTC (permalink / raw)
To: Alex Bennée
Cc: kvm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg, marc.zyngier-5wv7dgnIgG8,
peter.maydell-QSEj5FYQhm4dnm+yROfE0A, agraf-l3A5Bk7waGM,
drjones-H+wXaHxf7aLQT0dZR+AlfA, pbonzini-H+wXaHxf7aLQT0dZR+AlfA,
zhichao.huang-QSEj5FYQhm4dnm+yROfE0A,
jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ,
dahi-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
r65777-KZfg59tc24xl57MIdRCFDg, bp-l3A5Bk7waGM, Gleb Natapov,
Jonathan Corbet, Russell King, Catalin Marinas, Will Deacon,
Peter Zijlstra, Lorenzo Pieralisi, Ingo Molnar,
open list:DOCUMENTATION, open list, open list:ABI/API
In-Reply-To: <87mvzoxpg6.fsf-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Thu, Jun 25, 2015 at 07:38:33AM +0100, Alex Bennée wrote:
>
> Christoffer Dall <christoffer.dall-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
>
> > On Fri, Jun 19, 2015 at 01:23:48PM +0100, Alex Bennée wrote:
> >> This adds support for userspace to control the HW debug registers for
> >> guest debug. In the debug ioctl we copy the IMPDEF defined number of
> >
> > s/defined//
> >
> >> registers into a new register set called host_debug_state. There is now
> >> a new vcpu parameter called debug_ptr which selects which register set
> >> is to copied into the real registers when world switch occurs.
> >
> > But this patch doesn't seem to add the debug_ptr field?
>
> Oops, yes the comment belongs to the previous patch.
>
> >
> > s/to//
> >
> >>
> >> I've moved some helper functions into the hw_breakpoint.h header for
> >> re-use.
> >>
> >> As with single step we need to tweak the guest registers to enable the
> >> exceptions so we need to save and restore those bits.
> >>
> >> Two new capabilities have been added to the KVM_EXTENSION ioctl to allow
> >> userspace to query the number of hardware break and watch points
> >> available on the host hardware.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>
> >> ---
> >> v2
> >> - switched to C setup
> >> - replace host debug registers directly into context
> >> - minor tweak to api docs
> >> - setup right register for debug
> >> - add FAR_EL2 to debug exit structure
> >> - add support for trapping debug register access
> >> v3
> >> - remove stray trace statement
> >> - fix spacing around operators (various)
> >> - clean-up usage of trap_debug
> >> - introduce debug_ptr, replace excessive memcpy stuff
> >> - don't use memcpy in ioctl, just assign
> >> - update cap ioctl documentation
> >> - reword a number comments
> >> - rename host_debug_state->external_debug_state
> >> v4
> >> - use the new u32/u64 split debug_ptr approach
> >> - fix some wording/comments
> >> v5
> >> - don't set MDSCR_EL1.KDE (not needed)
> >> v6
> >> - update wording given change in commentary
> >> - KVM_GUESTDBG_USE_HW_BP->KVM_GUESTDBG_USE_HW
> >> ---
> >> Documentation/virtual/kvm/api.txt | 7 ++++++-
> >> arch/arm/kvm/arm.c | 7 +++++++
> >> arch/arm64/include/asm/hw_breakpoint.h | 12 +++++++++++
> >> arch/arm64/include/asm/kvm_host.h | 6 +++++-
> >> arch/arm64/kernel/hw_breakpoint.c | 12 -----------
> >> arch/arm64/kvm/debug.c | 37 +++++++++++++++++++++++++++++-----
> >> arch/arm64/kvm/handle_exit.c | 6 ++++++
> >> arch/arm64/kvm/reset.c | 12 +++++++++++
> >> include/uapi/linux/kvm.h | 2 ++
> >> 9 files changed, 82 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> >> index 33c8143..ada57df 100644
> >> --- a/Documentation/virtual/kvm/api.txt
> >> +++ b/Documentation/virtual/kvm/api.txt
> >> @@ -2668,7 +2668,7 @@ The top 16 bits of the control field are architecture specific control
> >> flags which can include the following:
> >>
> >> - KVM_GUESTDBG_USE_SW_BP: using software breakpoints [x86, arm64]
> >> - - KVM_GUESTDBG_USE_HW_BP: using hardware breakpoints [x86, s390]
> >> + - KVM_GUESTDBG_USE_HW_BP: using hardware breakpoints [x86, s390, arm64]
> >> - KVM_GUESTDBG_INJECT_DB: inject DB type exception [x86]
> >> - KVM_GUESTDBG_INJECT_BP: inject BP type exception [x86]
> >> - KVM_GUESTDBG_EXIT_PENDING: trigger an immediate guest exit [s390]
> >> @@ -2683,6 +2683,11 @@ updated to the correct (supplied) values.
> >> The second part of the structure is architecture specific and
> >> typically contains a set of debug registers.
> >>
> >> +For arm64 the number of debug registers is implementation defined and
> >> +can be determined by querying the KVM_CAP_GUEST_DEBUG_HW_BPS and
> >> +KVM_CAP_GUEST_DEBUG_HW_WPS capabilities which return a positive number
> >> +indicating the number of supported registers.
> >> +
> >> When debug events exit the main run loop with the reason
> >> KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run
> >> structure containing architecture specific debug information.
> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >> index 0d17c7b..60c4045 100644
> >> --- a/arch/arm/kvm/arm.c
> >> +++ b/arch/arm/kvm/arm.c
> >> @@ -307,6 +307,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> >>
> >> #define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE | \
> >> KVM_GUESTDBG_USE_SW_BP | \
> >> + KVM_GUESTDBG_USE_HW | \
> >> KVM_GUESTDBG_SINGLESTEP)
> >>
> >> /**
> >> @@ -327,6 +328,12 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> >>
> >> if (dbg->control & KVM_GUESTDBG_ENABLE) {
> >> vcpu->guest_debug = dbg->control;
> >> +
> >> + /* Hardware assisted Break and Watch points */
> >> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
> >> + vcpu->arch.external_debug_state = dbg->arch;
> >> + }
> >> +
> >> } else {
> >> /* If not enabled clear all flags */
> >> vcpu->guest_debug = 0;
> >> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> >> index 52b484b..c450552 100644
> >> --- a/arch/arm64/include/asm/hw_breakpoint.h
> >> +++ b/arch/arm64/include/asm/hw_breakpoint.h
> >> @@ -130,6 +130,18 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
> >> }
> >> #endif
> >>
> >> +/* Determine number of BRP registers available. */
> >> +static inline int get_num_brps(void)
> >> +{
> >> + return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> >> +}
> >> +
> >> +/* Determine number of WRP registers available. */
> >> +static inline int get_num_wrps(void)
> >> +{
> >> + return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> >> +}
> >> +
> >> extern struct pmu perf_ops_bp;
> >>
> >> #endif /* __KERNEL__ */
> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >> index 9697daf..0a3ee7b 100644
> >> --- a/arch/arm64/include/asm/kvm_host.h
> >> +++ b/arch/arm64/include/asm/kvm_host.h
> >> @@ -116,13 +116,17 @@ struct kvm_vcpu_arch {
> >> * debugging the guest from the host and to maintain separate host and
> >> * guest state during world switches. vcpu_debug_state are the debug
> >> * registers of the vcpu as the guest sees them. host_debug_state are
> >> - * the host registers which are saved and restored during world switches.
> >> + * the host registers which are saved and restored during
> >> + * world switches. external_debug_state contains the debug
> >> + * values we want to debugging the guest. This is set via the
> >
> > nit: s/debugging/debug/
> >
> >> + * KVM_SET_GUEST_DEBUG ioctl.
> >> *
> >> * debug_ptr points to the set of debug registers that should be loaded
> >> * onto the hardware when running the guest.
> >> */
> >> struct kvm_guest_debug_arch *debug_ptr;
> >> struct kvm_guest_debug_arch vcpu_debug_state;
> >> + struct kvm_guest_debug_arch external_debug_state;
> >>
> >> /* Pointer to host CPU context */
> >> kvm_cpu_context_t *host_cpu_context;
> >> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> >> index e7d934d..3a41bbf 100644
> >> --- a/arch/arm64/kernel/hw_breakpoint.c
> >> +++ b/arch/arm64/kernel/hw_breakpoint.c
> >> @@ -49,18 +49,6 @@ static DEFINE_PER_CPU(int, stepping_kernel_bp);
> >> static int core_num_brps;
> >> static int core_num_wrps;
> >>
> >> -/* Determine number of BRP registers available. */
> >> -static int get_num_brps(void)
> >> -{
> >> - return ((read_cpuid(ID_AA64DFR0_EL1) >> 12) & 0xf) + 1;
> >> -}
> >> -
> >> -/* Determine number of WRP registers available. */
> >> -static int get_num_wrps(void)
> >> -{
> >> - return ((read_cpuid(ID_AA64DFR0_EL1) >> 20) & 0xf) + 1;
> >> -}
> >> -
> >> int hw_breakpoint_slots(int type)
> >> {
> >> /*
> >> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> >> index d439eb8..b287bbc 100644
> >> --- a/arch/arm64/kvm/debug.c
> >> +++ b/arch/arm64/kvm/debug.c
> >> @@ -96,10 +96,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> >> MDCR_EL2_TDRA |
> >> MDCR_EL2_TDOSA);
> >>
> >> - /* Trap on access to debug registers? */
> >> - if (trap_debug)
> >> - vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
> >> -
> >> /* Is Guest debugging in effect? */
> >> if (vcpu->guest_debug) {
> >> /* Route all software debug exceptions to EL2 */
> >> @@ -134,11 +130,42 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> >> } else {
> >> vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
> >> }
> >> +
> >> + /*
> >> + * HW Breakpoints and watchpoints
> >> + *
> >> + * We simply switch the debug_ptr to point to our new
> >> + * external_debug_state which has been populated by the
> >> + * debug ioctl. The existing KVM_ARM64_DEBUG_DIRTY
> >> + * mechanism ensures the registers are updated on the
> >> + * world switch.
> >> + */
> >> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
> >> + /* Enable breakpoints/watchpoints */
> >> + vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_MDE;
> >> +
> >> + vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
> >> + vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
> >> + trap_debug = true;
> >> + }
> >> }
> >> +
> >> + /* Trap debug register access */
> >> + if (trap_debug)
> >> + vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
> >> }
> >>
> >> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
> >> {
> >> - if (vcpu->guest_debug)
> >> + if (vcpu->guest_debug) {
> >> restore_guest_debug_regs(vcpu);
> >> +
> >> + /*
> >> + * If we were using HW debug we need to restore the
> >> + * debug_ptr to the guest debug state.
> >> + */
> >> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW)
> >> + vcpu->arch.debug_ptr = &vcpu->arch.vcpu_debug_state;
> >
> > I still think this would be more cleanly done in the setup_debug
> > function, but ok:
>
> I don't follow, setup_debug is called before we enter KVM. It's pretty
> light when no debugging is being done so this ensure we leave state how
> we would like it when we stop debugging.
>
> I can move it to an else leg in setup if you really want.
>
I just feel like whenever you enter the guest you setup the state you
want for your guest and then when reading the code you never have to
worry about "did I set the pointer back correctly last time it exited",
but thinking about your response, I guess that's an extra store on each
world-switch, so theoretically that may be a bit more overhead (on top
of the hundreds other stores and spinlocks we take and stuff).
If you prefer, leave it like this, but consider adding a
BUG_ON(!guest_debugging && debug_ptr != &vcpu->arch.vcpu_debug_state) in
the setup function...
I'm probably being paranoid.
-Christoffer
^ permalink raw reply
* Re: [PATCH v6 10/12] KVM: arm64: guest debug, HW assisted debug support
From: Alex Bennée @ 2015-06-25 10:42 UTC (permalink / raw)
To: Christoffer Dall
Cc: kvm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg, marc.zyngier-5wv7dgnIgG8,
peter.maydell-QSEj5FYQhm4dnm+yROfE0A, agraf-l3A5Bk7waGM,
drjones-H+wXaHxf7aLQT0dZR+AlfA, pbonzini-H+wXaHxf7aLQT0dZR+AlfA,
zhichao.huang-QSEj5FYQhm4dnm+yROfE0A,
jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ,
dahi-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
r65777-KZfg59tc24xl57MIdRCFDg, bp-l3A5Bk7waGM, Gleb Natapov,
Jonathan Corbet, Russell King, Catalin Marinas, Will Deacon,
Peter Zijlstra, Lorenzo Pieralisi, Ingo Molnar,
open list:DOCUMENTATION, open list, open list:ABI/API
In-Reply-To: <20150625074934.GB28244@cbox>
Christoffer Dall <christoffer.dall-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
> On Thu, Jun 25, 2015 at 07:38:33AM +0100, Alex Bennée wrote:
>>
>> Christoffer Dall <christoffer.dall-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
>>
>> > On Fri, Jun 19, 2015 at 01:23:48PM +0100, Alex Bennée wrote:
>> >> This adds support for userspace to control the HW debug registers for
>> >> guest debug. In the debug ioctl we copy the IMPDEF defined number of
<snip>
>> >> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>> >> {
>> >> - if (vcpu->guest_debug)
>> >> + if (vcpu->guest_debug) {
>> >> restore_guest_debug_regs(vcpu);
>> >> +
>> >> + /*
>> >> + * If we were using HW debug we need to restore the
>> >> + * debug_ptr to the guest debug state.
>> >> + */
>> >> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW)
>> >> + vcpu->arch.debug_ptr = &vcpu->arch.vcpu_debug_state;
>> >
>> > I still think this would be more cleanly done in the setup_debug
>> > function, but ok:
>>
>> I don't follow, setup_debug is called before we enter KVM. It's pretty
>> light when no debugging is being done so this ensure we leave state how
>> we would like it when we stop debugging.
>>
>> I can move it to an else leg in setup if you really want.
>>
> I just feel like whenever you enter the guest you setup the state you
> want for your guest and then when reading the code you never have to
> worry about "did I set the pointer back correctly last time it exited",
> but thinking about your response, I guess that's an extra store on each
> world-switch, so theoretically that may be a bit more overhead (on top
> of the hundreds other stores and spinlocks we take and stuff).
The setup/clear() calls are tightly paired around the KVM_RUN ioctl code
without any obvious exit points.
Are there any cases you can escape the ioctl code flow? I notice irq's
are re-enabled so I guess a suitably determined irq function could
change the return address or mess around with guest_debug.
> If you prefer, leave it like this, but consider adding a
> BUG_ON(!guest_debugging && debug_ptr != &vcpu->arch.vcpu_debug_state) in
> the setup function...
The clear_debug() code would end up being a fairly sparse piece of code
without it ;-)
> I'm probably being paranoid.
A little paranoia goes a long way in kernel mode ;-)
>
> -Christoffer
--
Alex Bennée
^ permalink raw reply
* Re: [RESEND PATCH V2 0/3] Allow user to request memory to be locked on page fault
From: Eric B Munson @ 2015-06-25 14:16 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andrew Morton, Shuah Khan, Michal Hocko, Michael Kerrisk,
linux-alpha, linux-kernel, linux-mips, linux-parisc, linuxppc-dev,
sparclinux, linux-xtensa, linux-mm, linux-arch, linux-api
In-Reply-To: <55895956.5020707@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 2978 bytes --]
On Tue, 23 Jun 2015, Vlastimil Babka wrote:
> On 06/15/2015 04:43 PM, Eric B Munson wrote:
> >>Note that the semantic of MAP_LOCKED can be subtly surprising:
> >>
> >>"mlock(2) fails if the memory range cannot get populated to guarantee
> >>that no future major faults will happen on the range.
> >>mmap(MAP_LOCKED) on the other hand silently succeeds even if the
> >>range was populated only
> >>partially."
> >>
> >>( from http://marc.info/?l=linux-mm&m=143152790412727&w=2 )
> >>
> >>So MAP_LOCKED can silently behave like MAP_LOCKONFAULT. While
> >>MAP_LOCKONFAULT doesn't suffer from such problem, I wonder if that's
> >>sufficient reason not to extend mmap by new mlock() flags that can
> >>be instead applied to the VMA after mmapping, using the proposed
> >>mlock2() with flags. So I think instead we could deprecate
> >>MAP_LOCKED more prominently. I doubt the overhead of calling the
> >>extra syscall matters here?
> >
> >We could talk about retiring the MAP_LOCKED flag but I suspect that
> >would get significantly more pushback than adding a new mmap flag.
>
> Oh no we can't "retire" as in remove the flag, ever. Just not
> continue the way of mmap() flags related to mlock().
>
> >Likely that the overhead does not matter in most cases, but presumably
> >there are cases where it does (as we have a MAP_LOCKED flag today).
> >Even with the proposed new system calls I think we should have the
> >MAP_LOCKONFAULT for parity with MAP_LOCKED.
>
> I'm not convinced, but it's not a major issue.
>
> >>
> >>>- mlock() takes a `flags' argument. Presently that's
> >>> MLOCK_LOCKED|MLOCK_LOCKONFAULT.
> >>>
> >>>- munlock() takes a `flags' arument. MLOCK_LOCKED|MLOCK_LOCKONFAULT
> >>> to specify which flags are being cleared.
> >>>
> >>>- mlockall() and munlockall() ditto.
> >>>
> >>>
> >>>IOW, LOCKED and LOCKEDONFAULT are treated identically and independently.
> >>>
> >>>Now, that's how we would have designed all this on day one. And I
> >>>think we can do this now, by adding new mlock2() and munlock2()
> >>>syscalls. And we may as well deprecate the old mlock() and munlock(),
> >>>not that this matters much.
> >>>
> >>>*should* we do this? I'm thinking "yes" - it's all pretty simple
> >>>boilerplate and wrappers and such, and it gets the interface correct,
> >>>and extensible.
> >>
> >>If the new LOCKONFAULT functionality is indeed desired (I haven't
> >>still decided myself) then I agree that would be the cleanest way.
> >
> >Do you disagree with the use cases I have listed or do you think there
> >is a better way of addressing those cases?
>
> I'm somewhat sceptical about the security one. Are security
> sensitive buffers that large to matter? The performance one is more
> convincing and I don't see a better way, so OK.
They can be, the two that come to mind are medical images and high
resolution sensor data.
>
> >
> >>
> >>>What do others think?
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH V2 0/3] Allow user to request memory to be locked on page fault
From: Andy Lutomirski @ 2015-06-25 14:26 UTC (permalink / raw)
To: Eric B Munson
Cc: Vlastimil Babka, Andrew Morton, Shuah Khan, Michal Hocko,
Michael Kerrisk, linux-alpha, linux-kernel@vger.kernel.org,
Linux MIPS Mailing List, linux-parisc, linuxppc-dev, sparclinux,
linux-xtensa, linux-mm@kvack.org, linux-arch, Linux API
In-Reply-To: <20150625141638.GF2329@akamai.com>
On Thu, Jun 25, 2015 at 7:16 AM, Eric B Munson <emunson@akamai.com> wrote:
> On Tue, 23 Jun 2015, Vlastimil Babka wrote:
>
>> On 06/15/2015 04:43 PM, Eric B Munson wrote:
>> >>
>> >>If the new LOCKONFAULT functionality is indeed desired (I haven't
>> >>still decided myself) then I agree that would be the cleanest way.
>> >
>> >Do you disagree with the use cases I have listed or do you think there
>> >is a better way of addressing those cases?
>>
>> I'm somewhat sceptical about the security one. Are security
>> sensitive buffers that large to matter? The performance one is more
>> convincing and I don't see a better way, so OK.
>
> They can be, the two that come to mind are medical images and high
> resolution sensor data.
I think we've been handling sensitive memory pages wrong forever. We
shouldn't lock them into memory; we should flag them as sensitive and
encrypt them if they're ever written out to disk.
--Andy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RESEND PATCH V2 1/3] Add mmap flag to request pages are locked after page fault
From: Eric B Munson @ 2015-06-25 14:46 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, linux-alpha, linux-kernel, linux-mips,
linux-parisc, linuxppc-dev, sparclinux, linux-xtensa, linux-mm,
linux-arch, linux-api
In-Reply-To: <20150624085013.GB32756@dhcp22.suse.cz>
[-- Attachment #1: Type: text/plain, Size: 5707 bytes --]
On Wed, 24 Jun 2015, Michal Hocko wrote:
> On Mon 22-06-15 10:18:06, Eric B Munson wrote:
> > On Mon, 22 Jun 2015, Michal Hocko wrote:
> >
> > > On Fri 19-06-15 12:43:33, Eric B Munson wrote:
> [...]
> > > > Are you objecting to the addition of the VMA flag VM_LOCKONFAULT, or the
> > > > new MAP_LOCKONFAULT flag (or both)?
> > >
> > > I thought the MAP_FAULTPOPULATE (or any other better name) would
> > > directly translate into VM_FAULTPOPULATE and wouldn't be tight to the
> > > locked semantic. We already have VM_LOCKED for that. The direct effect
> > > of the flag would be to prevent from population other than the direct
> > > page fault - including any speculative actions like fault around or
> > > read-ahead.
> >
> > I like the ability to control other speculative population, but I am not
> > sure about overloading it with the VM_LOCKONFAULT case. Here is my
> > concern. If we are using VM_FAULTPOPULATE | VM_LOCKED to denote
> > LOCKONFAULT, how can we tell the difference between someone that wants
> > to avoid read-ahead and wants to use mlock()?
>
> Not sure I understand. Something like?
> addr = mmap(VM_FAULTPOPULATE) # To prevent speculative mappings into the vma
> [...]
> mlock(addr, len) # Now I want the full mlock semantic
So this leaves us without the LOCKONFAULT semantics? That is not at all
what I am looking for. What I want is a way to express 3 possible
states of a VMA WRT locking, locked (populated and all pages on the
unevictable LRU), lock on fault (populated by page fault, pages that are
present are on the unevictable LRU, newly faulted pages are added to
same), and not locked.
>
> and the later to have the full mlock semantic and populate the given
> area regardless of VM_FAULTPOPULATE being set on the vma? This would
> be an interesting question because mlock man page clearly states the
> semantic and that is to _always_ populate or fail. So I originally
> thought that it would obey VM_FAULTPOPULATE but this needs a more
> thinking.
>
> > This might lead to some
> > interesting states with mlock() and munlock() that take flags. For
> > instance, using VM_LOCKONFAULT mlock(MLOCK_ONFAULT) followed by
> > munlock(MLOCK_LOCKED) leaves the VMAs in the same state with
> > VM_LOCKONFAULT set.
>
> This is really confusing. Let me try to rephrase that. So you have
> mlock(addr, len, MLOCK_ONFAULT)
> munlock(addr, len, MLOCK_LOCKED)
>
> IIUC you would expect the vma still being MLOCK_ONFAULT, right? Isn't
> that behavior strange and unexpected? First of all, munlock has
> traditionally dropped the lock on the address range (e.g. what should
> happen if you did plain old munlock(addr, len)). But even without
> that. You are trying to unlock something that hasn't been locked the
> same way. So I would expect -EINVAL at least, if the two modes should be
> really represented by different flags.
I would expect it to remain MLOCK_LOCKONFAULT because the user requested
munlock(addr, len, MLOCK_LOCKED). It is not currently an error to
unlock memory that is not locked. We do this because we do not require
the user track what areas are locked. It is acceptable to have a mostly
locked area with holes unlocked with a single call to munlock that spans
the entire area. The same semantics should hold for munlock with flags.
If I have an area with MLOCK_LOCKED and MLOCK_ONFAULT interleaved, it
should be acceptable to clear the MLOCK_ONFAULT flag from those areas
with a single munlock call that spans the area.
On top of continuing with munlock semantics, the implementation would
need the ability to rollback an munlock call if it failed after altering
VMAs. If we have the same interleaved area as before and we go to
return -EINVAL the first time we hit an area that was MLOCK_LOCKED, how
do we restore the state of the VMAs we have already processed, and
possibly merged/split?
>
> Or did you mean the both types of lock like:
> mlock(addr, len, MLOCK_ONFAULT) | mmap(MAP_LOCKONFAULT)
> mlock(addr, len, MLOCK_LOCKED)
> munlock(addr, len, MLOCK_LOCKED)
>
> and that should keep MLOCK_ONFAULT?
> This sounds even more weird to me because that means that the vma in
> question would be locked by two different mechanisms. MLOCK_LOCKED with
> the "always populate" semantic would rule out MLOCK_ONFAULT so what
> would be the meaning of the other flag then? Also what should regular
> munlock(addr, len) without flags unlock? Both?
This is indeed confusing and not what I was trying to illustrate, but
since you bring it up. mlockall() currently clears all flags and then
sets the new flags with each subsequent call. mlock2 would use that
same behavior, if LOCKED was specified for a ONFAULT region, that region
would become LOCKED and vice versa.
I have the new system call set ready, I am waiting to post for rc1 so I
can run the benchmarks again on a base more stable than the middle of a
merge window. We should wait to hash out implementations until the code
is up rather than talk past eachother here.
>
> > If we use VM_FAULTPOPULATE, the same pair of calls
> > would clear VM_LOCKED, but leave VM_FAULTPOPULATE. It may not matter in
> > the end, but I am concerned about the subtleties here.
>
> This sounds like the proper behavior to me. munlock should simply always
> drop VM_LOCKED and the VM_FAULTPOPULATE can live its separate life.
>
> Btw. could you be more specific about semantic of m{un}lock(addr, len, flags)
> you want to propose? The more I think about that the more I am unclear
> about it, especially munlock behavior and possible flags.
> --
> Michal Hocko
> SUSE Labs
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH RFC] vhost: add ioctl to query nregions upper limit
From: Igor Mammedov @ 2015-06-25 16:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Paolo Bonzini,
kvm-u79uwXL29TY76Z2rM5mHXA,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150624170223-mutt-send-email-mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, 24 Jun 2015 17:08:56 +0200
"Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, Jun 24, 2015 at 04:52:29PM +0200, Igor Mammedov wrote:
> > On Wed, 24 Jun 2015 16:17:46 +0200
> > "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> > > On Wed, Jun 24, 2015 at 04:07:27PM +0200, Igor Mammedov wrote:
> > > > On Wed, 24 Jun 2015 15:49:27 +0200
> > > > "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > >
> > > > > Userspace currently simply tries to give vhost as many regions
> > > > > as it happens to have, but you only have the mem table
> > > > > when you have initialized a large part of VM, so graceful
> > > > > failure is very hard to support.
> > > > >
> > > > > The result is that userspace tends to fail catastrophically.
> > > > >
> > > > > Instead, add a new ioctl so userspace can find out how much
> > > > > kernel supports, up front. This returns a positive value that
> > > > > we commit to.
> > > > >
> > > > > Also, document our contract with legacy userspace: when
> > > > > running on an old kernel, you get -1 and you can assume at
> > > > > least 64 slots. Since 0 value's left unused, let's make that
> > > > > mean that the current userspace behaviour (trial and error)
> > > > > is required, just in case we want it back.
> > > > >
> > > > > Signed-off-by: Michael S. Tsirkin <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > > > Cc: Igor Mammedov <imammedo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > > > Cc: Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > > > ---
> > > > > include/uapi/linux/vhost.h | 17 ++++++++++++++++-
> > > > > drivers/vhost/vhost.c | 5 +++++
> > > > > 2 files changed, 21 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/include/uapi/linux/vhost.h
> > > > > b/include/uapi/linux/vhost.h index ab373191..f71fa6d 100644
> > > > > --- a/include/uapi/linux/vhost.h
> > > > > +++ b/include/uapi/linux/vhost.h
> > > > > @@ -80,7 +80,7 @@ struct vhost_memory {
> > > > > * Allows subsequent call to VHOST_OWNER_SET to succeed. */
> > > > > #define VHOST_RESET_OWNER _IO(VHOST_VIRTIO, 0x02)
> > > > >
> > > > > -/* Set up/modify memory layout */
> > > > > +/* Set up/modify memory layout: see also
> > > > > VHOST_GET_MEM_MAX_NREGIONS below. */ #define
> > > > > VHOST_SET_MEM_TABLE _IOW(VHOST_VIRTIO, 0x03, struct
> > > > > vhost_memory) /* Write logging setup. */
> > > > > @@ -127,6 +127,21 @@ struct vhost_memory {
> > > > > /* Set eventfd to signal an error */
> > > > > #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct
> > > > > vhost_vring_file)
> > > > > +/* Query upper limit on nregions in VHOST_SET_MEM_TABLE
> > > > > arguments.
> > > > > + * Returns:
> > > > > + * 0 < value <= MAX_INT - gives the upper limit,
> > > > > higher values will fail
> > > > > + * 0 - there's no static limit: try and see if it
> > > > > works
> > > > > + * -1 - on failure
> > > > > + */
> > > > > +#define VHOST_GET_MEM_MAX_NREGIONS _IO(VHOST_VIRTIO, 0x23)
> > > > > +
> > > > > +/* Returned by VHOST_GET_MEM_MAX_NREGIONS to mean there's no
> > > > > static limit:
> > > > > + * try and it'll work if you are lucky. */
> > > > > +#define VHOST_MEM_MAX_NREGIONS_NONE 0
> > > > is it needed? we always have a limit,
> > > > or don't have IOCTL => -1 => old try and see way
> > > >
> > > > > +/* We support at least as many nregions in
> > > > > VHOST_SET_MEM_TABLE:
> > > > > + * for use on legacy kernels without
> > > > > VHOST_GET_MEM_MAX_NREGIONS support. */ +#define
> > > > > VHOST_MEM_MAX_NREGIONS_DEFAULT 64
> > > > ^^^ not used below,
> > > > if it's for legacy then perhaps s/DEFAULT/LEGACY/
> > >
> > > The assumption was that userspace detecting old kernels will just
> > > use 64, this means we do want a flag to get the old way.
> > >
> > > OTOH if you won't think it's useful, let me know.
> > this header will be synced into QEMU's tree so that we could use
> > this define there, isn't it? IMHO then _LEGACY is more exact
> > description of macro.
> >
> > As for 0 return value, -1 is just fine for detecting old kernels
> > (i.e. try and see if it works), so 0 looks unnecessary but it
> > doesn't in any way hurt either. For me limit or -1 is enough to try
> > fix userspace.
>
> OK.
> Do you want to try now before I do v2?
I've just tried, idea to check limit is unusable in this case.
here is a link to a patch that implements it:
https://github.com/imammedo/qemu/commits/vhost_slot_limit_check
slots count is changing dynamically depending on used devices
and more importantly guest OS could change slots count during
its runtime when during managing devices it could trigger
repartitioning of current memory table as device's memory regions
mapped into address space.
That leads to 2 different values of used slots at guest startup
time and after guest booted or after hotplug.
I my case guest could be started with max 58 DIMMs coldplugged,
but after boot 3 more slots are freed and it's possible to hotadd
3 more DIMMs. That however leads to the guest that can't be migrated
to since by QEMU design all hotplugged devices should be present
at target's startup time i.e. 60 DIMMs total and that obviously
goes above vhost limit at that time.
Other issue with it is that QEMU could report only current
limit to mgmt tools, so they can't know for sure how many slots
exactly they can allow user to set when creating VM and will have to
guess or create a VM with unusable/under provisioned slots.
We have a similar limit check for kvm memslots but that is never
exercised path since we rose limit in kernel to 509 (similar issues),
over-provisioning it in about 2 times of currently possible
userspace consumption.
>
> > >
> > > > > +
> > > > > /* VHOST_NET specific defines */
> > > > >
> > > > > /* Attach virtio net ring to a raw socket, or tap device.
> > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > > index 9e8e004..3b68f9d 100644
> > > > > --- a/drivers/vhost/vhost.c
> > > > > +++ b/drivers/vhost/vhost.c
> > > > > @@ -917,6 +917,11 @@ long vhost_dev_ioctl(struct vhost_dev
> > > > > *d, unsigned int ioctl, void __user *argp) long r;
> > > > > int i, fd;
> > > > >
> > > > > + if (ioctl == VHOST_GET_MEM_MAX_NREGIONS) {
> > > > > + r = VHOST_MEMORY_MAX_NREGIONS;
> > > > > + goto done;
> > > > > + }
> > > > > +
> > > > > /* If you are not the owner, you can become one */
> > > > > if (ioctl == VHOST_SET_OWNER) {
> > > > > r = vhost_dev_set_owner(d);
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] ipc: Modify message queue accounting to reflect both total user data and auxiliary kernel data
From: Davidlohr Bueso @ 2015-06-25 18:21 UTC (permalink / raw)
To: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w
Cc: Doug Ledford, Marcus Gelderie, lkml, David Howells,
Alexander Viro, John Duffy, Arto Bendiken, Linux API,
Marcus Gelderie, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b
In-Reply-To: <CAKgNAkieR5zdpKm=P2dcTDJ_3X4HMRoeOQ2D8yghYVKOjDsYAg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[CC'ing akpm as he handles such changes]
On Thu, 2015-06-25 at 09:23 +0200, Michael Kerrisk (man-pages) wrote:
> On 25 June 2015 at 07:47, Davidlohr Bueso <dave-h16yJtLeMjHk1uMJSBkQmQ@public.gmane.org> wrote:
> > On Tue, 2015-06-23 at 00:25 +0200, Marcus Gelderie wrote:
> >> A while back, the message queue implementation in the kernel was
> >> improved to use btrees to speed up retrieval of messages (commit
> >> d6629859b36). The patch introducing the improved kernel handling of
> >> message queues (using btrees) has, as a by-product, changed the
> >> meaning of the QSIZE field in the pseudo-file created for the queue.
> >> Before, this field reflected the size of the user-data in the queue.
> >> Since, it also takes kernel data structures into account. For
> >> example, if 13 bytes of user data are in the queue, on my machine the
> >> file reports a size of 61 bytes.
> >
> > Good catch, and a nice opportunity to make the mq manpage more specific
> > wrt to queue sizes.
> >
> > [...]
> >
> >> Reporting the size of the message queue in kernel has its merits, but
> >> doing so in the QSIZE field of the pseudo file corresponding to the
> >> queue is a breaking change, as mentioned above. This patch therefore
> >> returns the QSIZE field to its original meaning. At the same time,
> >> it introduces a new field QKERSIZE that reflects the size of the queue
> >> in kernel (user data + kernel data).
> >
> > Hmmm I'm not sure about this. What are the specific benefits of having
> > QKERSIZE? We don't export in-kernel data like this in any other ipc
> > (posix or sysv) mechanism, afaik. Plus, we do not compromise kernel data
> > structures like this, as we would break userspace if later we change
> > posix_msg_tree_node. So NAK to this.
> >
> > I would just remove the extra
> > + info->qsize += sizeof(struct posix_msg_tree_node);
> >
> > bits from d6629859b36 (along with -stable v3.5), plus a patch updating
> > the manpage that this field only reflects user data.
>
> I've been hoping that Doug would jump into this discussion...
>
> If I recall/understand Doug correctly (see
> http://thread.gmane.org/gmane.linux.man/7050/focus=1797645 ),
Ah so we _had_ this conversation in the past.
> his
> rationale for the QSIZE change was that it then revealed a value that
> was closer to what was being used to account against the
> RLIMIT_MSGQUEUE resource limit. (Even with these changes, the QSIZE
> value was not 100% accurate for accounting against RLIMIT_MSGQUEUE,
> since some pieces of kernel overhead were still not being accounted
> for. Nevertheless, it's much closer than the old (pre 3.5) QSIZE for
> some corner cases.) Thus, Marcus's rationale for preserving this info
> as QKERSIZE.
>
> Now whether QKERSIZE is actually useful or used by anyone is another
> question. As far as I know, there was no user request that drove the
> change. But Doug can perhaps say something to this. QSIZE should I
> think definitely be fixed (reverted to pre-3.5 behavior). I'm agnostic
> about QKERSIZE.
Right, and we seemed to have agreed in the previous thread that the
QSIZE changes should be reverted back to its original values. We also
agree on the main reason why: it exposes unnecessarily kernel
implementation details to userland -- and as such I at least still don't
buy much into the idea of QKERSIZE either.
Thanks,
Davidlohr
^ permalink raw reply
* Re: [PATCH v2] ipc: Modify message queue accounting to reflect both total user data and auxiliary kernel data
From: Marcus Gelderie @ 2015-06-25 18:50 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Davidlohr Bueso, Doug Ledford, lkml, David Howells,
Alexander Viro, John Duffy, Arto Bendiken, Linux API
In-Reply-To: <CAKgNAkieR5zdpKm=P2dcTDJ_3X4HMRoeOQ2D8yghYVKOjDsYAg@mail.gmail.com>
Hey all,
answers in text below...
TLDR: I can remove the QKERSIZE field, as I believe that it does not affect he
RLIMIT accounting (details [=code] below). Question is: Should it?
Before I provide another patch, I would appreciate another round of feedback, though.
So...
On Thu, Jun 25, 2015 at 09:23:33AM +0200, Michael Kerrisk (man-pages) wrote:
> On 25 June 2015 at 07:47, Davidlohr Bueso <dave@stgolabs.net> wrote:
> > Good catch, and a nice opportunity to make the mq manpage more specific
> > wrt to queue sizes.
> >
> > [...]
> >
ACK, I can write a patch for the manpage once we figure out what to do
here.
> >> Reporting the size of the message queue in kernel has its merits, but
> >> doing so in the QSIZE field of the pseudo file corresponding to the
> >> queue is a breaking change, as mentioned above. This patch therefore
> >> returns the QSIZE field to its original meaning. At the same time,
> >> it introduces a new field QKERSIZE that reflects the size of the queue
> >> in kernel (user data + kernel data).
> >
> > Hmmm I'm not sure about this. What are the specific benefits of having
> > QKERSIZE? We don't export in-kernel data like this in any other ipc
> > (posix or sysv) mechanism, afaik. Plus, we do not compromise kernel data
> > structures like this, as we would break userspace if later we change
> > posix_msg_tree_node. So NAK to this.
> >
> > I would just remove the extra
> > + info->qsize += sizeof(struct posix_msg_tree_node);
> >
> > bits from d6629859b36 (along with -stable v3.5), plus a patch updating
> > the manpage that this field only reflects user data.
>
This can be done if the RLIMIT accounting is not done against that
value (more below).
> I've been hoping that Doug would jump into this discussion...
>
> If I recall/understand Doug correctly (see
> http://thread.gmane.org/gmane.linux.man/7050/focus=1797645 ), his
> rationale for the QSIZE change was that it then revealed a value that
> was closer to what was being used to account against the
> RLIMIT_MSGQUEUE resource limit. (Even with these changes, the QSIZE
> value was not 100% accurate for accounting against RLIMIT_MSGQUEUE,
> since some pieces of kernel overhead were still not being accounted
> for. Nevertheless, it's much closer than the old (pre 3.5) QSIZE for
> some corner cases.) Thus, Marcus's rationale for preserving this info
> as QKERSIZE.
>
Actually, looking at the code, it seems to me that the QKERSIZE
(a.k.a. the current qsize field) is actually not used for the purpose of
accounting at all. From ipc/mqueue.c (line 281 ff, comments added by me):
/* worst case estimate for the btree in memory */
mq_treesize = info->attr.mq_maxmsg * sizeof(struct msg_msg) +
min_t(unsigned int, info->attr.mq_maxmsg, MQ_PRIO_MAX) *
sizeof(struct posix_msg_tree_node);
/* worst case estimate for the user data in the queue */
mq_bytes = mq_treesize + (info->attr.mq_maxmsg *
info->attr.mq_msgsize);
spin_lock(&mq_lock);
/* acutal accounting; u->mq_bytes is the other
* message queus for this user (computed in the way
* above (i.e. worst-case estimates)
*/
if (u->mq_bytes + mq_bytes < u->mq_bytes ||
u->mq_bytes + mq_bytes > rlimit(RLIMIT_MSGQUEUE)) {
[....]
ret = -EMFILE;
[.....]
So, I think that if the RLIMIT should take the actual size (not the
worst case) into account, then we have a bug. I can write a patch, but that
should be separate from this... meaning I'll turn this into a patchset.
However, we should decide what we want first: If I read the manpage (mq_overview), I
am inclined to think that the RLIMIT should use the acutal amount of
data occupied by the queue (or at least the user data). But it does not
necessarily rule out an accounting against worst-case estimation (to me
at least).
> Now whether QKERSIZE is actually useful or used by anyone is another
> question. As far as I know, there was no user request that drove the
> change. But Doug can perhaps say something to this. QSIZE should I
> think definitely be fixed (reverted to pre-3.5 behavior). I'm agnostic
> about QKERSIZE.
>
> Cheers,
>
> Michael
>
>
> --
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox