[RFC local_t removal V1 0/4] Remove local_t
Christoph Lameter
Tracing uses local_t for per cpu safe atomic operations in the form
of cmpxchg and additions. We already have a cmpxchg_local but no "local"
form of addition.
The patchset introduces a similar local primitive add_local() and then
uses cmpxchg_local() and add_local() to remove local_t use from the
trace subsystem.
The last patch removes local_t support from the kernel tree.
The support for add_local() is pretty basic. We can add more
fancy inc/dec variants and more optimization in the next
revision of the patchset.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Mathieu Desnoyers
> Current -next has only the trace subsystem left as a user of local_t
>
> Tracing uses local_t for per cpu safe atomic operations in the form
> of cmpxchg and additions. We already have a cmpxchg_local but no "local"
> form of addition.
>
> The patchset introduces a similar local primitive add_local() and then
> uses cmpxchg_local() and add_local() to remove local_t use from the
> trace subsystem.
>
> The last patch removes local_t support from the kernel tree.
>
> The support for add_local() is pretty basic. We can add more
> fancy inc/dec variants and more optimization in the next
> revision of the patchset.
>
Hi Christoph,
Yes, removing the local_t type could make sense. However, local_t maps
to a volatile long, not just "long". Secondly, I am concerned about the
fact that the patch you propose:
- does not create the primitives I use in lttng
- only deals with x86
In lttng (which is out of tree, but widely used), I need the equivalent
of:
local_read
local_set
local_add
local_cmpxchg
local_add_return
local_inc
The approach of just doing the x86 implementation and leaving all the
other architectures "for later" with a slow/non atomic generic fallback
is, imho, a no-go, given that some people (myself, actually) already
took the time to go through all the kernel architectures to create the
optimized local.h headers. Basically, you are destroying all that work,
asking for it to be done all over again.
I therefore argue that we should keep local.h as-is as long as the
replacement lacks the wide architecture support and primitive variety
found in local.h.
Thanks,
Mathieu
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
Mike Frysinger
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/include/asm-generic/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,13 @@
> +#ifndef __ASM_GENERIC_ADD_LOCAL_H
> +#define __ASM_GENERIC_ADD_LOCAL_H
needs comment header (blurb/copyright/license)
> +#define add_return_local(ptr, v) \
> + ((__typeof__(*(ptr)))__add_return_local_generic((ptr), \
> + (unsigned long)(v), sizeof(*(ptr))))
> +
> +#define add_local(ptr, v) (void)__add_return_local_generic((ptr), \
> + (unsigned long)(v), sizeof(*(ptr)))
the only difference here is the cast ... perhaps define an
_add_local() macro to avoid the duplication
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
these arch stubs all have an extra new line
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/include/asm-generic/add-local-generic.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,40 @@
> +#ifndef __ASM_GENERIC_ADD_LOCAL_GENERIC_H
> +#define __ASM_GENERIC_ADD_LOCAL_GENERIC_H
needs comment header (blurb/copyright/license)
> +/*
> + * Generic version of __add_return_local (disables interrupts). Takes an
> + * unsigned long parameter, supporting various types of architectures.
> + */
> +static inline unsigned long __add_return_local_generic(volatile void *ptr,
> + unsigned long value, int size)
size_t for size ?
> +extern unsigned long wrong_size_add_local(volatile void *ptr);
> ...
> + /*
> + * Sanity checking, compile-time.
> + */
> + if (size == 8 && sizeof(unsigned long) != 8)
> + wrong_size_add_local(ptr);
> ...
> + default:
> + wrong_size_add_local(ptr);
> + }
should be BUILD_BUG_ON() ?
-mike
Christoph Lameter
> Yes, removing the local_t type could make sense. However, local_t maps
> to a volatile long, not just "long". Secondly, I am concerned about the
> fact that the patch you propose:
Volatile is discouraged as far as I can tell.
> - does not create the primitives I use in lttng
> - only deals with x8
As I said its an RFC. This provides all the functionality you need
through. The rest is sugar coating.
> In lttng (which is out of tree, but widely used), I need the equivalent
> of:
>
> local_read
> local_set
> local_add
> local_cmpxchg
> local_add_return
> local_inc
Please read the patch! This is all provided. add_local_return in the RFC
provides what is needed to replace local_add, local_inc. We can add these
explicitly.
local_cmpxchg replacement is already in there in the form of
cmpxchg_local().
> The approach of just doing the x86 implementation and leaving all the
> other architectures "for later" with a slow/non atomic generic fallback
> is, imho, a no-go, given that some people (myself, actually) already
> took the time to go through all the kernel architectures to create the
> optimized local.h headers. Basically, you are destroying all that work,
> asking for it to be done all over again.
AS I said this is an RFC. I can easily generate all these things from the
existing local.hs for the architectures.
> I therefore argue that we should keep local.h as-is as long as the
> replacement lacks the wide architecture support and primitive variety
> found in local.h.
Cool down and please review the patch.
Christoph Lameter
> On Tue, Jan 5, 2010 at 17:04, Christoph Lameter wrote:
> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/include/asm-generic/add-local.h 2010-01-05 15:36:02.000000000 -0600
> > @@ -0,0 +1,13 @@
> > +#ifndef __ASM_GENERIC_ADD_LOCAL_H
> > +#define __ASM_GENERIC_ADD_LOCAL_H
>
> needs comment header (blurb/copyright/license)
A simple small include file? Really?
> > +#define add_return_local(ptr, v) \
> > + ((__typeof__(*(ptr)))__add_return_local_generic((ptr), \
> > + (unsigned long)(v), sizeof(*(ptr))))
> > +
> > +#define add_local(ptr, v) (void)__add_return_local_generic((ptr), \
> > + (unsigned long)(v), sizeof(*(ptr)))
>
> the only difference here is the cast ... perhaps define an
> _add_local() macro to avoid the duplication
Right. This is a shortcut to define the add_local() operation without too
much effort. We should be using add / inc /dec there at some point.
> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> > @@ -0,0 +1,2 @@
> > +#include <asm-generic/add-local.h>
> > +
>
> these arch stubs all have an extra new line
Thats bad?
> > +/*
> > + * Generic version of __add_return_local (disables interrupts). Takes an
> > + * unsigned long parameter, supporting various types of architectures.
> > + */
> > +static inline unsigned long __add_return_local_generic(volatile void *ptr,
> > + unsigned long value, int size)
>
> size_t for size ?
No. It can be anything.
> > +extern unsigned long wrong_size_add_local(volatile void *ptr);
> > ...
> > + /*
> > + * Sanity checking, compile-time.
> > + */
> > + if (size == 8 && sizeof(unsigned long) != 8)
> > + wrong_size_add_local(ptr);
> > ...
> > + default:
> > + wrong_size_add_local(ptr);
> > + }
>
> should be BUILD_BUG_ON() ?
Does not work there.
Mathieu Desnoyers
> is only safe from concurrent access on the local cpu.
>
> Add another operation that in the similar fasion allow "local" safe adds to
> a variable.
>
> Signed-off-by: Christoph Lameter <c...@linux-foundation.org>
>
> ---
> arch/alpha/include/asm/add-local.h | 2 +
> arch/arm/include/asm/add-local.h | 2 +
> arch/avr32/include/asm/add-local.h | 2 +
> arch/blackfin/include/asm/add-local.h | 2 +
> arch/cris/include/asm/add-local.h | 2 +
> arch/frv/include/asm/add-local.h | 2 +
> arch/h8300/include/asm/add-local.h | 2 +
> arch/ia64/include/asm/add-local.h | 2 +
> arch/m32r/include/asm/add-local.h | 2 +
> arch/m68k/include/asm/add-local.h | 2 +
> arch/microblaze/include/asm/add-local.h | 2 +
> arch/mips/include/asm/add-local.h | 2 +
> arch/mn10300/include/asm/add-local.h | 2 +
> arch/parisc/include/asm/add-local.h | 2 +
> arch/powerpc/include/asm/add-local.h | 2 +
> arch/s390/include/asm/add-local.h | 2 +
> arch/score/include/asm/add-local.h | 2 +
> arch/sh/include/asm/add-local.h | 2 +
> arch/sparc/include/asm/add-local.h | 2 +
> arch/um/include/asm/add-local.h | 2 +
> arch/x86/include/asm/add-local.h | 2 +
> arch/xtensa/include/asm/add-local.h | 2 +
> include/asm-generic/add-local-generic.h | 40 ++++++++++++++++++++++++++++++++
> include/asm-generic/add-local.h | 13 ++++++++++
> 24 files changed, 97 insertions(+)
The problem I see here is that with ~5-6 operations, we will end up
having 20*5 = 100 headers only for this. Can we combine these in a
single header file instead ? local.h wasn't bad in this respect.
Also, separating all these in sub-files will make it a bit difficult to
pinpoint errors that would arise from using a bad combination of, e.g.
generic add with a non-protected read or set.
Thanks,
Mathieu
>
> Index: linux-2.6/include/asm-generic/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/include/asm-generic/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,13 @@
> +#ifndef __ASM_GENERIC_ADD_LOCAL_H
> +#define __ASM_GENERIC_ADD_LOCAL_H
> +
> +#include <asm-generic/add-local-generic.h>
> +
> +#define add_return_local(ptr, v) \
> + ((__typeof__(*(ptr)))__add_return_local_generic((ptr), \
> + (unsigned long)(v), sizeof(*(ptr))))
> +
> +#define add_local(ptr, v) (void)__add_return_local_generic((ptr), \
> + (unsigned long)(v), sizeof(*(ptr)))
> +
> +#endif
> Index: linux-2.6/arch/alpha/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/arm/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/arm/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/avr32/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/avr32/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/blackfin/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/blackfin/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/cris/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/cris/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/frv/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/frv/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/h8300/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/h8300/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/ia64/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/ia64/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/m32r/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/m32r/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/m68k/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/m68k/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/microblaze/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/microblaze/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/mips/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/mips/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/mn10300/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/mn10300/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/parisc/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/parisc/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/powerpc/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/powerpc/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/s390/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/s390/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/score/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/score/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/sh/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/sh/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/sparc/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/sparc/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/um/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/um/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/x86/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/x86/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/arch/xtensa/include/asm/add-local.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/arch/xtensa/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,2 @@
> +#include <asm-generic/add-local.h>
> +
> Index: linux-2.6/include/asm-generic/add-local-generic.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/include/asm-generic/add-local-generic.h 2010-01-05 15:36:02.000000000 -0600
> @@ -0,0 +1,40 @@
> +#ifndef __ASM_GENERIC_ADD_LOCAL_GENERIC_H
> +#define __ASM_GENERIC_ADD_LOCAL_GENERIC_H
> +
> +#include <linux/types.h>
> +
> +extern unsigned long wrong_size_add_local(volatile void *ptr);
> +
> +/*
> + * Generic version of __add_return_local (disables interrupts). Takes an
> + * unsigned long parameter, supporting various types of architectures.
> + */
> +static inline unsigned long __add_return_local_generic(volatile void *ptr,
> + unsigned long value, int size)
> +{
> + unsigned long flags, r;
> +
> + /*
> + * Sanity checking, compile-time.
> + */
> + if (size == 8 && sizeof(unsigned long) != 8)
> + wrong_size_add_local(ptr);
> +
> + local_irq_save(flags);
> + switch (size) {
> + case 1: r = (*((u8 *)ptr) += value);
> + break;
> + case 2: r = (*((u16 *)ptr) += value);
> + break;
> + case 4: r = (*((u32 *)ptr) += value);
> + break;
> + case 8: r = (*((u64 *)ptr) += value);
> + break;
> + default:
> + wrong_size_add_local(ptr);
> + }
> + local_irq_restore(flags);
> + return r;
> +}
> +
> +#endif
>
> --
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
Mathieu Desnoyers
>
> > Yes, removing the local_t type could make sense. However, local_t maps
> > to a volatile long, not just "long". Secondly, I am concerned about the
> > fact that the patch you propose:
>
> Volatile is discouraged as far as I can tell.
If you want to ensure that a simple variable assignment or read
(local_set, local_read) are not performed piecewise by the compiler
which can cause odd effects when shared with interrupt handlers, this
will however be necessary.
>
> > - does not create the primitives I use in lttng
> > - only deals with x8
>
> As I said its an RFC. This provides all the functionality you need
> through. The rest is sugar coating.
OK
OK :)
Mathieu
>
>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
Mathieu Desnoyers
> updated with the required level of concurrency protection through cmpxchg_local()
> and add_local().> kernel/trace/ring_buffer.c | 150 +++++++++++++++++------------------
> kernel/trace/ring_buffer_benchmark.c | 5 -
> kernel/trace/trace_branch.c | 1
> 3 files changed, 77 insertions(+), 79 deletions(-)
>
> Index: linux-2.6/kernel/trace/ring_buffer.c
> ===================================================================
> --- linux-2.6.orig/kernel/trace/ring_buffer.c 2010-01-05 15:35:35.000000000 -0600
> +++ linux-2.6/kernel/trace/ring_buffer.c 2010-01-05 15:36:07.000000000 -0600
> @@ -20,7 +20,7 @@
> #include <linux/cpu.h>
> #include <linux/fs.h>
>
> -#include <asm/local.h>
> +#include <asm/add-local.h>
> #include "trace.h"
>
> /*
> @@ -312,7 +312,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_event_data
>
> struct buffer_data_page {
> u64 time_stamp; /* page time stamp */
> - local_t commit; /* write committed index */
> + long commit; /* write committed index */
> unsigned char data[]; /* data of buffer page */
> };
>
> @@ -326,9 +326,9 @@ struct buffer_data_page {
> */
> struct buffer_page {
> struct list_head list; /* list of buffer pages */
> - local_t write; /* index for next write */
> + long write; /* index for next write */
> unsigned read; /* index for next read */
> - local_t entries; /* entries on this page */
> + long entries; /* entries on this page */
> struct buffer_data_page *page; /* Actual data page */
> };
>
> @@ -349,7 +349,7 @@ struct buffer_page {
>
> static void rb_init_page(struct buffer_data_page *bpage)
> {
> - local_set(&bpage->commit, 0);
> + bpage->commit = 0;
This is incorrect. You are turning a "volatile" write into a
non-volatile write, which can be turned into multiple writes by the
compiler and therefore expose inconsistent state to interrupt handlers.
> }
>
> /**
> @@ -360,7 +360,7 @@ static void rb_init_page(struct buffer_d
> */
> size_t ring_buffer_page_len(void *page)
> {
> - return local_read(&((struct buffer_data_page *)page)->commit)
> + return ((struct buffer_data_page *)page)->commit
> + BUF_PAGE_HDR_SIZE;
> }
>
> @@ -431,11 +431,11 @@ struct ring_buffer_per_cpu {
> struct buffer_page *tail_page; /* write to tail */
> struct buffer_page *commit_page; /* committed pages */
> struct buffer_page *reader_page;
> - local_t commit_overrun;
> - local_t overrun;
> - local_t entries;
> - local_t committing;
> - local_t commits;
> + long commit_overrun;
> + long overrun;
> + long entries;
> + long committing;
> + long commits;
> unsigned long read;
> u64 write_stamp;
> u64 read_stamp;
> @@ -824,8 +824,8 @@ static int rb_tail_page_update(struct ri
> *
> * We add a counter to the write field to denote this.
> */
> - old_write = local_add_return(RB_WRITE_INTCNT, &next_page->write);
> - old_entries = local_add_return(RB_WRITE_INTCNT, &next_page->entries);
> + old_write = add_return_local(&next_page->write, RB_WRITE_INTCNT);
> + old_entries = add_return_local(&next_page->entries, RB_WRITE_INTCNT);
>
> /*
> * Just make sure we have seen our old_write and synchronize
> @@ -853,15 +853,15 @@ static int rb_tail_page_update(struct ri
> * cmpxchg to only update if an interrupt did not already
> * do it for us. If the cmpxchg fails, we don't care.
> */
> - (void)local_cmpxchg(&next_page->write, old_write, val);
> - (void)local_cmpxchg(&next_page->entries, old_entries, eval);
> + (void)cmpxchg_local(&next_page->write, old_write, val);
> + (void)cmpxchg_local(&next_page->entries, old_entries, eval);
>
> /*
> * No need to worry about races with clearing out the commit.
> * it only can increment when a commit takes place. But that
> * only happens in the outer most nested commit.
> */
> - local_set(&next_page->page->commit, 0);
> + next_page->page->commit = 0;
>
> old_tail = cmpxchg(&cpu_buffer->tail_page,
> tail_page, next_page);
> @@ -1394,17 +1394,17 @@ rb_iter_head_event(struct ring_buffer_it
>
> static inline unsigned long rb_page_write(struct buffer_page *bpage)
> {
> - return local_read(&bpage->write) & RB_WRITE_MASK;
> + return bpage->write & RB_WRITE_MASK;
Same problem here: missing volatile for read. Same applies thorough the
patch.
> }
>
> static inline unsigned rb_page_commit(struct buffer_page *bpage)
> {
> - return local_read(&bpage->page->commit);
> + return bpage->page->commit;
> }
>
> static inline unsigned long rb_page_entries(struct buffer_page *bpage)
> {
> - return local_read(&bpage->entries) & RB_WRITE_MASK;
> + return bpage->entries & RB_WRITE_MASK;
> }
>
> /* Size is determined by what has been commited */
> @@ -1463,8 +1463,8 @@ rb_set_commit_to_write(struct ring_buffe
> if (RB_WARN_ON(cpu_buffer,
> rb_is_reader_page(cpu_buffer->tail_page)))
> return;
> - local_set(&cpu_buffer->commit_page->page->commit,
> - rb_page_write(cpu_buffer->commit_page));
> + cpu_buffer->commit_page->page->commit =
> + rb_page_write(cpu_buffer->commit_page);
> rb_inc_page(cpu_buffer, &cpu_buffer->commit_page);
> cpu_buffer->write_stamp =
> cpu_buffer->commit_page->page->time_stamp;
> @@ -1474,10 +1474,10 @@ rb_set_commit_to_write(struct ring_buffe
> while (rb_commit_index(cpu_buffer) !=
> rb_page_write(cpu_buffer->commit_page)) {
>
> - local_set(&cpu_buffer->commit_page->page->commit,
> - rb_page_write(cpu_buffer->commit_page));
> + cpu_buffer->commit_page->page->commit =
> + rb_page_write(cpu_buffer->commit_page);
> RB_WARN_ON(cpu_buffer,
> - local_read(&cpu_buffer->commit_page->page->commit) &
> + cpu_buffer->commit_page->page->commit &
> ~RB_WRITE_MASK);
> barrier();
> }
> @@ -1600,7 +1600,7 @@ rb_handle_head_page(struct ring_buffer_p
> * it is our responsibility to update
> * the counters.
> */
> - local_add(entries, &cpu_buffer->overrun);
> + add_local(&cpu_buffer->overrun, entries);
>
> /*
> * The entries will be zeroed out when we move the
> @@ -1741,7 +1741,7 @@ rb_reset_tail(struct ring_buffer_per_cpu
> * must fill the old tail_page with padding.
> */
> if (tail >= BUF_PAGE_SIZE) {
> - local_sub(length, &tail_page->write);
> + add_local(&tail_page->write, -length);
[...]
If we can have inc/dec/sub already, that would be good, rather than
going with add -val. This would ensure that we don't do too much
ping-pong with the code using these primitives.
In the end, the fact that the missing volatile access bug crept up as
part of this patch makes me think that keeping local_t was doing a fine
encapsulation job. However, if we really want to go down the path of
removing this encapsulation, then we should:
- make sure that _all_ variable accesses are encapsulated, even
read_local and set_local.
- put all this API into a single header per architecture, easy for
people to find and understand, rather than multiple headers sprinkled
all over the place.
- document that accessing the variables without the API violates the
consistency guarantees.
Thanks,
Mathieu
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
Mathieu Desnoyers
xadd should only be used to implement add_local_return, not add_local.
add_local can be implemented with the "add" instruction, which is
significantly faster if my memory serves me correctly.
Thanks,
Mathieu
>
> Signed-off-by: Christoph Lameter <c...@linux-foundation.org>
>
> ---
> arch/x86/include/asm/add-local.h | 56 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 55 insertions(+), 1 deletion(-)
>
> Index: linux-2.6/arch/x86/include/asm/add-local.h
> ===================================================================
> --- linux-2.6.orig/arch/x86/include/asm/add-local.h 2010-01-05 15:29:11.000000000 -0600
> +++ linux-2.6/arch/x86/include/asm/add-local.h 2010-01-05 15:33:59.000000000 -0600
> @@ -1,2 +1,56 @@
> -#include <asm-generic/add-local.h>
> +#ifndef __ASM_X86_ADD_LOCAL_H
> +#define __ASM_X86_ADD_LOCAL_H
> +
> +#include <linux/types.h>
> +#include <asm-generic/add-local-generic.h>
> +
> +static inline unsigned long __add_return_local(volatile void *ptr,
> + unsigned long value, int size)
> +{
> + unsigned long r;
> +
> +#ifdef CONFIG_M386
> + if (unlikely(boot_cpu_data.x86 <= 3))
> + return __add_return_local_generic(ptr, value, size);
> +#endif
> +
> + /*
> + * Sanity checking, compile-time.
> + */
> + if (size == 8 && sizeof(unsigned long) != 8)
> + wrong_size_add_local(ptr);
> +
> + r = value;
> + switch (size) {
> + case 1:
> + asm volatile("xaddb %0, %1;": "+r" (r), "+m" (*((u8 *)ptr)):
> + : "memory");
> + break;
> + case 2:
> + asm volatile("xaddw %0, %1;": "+r" (r), "+m" (*((u16 *)ptr)):
> + : "memory");
> + break;
> + case 4:
> + asm volatile("xaddl %0, %1;": "+r" (r), "+m" (*((u32 *)ptr)):
> + : "memory");
> + break;
> + case 8:
> + asm volatile("xaddq %0, %1;": "+r" (r), "+m" (*((u64 *)ptr)):
> + : "memory");
> + break;
> + default:
> + wrong_size_add_local(ptr);
> + }
> + return r + value;
> +}
> +
> +#define add_return_local(ptr, v) \
> + ((__typeof__(*(ptr)))__add_return_local((ptr), (unsigned long)(v), \
> + sizeof(*(ptr))))
> +
> +#define add_local(ptr, v) (void)__add_return_local((ptr), (unsigned long)(v), \
> + sizeof(*(ptr)))
> +
> +
> +#endif
>
>
> --
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
Mike Frysinger
> On Tue, 5 Jan 2010, Mike Frysinger wrote:
>> On Tue, Jan 5, 2010 at 17:04, Christoph Lameter wrote:
>> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
>> > +++ linux-2.6/include/asm-generic/add-local.h 2010-01-05 15:36:02.000000000 -0600
>> > @@ -0,0 +1,13 @@
>> > +#ifndef __ASM_GENERIC_ADD_LOCAL_H
>> > +#define __ASM_GENERIC_ADD_LOCAL_H
>>
>> needs comment header (blurb/copyright/license)
>
> A simple small include file? Really?
if there's enough content to warrant protection against multiple
inclusion, then generally yes
>> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
>> > +++ linux-2.6/arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
>> > @@ -0,0 +1,2 @@
>> > +#include <asm-generic/add-local.h>
>> > +
>>
>> these arch stubs all have an extra new line
>
> Thats bad?
files shouldnt have trailing new lines
>> > +/*
>> > + * Generic version of __add_return_local (disables interrupts). Takes an
>> > + * unsigned long parameter, supporting various types of architectures.
>> > + */
>> > +static inline unsigned long __add_return_local_generic(volatile void *ptr,
>> > + unsigned long value, int size)
>>
>> size_t for size ?
>
> No. It can be anything.
you're passing it sizeof() which returns a size_t
>> > +extern unsigned long wrong_size_add_local(volatile void *ptr);
>> > ...
>> > + /*
>> > + * Sanity checking, compile-time.
>> > + */
>> > + if (size == 8 && sizeof(unsigned long) != 8)
>> > + wrong_size_add_local(ptr);
>> > ...
>> > + default:
>> > + wrong_size_add_local(ptr);
>> > + }
>>
>> should be BUILD_BUG_ON() ?
>
> Does not work there.
why not ? BUILD_BUG_ON() should work on any compile-time constant
which sizeof() is ... unless you plan on letting people call this
function with arbitrary sizes ?
Arnd Bergmann
>
> The problem I see here is that with ~5-6 operations, we will end up
> having 20*5 = 100 headers only for this. Can we combine these in a
> single header file instead ? local.h wasn't bad in this respect.
I have an old patch that I was planning to dig out for 2.6.34,
which autogenerates arch/*/include/foo.h files that only contain
"#include <asm-generic/foo.h>".
I guess this would be sufficient to avoid the overload with all
these header files.
> Also, separating all these in sub-files will make it a bit difficult to
> pinpoint errors that would arise from using a bad combination of, e.g.
> generic add with a non-protected read or set.
Yes.
> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/include/asm-generic/add-local-generic.h 2010-01-05 15:36:02.000000000 -0600
> > @@ -0,0 +1,40 @@
> > +#ifndef __ASM_GENERIC_ADD_LOCAL_GENERIC_H
> > +#define __ASM_GENERIC_ADD_LOCAL_GENERIC_H
Why split the file between asm-generic/add-local.h and add-local-generic.h?
I don't see how any architecture could use one but not the other.
> > +#include <linux/types.h>
> > +
> > +extern unsigned long wrong_size_add_local(volatile void *ptr);
> > +
> > +/*
> > + * Generic version of __add_return_local (disables interrupts). Takes an
> > + * unsigned long parameter, supporting various types of architectures.
> > + */
> > +static inline unsigned long __add_return_local_generic(volatile void *ptr,
> > + unsigned long value, int size)
You could probably lose the 'volatile' here, if you want to discourage
marking data as volatile in the code.
> > +{
> > + unsigned long flags, r;
> > +
> > + /*
> > + * Sanity checking, compile-time.
> > + */
> > + if (size == 8 && sizeof(unsigned long) != 8)
> > + wrong_size_add_local(ptr);
It can be BUILD_BUG_ON if you move it to the outer macro.
> > + local_irq_save(flags);
> > + switch (size) {
> > + case 1: r = (*((u8 *)ptr) += value);
> > + break;
> > + case 2: r = (*((u16 *)ptr) += value);
> > + break;
> > + case 4: r = (*((u32 *)ptr) += value);
> > + break;
> > + case 8: r = (*((u64 *)ptr) += value);
> > + break;
But I think here you actually need to add the volatile in order
to make these atomic assignments.
Arnd
Mathieu Desnoyers
> On Tuesday 05 January 2010, Mathieu Desnoyers wrote:
> >
> > The problem I see here is that with ~5-6 operations, we will end up
> > having 20*5 = 100 headers only for this. Can we combine these in a
> > single header file instead ? local.h wasn't bad in this respect.
>
> I have an old patch that I was planning to dig out for 2.6.34,
> which autogenerates arch/*/include/foo.h files that only contain
> "#include <asm-generic/foo.h>".
>
> I guess this would be sufficient to avoid the overload with all
> these header files.
Well, given we already have local.h, I am not completely sure that this
whole exercise is giving us.
[...]
Yes, you are right. If we ever try to access these variables from a
remote CPU with a load (but not with any concurrent store operation, as
this would be semantically invalid), then the volatile is important.
Mathieu
>
> Arnd
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
Arnd Bergmann
> * Arnd Bergmann (ar...@arndb.de) wrote:
> > > > + local_irq_save(flags);
> > > > + switch (size) {
> > > > + case 1: r = (*((u8 *)ptr) += value);
> > > > + break;
> > > > + case 2: r = (*((u16 *)ptr) += value);
> > > > + break;
> > > > + case 4: r = (*((u32 *)ptr) += value);
> > > > + break;
> > > > + case 8: r = (*((u64 *)ptr) += value);
> > > > + break;
> >
> > But I think here you actually need to add the volatile in order
> > to make these atomic assignments.
>
> Yes, you are right. If we ever try to access these variables from a
> remote CPU with a load (but not with any concurrent store operation, as
> this would be semantically invalid), then the volatile is important.
Just to make sure everyone has the same understanding: We need the volatile
in the cast in these lines, not the one in the function prototype which
only serves to avoid warnings but has no impact on the object code when
we cast the pointer to a non-volatile type for the assignment.
Arnd
Christoph Lameter
> The problem I see here is that with ~5-6 operations, we will end up
> having 20*5 = 100 headers only for this. Can we combine these in a
> single header file instead ? local.h wasn't bad in this respect.
We could actually keep local.h and just thin it out a bit. Get rid of the
local_t type and use long instead? Then make the local_inc/local_add work
on an arbitrary scalar like cmpxchg_local?
> Also, separating all these in sub-files will make it a bit difficult to
> pinpoint errors that would arise from using a bad combination of, e.g.
> generic add with a non-protected read or set.
Yes surely I dont want many files. I thought about adding
#define inc_local(x) add_local(x, 1)
etc to add-local.h
Christoph Lameter
> >> > +static inline unsigned long __add_return_local_generic(volatile void *ptr,
> >> > + unsigned long value, int size)
> >>
> >> size_t for size ?
> >
> > No. It can be anything.
>
> you're passing it sizeof() which returns a size_t
Ahh not value but the size parameter... Ok.
Christoph Lameter
> > Volatile is discouraged as far as I can tell.
>
> If you want to ensure that a simple variable assignment or read
> (local_set, local_read) are not performed piecewise by the compiler
> which can cause odd effects when shared with interrupt handlers, this
> will however be necessary.
Piecewise? Assignment of scalars or a pointer is an atomic operation by
default. Lots of things will break if that is not true.
Christoph Lameter
> add_local can be implemented with the "add" instruction, which is
> significantly faster if my memory serves me correctly.
Yes a full patchset would do that. Lets first see if we can come to an
agreement to go down this road before I put the effort in.
Christoph Lameter
> > static void rb_init_page(struct buffer_data_page *bpage)
> > {
> > - local_set(&bpage->commit, 0);
> > + bpage->commit = 0;
>
> This is incorrect. You are turning a "volatile" write into a
> non-volatile write, which can be turned into multiple writes by the
> compiler and therefore expose inconsistent state to interrupt handlers.
The structure is being setup and therefore not reachable by anyone?
Even if that is not the case: The assignment of a scalar is atomic.
> > static inline unsigned long rb_page_write(struct buffer_page *bpage)
> > {
> > - return local_read(&bpage->write) & RB_WRITE_MASK;
> > + return bpage->write & RB_WRITE_MASK;
>
> Same problem here: missing volatile for read. Same applies thorough the
> patch.
Reading of a scalar is atomic.
> > if (tail >= BUF_PAGE_SIZE) {
> > - local_sub(length, &tail_page->write);
> > + add_local(&tail_page->write, -length);
>
> [...]
>
> If we can have inc/dec/sub already, that would be good, rather than
> going with add -val. This would ensure that we don't do too much
> ping-pong with the code using these primitives.
ok.
> In the end, the fact that the missing volatile access bug crept up as
> part of this patch makes me think that keeping local_t was doing a fine
> encapsulation job. However, if we really want to go down the path of
> removing this encapsulation, then we should:
I am not sure that there is anything to be won by volatile.
> - make sure that _all_ variable accesses are encapsulated, even
> read_local and set_local.
> - put all this API into a single header per architecture, easy for
> people to find and understand, rather than multiple headers sprinkled
> all over the place.
> - document that accessing the variables without the API violates the
> consistency guarantees.
Then we better leave things as is. local.h would then become a private
operations set for ringbuffer operations?
I'd like to see local operations that are generically usable also in
other subsystems. Locall operations that work generically on scalars
(pointer, int, long etc) like cmpxchg_local.
The only new operation needed for ringbuffer.c is add_local().
Sugarcoating with inc/dec/sub can be easily added and add_local can be
modified to generate inc/dec if it discovers 1 or -1 being passed to it.
Christoph Lameter
Tracing uses local_t for per cpu safe atomic operations in the form
of cmpxchg and additions.
This patchset removes unused function in local.h and then genericizes
local.h by removing local_t. This results in a very small set of
functions.
"long" is used for now instead of local_t. With some additional work it
would be possible to pass arbitrary types to local_xx() function like
cmpxchg_local() and the this_cpu_xx() functions. Maybe a more flexible
way of handling local_xx() would allow the use of these functions in
other kernel subsystems.
Still RFC state. Lots of stuff todo. Compiles on my box.
V1->V2
- Preserve local.h
- Add a rationale why the remaining functions are useful and how
they differ from this_cpu_xx.
Arnd Bergmann
> -#define local_add_return(i, l) atomic_long_add_return((i), (&(l)->a))
> + local_irq_save(flags);
> + switch (size) {
> + case 1: r = (*((u8 *)ptr) += value);
> + break;
> + case 2: r = (*((u16 *)ptr) += value);
> + break;
> + case 4: r = (*((u32 *)ptr) += value);
> + break;
> + case 8: r = (*((u64 *)ptr) += value);
> + break;
> + local_irq_restore(flags);
> + return r;
> +}
This still misses 'volatile'.
Arnd
Steven Rostedt
On Tue, 2010-01-05 at 16:04 -0600, Christoph Lameter wrote:
> plain text document attachment (remove_local_t_ringbuffer_convert)
> Replace the local_t use with longs in the trace subsystem. The longs can then be
> updated with the required level of concurrency protection through cmpxchg_local()
> and add_local().
>
> Signed-off-by: Christoph Lameter <c...@linux-foundation.org>
>
> @@ -1741,7 +1741,7 @@ rb_reset_tail(struct ring_buffer_per_cpu
> * must fill the old tail_page with padding.
> */
> if (tail >= BUF_PAGE_SIZE) {
> - local_sub(length, &tail_page->write);
> + add_local(&tail_page->write, -length);
Can't you have a helper function or macro that does:
#define sub_local(local, val) add_local(local, -(val))
> return;
> }
>
> static struct ring_buffer_event *
> @@ -1801,7 +1801,7 @@ rb_move_tail(struct ring_buffer_per_cpu
> * about it.
> */
> if (unlikely(next_page == commit_page)) {
> - local_inc(&cpu_buffer->commit_overrun);
> + add_local(&cpu_buffer->commit_overrun, 1);
As well as a:
#define inc_local(local) add_local(local, 1)
> goto out_reset;
> }
>
> again:
> - commits = local_read(&cpu_buffer->commits);
> + commits = cpu_buffer->commits;
> /* synchronize with interrupts */
> barrier();
> - if (local_read(&cpu_buffer->committing) == 1)
> + if (cpu_buffer->committing == 1)
> rb_set_commit_to_write(cpu_buffer);
>
> - local_dec(&cpu_buffer->committing);
> + add_local(&cpu_buffer->committing, -1);
As well as a:
#define dec_local(local) add_local(local, -1)
>
> /* synchronize with interrupts */
> barrier();
> @@ -2059,9 +2059,9 @@ static void rb_end_commit(struct ring_bu
> * updating of the commit page and the clearing of the
> * committing counter.
> */
The reason I ask for the above is because it sticks out much better. The
subtracting and inc/dec pairs I need to match. So I usually end up
scanning a matching "dec" for a "inc", or a matching "sub" for an "add",
but having "-1" to match "1" is not going to stand out, and I envision a
lot of stupid bugs happening because of this.
-- Steve