Re: [RFC patch 23/27] Immediate Values - Powerpc Optimization NMI MCE support

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Wed, Apr 16, 2008 at 09:24:20PM -0400, Mathieu Desnoyers wrote:
> * 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.

There have been CPUs in the past for which this was false.  I am not aware
of any these days, but I would need to ask the architecture maintainers.

A lot depends on the compiler as well as the CPU, of course.  :-(

							Thanx, Paul

> 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