lkml.org 
[lkml]   [2008]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC patch 23/27] Immediate Values - Powerpc Optimization NMI MCE support
* Paul Mackerras (paulus@samba.org) wrote:
> Mathieu Desnoyers writes:
>
> > * Paul Mackerras (paulus@samba.org) wrote:
> > > Mathieu Desnoyers writes:
> > >
> > > > Use an atomic update for immediate values.
> > >
> > > What is meant by an "atomic" update in this context? AFAICS you are
> > > using memcpy, which is not in any way guaranteed to be atomic.
> > >
> > > Paul.
> >
> > I expect memcpy to perform the copy in one memory access, given I put a
> >
> > .align 2
> >
> > before the 2 bytes instruction. It makes sure the instruction modified
> > fits in a single, aligned, memory write.
>
> My original question was in the context of the powerpc architecture,
> where instructions are always 4 bytes long and aligned. So that's not
> an issue.
>

Sorry, I meant 4 byte instruction with 2 bytes immediate value, but we
both understand it would be a memory write aligned on 2 bytes since we
only change the immediate value.

> > Or maybe am I expecting too much from memcpy ?
>
> I don't think memcpy gives you any such guarantees. It would be quite
> within its rights to say "it's only a few bytes, I'll do it byte by
> byte".
>
> If you really want it to be atomic (which I agree is probably a good
> idea), I think the best way to do it is to use an asm to generate a
> sth (store halfword) instruction to the immediate field (instruction
> address + 2). That's on powerpc of course; I don't know what you
> would do on other architectures.
>

A simple

*(uint16_t* )destptr = newvalue;

seems to generate the "sth" instruction.

Do you see any reason why the compiler could choose a different, non
atomic assembler primitive ?

quoting Documentation/RCU/whatisRCU.txt :

"In contrast, RCU-based updaters typically take advantage of the fact
that writes to single aligned pointers are atomic on modern CPUs"

Paul E. McKenney could say if I am wrong if I assume that any object
smaller or equal to the architecture pointer size, aligned on a multiple
of its own size, will be read or written atomically.

Therefore, I would suggest the following replacement patch :


Immediate Values - Powerpc Optimization NMI MCE support

Use an atomic update for immediate values.

- Changelog :
Use a direct assignment instead of memcpy to be sure the update is
atomic.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
CC: Rusty Russell <rusty@rustcorp.com.au>
CC: Christoph Hellwig <hch@infradead.org>
CC: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kernel/Makefile | 1
arch/powerpc/kernel/immediate.c | 70 ++++++++++++++++++++++++++++++++++++++++
include/asm-powerpc/immediate.h | 18 ++++++++++
3 files changed, 89 insertions(+)

Index: linux-2.6-lttng/arch/powerpc/kernel/immediate.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6-lttng/arch/powerpc/kernel/immediate.c 2008-04-16 21:22:29.000000000 -0400
@@ -0,0 +1,70 @@
+/*
+ * Powerpc optimized immediate values enabling/disabling.
+ *
+ * Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
+ */
+
+#include <linux/module.h>
+#include <linux/immediate.h>
+#include <linux/string.h>
+#include <linux/kprobes.h>
+#include <asm/cacheflush.h>
+#include <asm/page.h>
+
+#define LI_OPCODE_LEN 2
+
+/**
+ * arch_imv_update - update one immediate value
+ * @imv: pointer of type const struct __imv to update
+ * @early: early boot (1), normal (0)
+ *
+ * Update one immediate value. Must be called with imv_mutex held.
+ */
+int arch_imv_update(const struct __imv *imv, int early)
+{
+#ifdef CONFIG_KPROBES
+ kprobe_opcode_t *insn;
+ /*
+ * Fail if a kprobe has been set on this instruction.
+ * (TODO: we could eventually do better and modify all the (possibly
+ * nested) kprobes for this site if kprobes had an API for this.
+ */
+ switch (imv->size) {
+ case 1: /* The uint8_t points to the 3rd byte of the
+ * instruction */
+ insn = (void *)(imv->imv - 1 - LI_OPCODE_LEN);
+ break;
+ case 2: insn = (void *)(imv->imv - LI_OPCODE_LEN);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (unlikely(!early && *insn == BREAKPOINT_INSTRUCTION)) {
+ printk(KERN_WARNING "Immediate value in conflict with kprobe. "
+ "Variable at %p, "
+ "instruction at %p, size %lu\n",
+ (void *)imv->imv,
+ (void *)imv->var, imv->size);
+ return -EBUSY;
+ }
+#endif
+
+ /*
+ * If the variable and the instruction have the same value, there is
+ * nothing to do.
+ */
+ switch (imv->size) {
+ case 1: if (*(uint8_t *)imv->imv == *(uint8_t *)imv->var)
+ return 0;
+ *(uint8_t *)imv->imv = *(uint8_t *)imv->var;
+ break;
+ case 2: if (*(uint16_t *)imv->imv == *(uint16_t *)imv->var)
+ return 0;
+ *(uint16_t *)imv->imv = *(uint16_t *)imv->var;
+ break;
+ default:return -EINVAL;
+ }
+ flush_icache_range(imv->imv, imv->imv + imv->size);
+ return 0;
+}
Index: linux-2.6-lttng/include/asm-powerpc/immediate.h
===================================================================
--- linux-2.6-lttng.orig/include/asm-powerpc/immediate.h 2008-04-16 12:25:42.000000000 -0400
+++ linux-2.6-lttng/include/asm-powerpc/immediate.h 2008-04-16 20:49:48.000000000 -0400
@@ -12,6 +12,16 @@

#include <asm/asm-compat.h>

+struct __imv {
+ unsigned long var; /* Identifier variable of the immediate value */
+ unsigned long imv; /*
+ * Pointer to the memory location that holds
+ * the immediate value within the load immediate
+ * instruction.
+ */
+ unsigned char size; /* Type size. */
+} __attribute__ ((packed));
+
/**
* imv_read - read immediate variable
* @name: immediate value name
@@ -19,6 +29,11 @@
* Reads the value of @name.
* Optimized version of the immediate.
* Do not use in __init and __exit functions. Use _imv_read() instead.
+ * Makes sure the 2 bytes update will be atomic by aligning the immediate
+ * value. Use a normal memory read for the 4 bytes immediate because there is no
+ * way to atomically update it without using a seqlock read side, which would
+ * cost more in term of total i-cache and d-cache space than a simple memory
+ * read.
*/
#define imv_read(name) \
({ \
@@ -40,6 +55,7 @@
PPC_LONG "%c1, ((1f)-2)\n\t" \
".byte 2\n\t" \
".previous\n\t" \
+ ".align 2\n\t" \
"li %0,0\n\t" \
"1:\n\t" \
: "=r" (value) \
@@ -52,4 +68,6 @@
value; \
})

+extern int arch_imv_update(const struct __imv *imv, int early);
+
#endif /* _ASM_POWERPC_IMMEDIATE_H */
Index: linux-2.6-lttng/arch/powerpc/kernel/Makefile
===================================================================
--- linux-2.6-lttng.orig/arch/powerpc/kernel/Makefile 2008-04-16 12:23:07.000000000 -0400
+++ linux-2.6-lttng/arch/powerpc/kernel/Makefile 2008-04-16 12:25:44.000000000 -0400
@@ -45,6 +45,7 @@ obj-$(CONFIG_HIBERNATION) += swsusp.o su
obj64-$(CONFIG_HIBERNATION) += swsusp_asm64.o
obj-$(CONFIG_MODULES) += module_$(CONFIG_WORD_SIZE).o
obj-$(CONFIG_44x) += cpu_setup_44x.o
+obj-$(CONFIG_IMMEDIATE) += immediate.o

ifeq ($(CONFIG_PPC_MERGE),y)

--
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68


\
 
 \ /
  Last update: 2008-04-17 03:37    [W:0.093 / U:0.544 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site