Remove local_t

RFC Superseded Christoph Lameter 5 participants 24 messages Jan 5, 2010 – Jan 14, 2010

Subsystems (18)

Documentation Arm (32-bit) x86 PowerPC +14 more subsystems

lore

Share & tools

lore

Christoph Lameter

author

2010-01-05 22:05 UTC #0 lore raw

cover Remove local_t

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.

Christoph Lameter

author

2010-01-05 22:05 UTC #1 lore raw

RFC 1/4 Add add_local() and add_local_return()

We already have cmpxchg_local that can work on an arbitrary type. The cmpxchg 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 <[email protected]>

---
 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(+)

include/asm-generic/add-local.h new file +13 -0

Index: linux-2.6/include/asm-generic/add-local.h
===================================================================
@@ -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

arch/alpha/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/alpha/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/arm/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/arm/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/avr32/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/avr32/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/blackfin/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/blackfin/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/cris/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/cris/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/frv/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/frv/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/h8300/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/h8300/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/ia64/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/ia64/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/m32r/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/m32r/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/m68k/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/m68k/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/microblaze/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/microblaze/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/mips/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/mips/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/mn10300/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/mn10300/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/parisc/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/parisc/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/powerpc/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/powerpc/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/s390/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/s390/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/score/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/score/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/sh/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/sh/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/sparc/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/sparc/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/um/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/um/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/x86/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/x86/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

arch/xtensa/include/asm/add-local.h new file +2 -0

Index: linux-2.6/arch/xtensa/include/asm/add-local.h
===================================================================
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+

include/asm-generic/add-local-generic.h new file +40 -0

Index: linux-2.6/include/asm-generic/add-local-generic.h
===================================================================
@@ -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

Christoph Lameter

author

2010-01-05 22:30 UTC #3 lore raw

Re: [RFC local_t removal V1 1/4] Add add_local() and add_local_return()

Quoted 8 lines · Mike Frysinger

include/asm-generic/add-local.h 2010-01-05 15:36:02.000000000 -0600 line 2
Quoted 5 lines · Christoph Lameter
include/asm-generic/add-local.h 2010-01-05 15:36:02.000000000 -0600 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +2 -0
@@ -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?

Quoted 9 lines

include/asm-generic/add-local.h 2010-01-05 15:36:02.000000000 -0600 line 8
Quoted 6 lines
include/asm-generic/add-local.h 2010-01-05 15:36:02.000000000 -0600 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +6 -0
+#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.

Quoted 7 lines

arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600 line 2
Quoted 5 lines
arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
these arch stubs all have an extra new line

Thats bad?

Quoted 8 lines

arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
Quoted 6 lines
arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +6 -0
+/*
+ * 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.

Quoted 12 lines

Quoted 11 lines
+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);
+ }

Quoted 1 line

should be BUILD_BUG_ON() ?

Does not work there.

Mike Frysinger

2010-01-06 19:24 UTC #4 lore raw

Re: [RFC local_t removal V1 1/4] Add add_local() and add_local_return()

Quoted 11 lines · Christoph Lameter

Quoted 8 lines · Mike Frysinger
include/asm-generic/add-local.h 2010-01-05 15:36:02.000000000 -0600 line 2
Quoted 5 lines · Christoph Lameter
include/asm-generic/add-local.h 2010-01-05 15:36:02.000000000 -0600 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +2 -0
@@ -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

Quoted 9 lines

Quoted 7 lines
arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600 line 2
Quoted 5 lines
arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +2 -0
@@ -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

Quoted 10 lines

Quoted 8 lines
arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600
Quoted 6 lines
arch/alpha/include/asm/add-local.h 2010-01-05 15:36:02.000000000 -0600 renamed from dev/null 1970-01-01 00:00:00.000000000 +0000 +6 -0
+/*
+ * 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

Quoted 12 lines

Quoted 12 lines
Quoted 11 lines
+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);
+ }

Quoted 3 lines

Quoted 1 line
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 ?
-mike

Christoph Lameter

author

2010-01-07 17:04 UTC #5 lore raw

Re: [RFC local_t removal V1 1/4] Add add_local() and add_local_return()

Quoted 8 lines · Mike Frysinger

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
+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.

Mathieu Desnoyers

2010-01-05 22:49 UTC #6 lore raw

Re: [RFC local_t removal V1 1/4] Add add_local() and add_local_return()

In reply to Christoph Lameter

Quoted 26 lines · * Christoph Lameter ([email protected]) wrote:

We already have cmpxchg_local that can work on an arbitrary type. The cmpxchg 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 <[email protected]>

---
 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 +

Quoted 8 lines

 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

Quoted 220 lines

Index: linux-2.6/include/asm-generic/add-local.h
===================================================================
include/asm-generic/add-local.h new file +13 -0
@@ -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
===================================================================
arch/alpha/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/arm/include/asm/add-local.h
===================================================================
arch/arm/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/avr32/include/asm/add-local.h
===================================================================
arch/avr32/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/blackfin/include/asm/add-local.h
===================================================================
arch/blackfin/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/cris/include/asm/add-local.h
===================================================================
arch/cris/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/frv/include/asm/add-local.h
===================================================================
arch/frv/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/h8300/include/asm/add-local.h
===================================================================
arch/h8300/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/ia64/include/asm/add-local.h
===================================================================
arch/ia64/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/m32r/include/asm/add-local.h
===================================================================
arch/m32r/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/m68k/include/asm/add-local.h
===================================================================
arch/m68k/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/microblaze/include/asm/add-local.h
===================================================================
arch/microblaze/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/mips/include/asm/add-local.h
===================================================================
arch/mips/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/mn10300/include/asm/add-local.h
===================================================================
arch/mn10300/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/parisc/include/asm/add-local.h
===================================================================
arch/parisc/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/powerpc/include/asm/add-local.h
===================================================================
arch/powerpc/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/s390/include/asm/add-local.h
===================================================================
arch/s390/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/score/include/asm/add-local.h
===================================================================
arch/score/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/sh/include/asm/add-local.h
===================================================================
arch/sh/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/sparc/include/asm/add-local.h
===================================================================
arch/sparc/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/um/include/asm/add-local.h
===================================================================
arch/um/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/x86/include/asm/add-local.h
===================================================================
arch/x86/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/arch/xtensa/include/asm/add-local.h
===================================================================
arch/xtensa/include/asm/add-local.h new file +2 -0
@@ -0,0 +1,2 @@
+#include <asm-generic/add-local.h>
+
Index: linux-2.6/include/asm-generic/add-local-generic.h
===================================================================
include/asm-generic/add-local-generic.h new file +40 -0
@@ -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
Signature · 1 line
-- 

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Arnd Bergmann

maintainer

2010-01-07 13:46 UTC #7 lore raw

Re: [RFC local_t removal V1 1/4] Add add_local() and add_local_return()

Quoted 4 lines · Mathieu Desnoyers

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.

Quoted 3 lines

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.

Quoted 5 lines

include/asm-generic/add-local-generic.h line 2
Quoted 5 lines
include/asm-generic/add-local-generic.h new file +2 -0
@@ -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.

Quoted 10 lines

include/asm-generic/add-local-generic.h line 13

Quoted 10 lines

include/asm-generic/add-local-generic.h new file +10 -0

+#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.

Quoted 8 lines

include/asm-generic/add-local-generic.h line 21
Quoted 8 lines
include/asm-generic/add-local-generic.h new file +8 -0
+{
+	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.

Quoted 10 lines

include/asm-generic/add-local-generic.h line 32

Quoted 10 lines

include/asm-generic/add-local-generic.h new file +10 -0

+	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

2010-01-07 13:58 UTC #8 lore raw

Re: [RFC local_t removal V1 1/4] Add add_local() and add_local_return()

Quoted 6 lines · * Arnd Bergmann ([email protected]) wrote:

Quoted 4 lines · Mathieu Desnoyers
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.

Quoted 6 lines

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.

[...]

Quoted 25 lines

Quoted 10 lines
include/asm-generic/add-local-generic.h line 13
Quoted 10 lines
include/asm-generic/add-local-generic.h new file +10 -0
+#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.
Quoted 8 lines
include/asm-generic/add-local-generic.h line 21
Quoted 8 lines
include/asm-generic/add-local-generic.h new file +8 -0
+{
+	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.

Quoted 13 lines

Quoted 10 lines
include/asm-generic/add-local-generic.h line 32
Quoted 10 lines
include/asm-generic/add-local-generic.h new file +10 -0
+	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.

Mathieu

Quoted 2 lines

Arnd

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Arnd Bergmann

maintainer

2010-01-07 14:22 UTC #9 lore raw

Re: [RFC local_t removal V1 1/4] Add add_local() and add_local_return()

Quoted 12 lines · Mathieu Desnoyers

Quoted 11 lines · * Arnd Bergmann ([email protected]) wrote:
Quoted 10 lines
Quoted 10 lines
+ local_irq_save(flags);
+ switch (size) {
+ case 1: r = (*((u8 *)ptr) += value);
include/asm-generic/add-local-generic.h new file +7 -0
+         break;
+ case 2: r = (*((u16 *)ptr) += value);
+         break;
+ case 4: r = (*((u32 *)ptr) += value);
+         break;
+ case 8: r = (*((u64 *)ptr) += value);
+         break;

Quoted 6 lines

Quoted 2 lines
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

author

2010-01-07 17:08 UTC #10 lore raw

Re: [RFC local_t removal V1 1/4] Add add_local() and add_local_return()

In reply to Mathieu Desnoyers

Quoted 3 lines · Mathieu Desnoyers

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?

Quoted 3 lines

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

author

2010-01-05 22:06 UTC #11 lore raw

RFC 2/4 Replace local_t use in trace subsystem

In reply to Christoph Lameter

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 <[email protected]>

---
 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(-)

kernel/trace/ring_buffer.c +75 ~~-75~~

Index: linux-2.6/kernel/trace/ring_buffer.c
===================================================================
@@ -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;
 }
 
 /**

@@ -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;
 }
 
 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);
 		return;
 	}

@@ -1766,7 +1766,7 @@ rb_reset_tail(struct ring_buffer_per_cpu
 		rb_event_set_padding(event);
 
 		/* Set the write back to the previous setting */
-		local_sub(length, &tail_page->write);
+		add_local(&tail_page->write, -length);
 		return;
 	}

@@ -1778,7 +1778,7 @@ rb_reset_tail(struct ring_buffer_per_cpu
 
 	/* Set write to end of buffer */
 	length = (tail + length) - BUF_PAGE_SIZE;
-	local_sub(length, &tail_page->write);
+	add_local(&tail_page->write, -length);
 }
 
 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);
 		goto out_reset;
 	}

@@ -1855,7 +1855,7 @@ rb_move_tail(struct ring_buffer_per_cpu 
 				      cpu_buffer->tail_page) &&
 				     (cpu_buffer->commit_page ==
 				      cpu_buffer->reader_page))) {
-				local_inc(&cpu_buffer->commit_overrun);
+				add_local(&cpu_buffer->commit_overrun, 1);
 				goto out_reset;
 			}
 		}

@@ -1894,7 +1894,7 @@ __rb_reserve_next(struct ring_buffer_per
 	unsigned long tail, write;
 
 	tail_page = cpu_buffer->tail_page;
-	write = local_add_return(length, &tail_page->write);
+	write = add_return_local(&tail_page->write, length);
 
 	/* set write to only the index of the write */
 	write &= RB_WRITE_MASK;

@@ -1913,7 +1913,7 @@ __rb_reserve_next(struct ring_buffer_per
 
 	/* The passed in type is zero for DATA */
 	if (likely(!type))
-		local_inc(&tail_page->entries);
+		add_local(&tail_page->entries, 1);
 
 	/*
 	 * If this is the first commit on the page, then update

@@ -1943,7 +1943,7 @@ rb_try_to_discard(struct ring_buffer_per
 
 	if (bpage->page == (void *)addr && rb_page_write(bpage) == old_index) {
 		unsigned long write_mask =
-			local_read(&bpage->write) & ~RB_WRITE_MASK;
+			bpage->write & ~RB_WRITE_MASK;
 		/*
 		 * This is on the tail page. It is possible that
 		 * a write could come in and move the tail page

@@ -1952,7 +1952,7 @@ rb_try_to_discard(struct ring_buffer_per
 		 */
 		old_index += write_mask;
 		new_index += write_mask;
-		index = local_cmpxchg(&bpage->write, old_index, new_index);
+		index = cmpxchg_local(&bpage->write, old_index, new_index);
 		if (index == old_index)
 			return 1;
 	}

@@ -2030,8 +2030,8 @@ rb_add_time_stamp(struct ring_buffer_per
 
 static void rb_start_commit(struct ring_buffer_per_cpu *cpu_buffer)
 {
-	local_inc(&cpu_buffer->committing);
-	local_inc(&cpu_buffer->commits);
+	add_local(&cpu_buffer->committing, 1);
+	add_local(&cpu_buffer->commits, 1);
 }
 
 static void rb_end_commit(struct ring_buffer_per_cpu *cpu_buffer)

@@ -2039,17 +2039,17 @@ static void rb_end_commit(struct ring_bu
 	unsigned long commits;
 
 	if (RB_WARN_ON(cpu_buffer,
-		       !local_read(&cpu_buffer->committing)))
+		       !cpu_buffer->committing))
 		return;
 
  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);
 
 	/* 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.
 	 */
-	if (unlikely(local_read(&cpu_buffer->commits) != commits) &&
-	    !local_read(&cpu_buffer->committing)) {
-		local_inc(&cpu_buffer->committing);
+	if (unlikely(cpu_buffer->commits != commits) &&
+	    !cpu_buffer->committing) {
+		add_local(&cpu_buffer->committing, 1);
 		goto again;
 	}
 }

@@ -2087,8 +2087,8 @@ rb_reserve_next_event(struct ring_buffer
 	 */
 	barrier();
 	if (unlikely(ACCESS_ONCE(cpu_buffer->buffer) != buffer)) {
-		local_dec(&cpu_buffer->committing);
-		local_dec(&cpu_buffer->commits);
+		add_local(&cpu_buffer->committing, -1);
+		add_local(&cpu_buffer->commits, -1);
 		return NULL;
 	}
 #endif

@@ -2291,7 +2291,7 @@ rb_update_write_stamp(struct ring_buffer
 static void rb_commit(struct ring_buffer_per_cpu *cpu_buffer,
 		      struct ring_buffer_event *event)
 {
-	local_inc(&cpu_buffer->entries);
+	add_local(&cpu_buffer->entries, -1);
 	rb_update_write_stamp(cpu_buffer, event);
 	rb_end_commit(cpu_buffer);
 }

@@ -2357,7 +2357,7 @@ rb_decrement_entry(struct ring_buffer_pe
 
 	/* Do the likely case first */
 	if (likely(bpage->page == (void *)addr)) {
-		local_dec(&bpage->entries);
+		add_local(&bpage->entries, -1);
 		return;
 	}

@@ -2369,7 +2369,7 @@ rb_decrement_entry(struct ring_buffer_pe
 	start = bpage;
 	do {
 		if (bpage->page == (void *)addr) {
-			local_dec(&bpage->entries);
+			add_local(&bpage->entries, -1);
 			return;
 		}
 		rb_inc_page(cpu_buffer, &bpage);

@@ -2415,7 +2415,7 @@ void ring_buffer_discard_commit(struct r
 	 * committed yet. Thus we can assume that preemption
 	 * is still disabled.
 	 */
-	RB_WARN_ON(buffer, !local_read(&cpu_buffer->committing));
+	RB_WARN_ON(buffer, !cpu_buffer->committing);
 
 	rb_decrement_entry(cpu_buffer, event);
 	if (rb_try_to_discard(cpu_buffer, event))

@@ -2604,7 +2604,7 @@ unsigned long ring_buffer_entries_cpu(st
 		return 0;
 
 	cpu_buffer = buffer->buffers[cpu];
-	ret = (local_read(&cpu_buffer->entries) - local_read(&cpu_buffer->overrun))
+	ret = cpu_buffer->entries - cpu_buffer->overrun
 		- cpu_buffer->read;
 
 	return ret;

@@ -2625,7 +2625,7 @@ unsigned long ring_buffer_overrun_cpu(st
 		return 0;
 
 	cpu_buffer = buffer->buffers[cpu];
-	ret = local_read(&cpu_buffer->overrun);
+	ret = cpu_buffer->overrun;
 
 	return ret;
 }

@@ -2646,7 +2646,7 @@ ring_buffer_commit_overrun_cpu(struct ri
 		return 0;
 
 	cpu_buffer = buffer->buffers[cpu];
-	ret = local_read(&cpu_buffer->commit_overrun);
+	ret = cpu_buffer->commit_overrun;
 
 	return ret;
 }

@@ -2668,8 +2668,8 @@ unsigned long ring_buffer_entries(struct
 	/* if you care about this being correct, lock the buffer */
 	for_each_buffer_cpu(buffer, cpu) {
 		cpu_buffer = buffer->buffers[cpu];
-		entries += (local_read(&cpu_buffer->entries) -
-			    local_read(&cpu_buffer->overrun)) - cpu_buffer->read;
+		entries += cpu_buffer->entries -
+			    cpu_buffer->overrun - cpu_buffer->read;
 	}
 
 	return entries;

@@ -2692,7 +2692,7 @@ unsigned long ring_buffer_overruns(struc
 	/* if you care about this being correct, lock the buffer */
 	for_each_buffer_cpu(buffer, cpu) {
 		cpu_buffer = buffer->buffers[cpu];
-		overruns += local_read(&cpu_buffer->overrun);
+		overruns += cpu_buffer->overrun;
 	}
 
 	return overruns;

@@ -2861,9 +2861,9 @@ rb_get_reader_page(struct ring_buffer_pe
 	/*
 	 * Reset the reader page to size zero.
 	 */
-	local_set(&cpu_buffer->reader_page->write, 0);
-	local_set(&cpu_buffer->reader_page->entries, 0);
-	local_set(&cpu_buffer->reader_page->page->commit, 0);
+	cpu_buffer->reader_page->write = 0;
+	cpu_buffer->reader_page->entries = 0;
+	cpu_buffer->reader_page->page->commit = 0;
 
  spin:
 	/*

@@ -3354,9 +3354,9 @@ rb_reset_cpu(struct ring_buffer_per_cpu 
 
 	cpu_buffer->head_page
 		= list_entry(cpu_buffer->pages, struct buffer_page, list);
-	local_set(&cpu_buffer->head_page->write, 0);
-	local_set(&cpu_buffer->head_page->entries, 0);
-	local_set(&cpu_buffer->head_page->page->commit, 0);
+	cpu_buffer->head_page->write = 0;
+	cpu_buffer->head_page->entries = 0;
+	cpu_buffer->head_page->page->commit = 0;
 
 	cpu_buffer->head_page->read = 0;

@@ -3364,16 +3364,16 @@ rb_reset_cpu(struct ring_buffer_per_cpu 
 	cpu_buffer->commit_page = cpu_buffer->head_page;
 
 	INIT_LIST_HEAD(&cpu_buffer->reader_page->list);
-	local_set(&cpu_buffer->reader_page->write, 0);
-	local_set(&cpu_buffer->reader_page->entries, 0);
-	local_set(&cpu_buffer->reader_page->page->commit, 0);
+	cpu_buffer->reader_page->write = 0;
+	cpu_buffer->reader_page->entries = 0;
+	cpu_buffer->reader_page->page->commit = 0;
 	cpu_buffer->reader_page->read = 0;
 
-	local_set(&cpu_buffer->commit_overrun, 0);
-	local_set(&cpu_buffer->overrun, 0);
-	local_set(&cpu_buffer->entries, 0);
-	local_set(&cpu_buffer->committing, 0);
-	local_set(&cpu_buffer->commits, 0);
+	cpu_buffer->commit_overrun = 0;
+	cpu_buffer->overrun = 0;
+	cpu_buffer->entries = 0;
+	cpu_buffer->committing = 0;
+	cpu_buffer->commits = 0;
 	cpu_buffer->read = 0;
 
 	cpu_buffer->write_stamp = 0;

@@ -3399,7 +3399,7 @@ void ring_buffer_reset_cpu(struct ring_b
 
 	spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
 
-	if (RB_WARN_ON(cpu_buffer, local_read(&cpu_buffer->committing)))
+	if (RB_WARN_ON(cpu_buffer, cpu_buffer->committing))
 		goto out;
 
 	arch_spin_lock(&cpu_buffer->lock);

@@ -3547,9 +3547,9 @@ int ring_buffer_swap_cpu(struct ring_buf
 	atomic_inc(&cpu_buffer_b->record_disabled);
 
 	ret = -EBUSY;
-	if (local_read(&cpu_buffer_a->committing))
+	if (cpu_buffer_a->committing)
 		goto out_dec;
-	if (local_read(&cpu_buffer_b->committing))
+	if (cpu_buffer_b->committing)
 		goto out_dec;
 
 	buffer_a->buffers[cpu] = cpu_buffer_b;

@@ -3733,7 +3733,7 @@ int ring_buffer_read_page(struct ring_bu
 		} while (len > size);
 
 		/* update bpage */
-		local_set(&bpage->commit, pos);
+		bpage->commit = pos;
 		bpage->time_stamp = save_timestamp;
 
 		/* we copied everything to the beginning */

@@ -3746,8 +3746,8 @@ int ring_buffer_read_page(struct ring_bu
 		rb_init_page(bpage);
 		bpage = reader->page;
 		reader->page = *data_page;
-		local_set(&reader->write, 0);
-		local_set(&reader->entries, 0);
+		reader->write = 0;
+		reader->entries = 0;
 		reader->read = 0;
 		*data_page = bpage;
 	}

kernel/trace/ring_buffer_benchmark.c +2 -3

Index: linux-2.6/kernel/trace/ring_buffer_benchmark.c
===================================================================
@@ -8,11 +8,10 @@
 #include <linux/kthread.h>
 #include <linux/module.h>
 #include <linux/time.h>
-#include <asm/local.h>
 
 struct rb_page {
 	u64		ts;
-	local_t		commit;
+	long		commit;
 	char		data[4080];
 };

@@ -113,7 +112,7 @@ static enum event_status read_page(int c
 	ret = ring_buffer_read_page(buffer, &bpage, PAGE_SIZE, cpu, 1);
 	if (ret >= 0) {
 		rpage = bpage;
-		commit = local_read(&rpage->commit);
+		commit = rpage->commit;
 		for (i = 0; i < commit && !kill_test; i += inc) {
 
 			if (i >= (PAGE_SIZE - offsetof(struct rb_page, data))) {

kernel/trace/trace_branch.c +0 -1

Index: linux-2.6/kernel/trace/trace_branch.c
===================================================================
@@ -13,7 +13,6 @@
 #include <linux/ftrace.h>
 #include <linux/hash.h>
 #include <linux/fs.h>
-#include <asm/local.h>
 
 #include "trace.h"
 #include "trace_stat.h"

Mathieu Desnoyers

2010-01-05 22:57 UTC #12 lore raw

Re: [RFC local_t removal V1 2/4] Replace local_t use in trace subsystem

Quoted 47 lines · * Christoph Lameter ([email protected]) wrote:

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 <[email protected]>

---
 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
===================================================================

kernel/trace/ring_buffer.c +4 -4

@@ -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 */
 };

kernel/trace/ring_buffer.c struct buffer_page { · line 352

Quoted 5 lines

kernel/trace/ring_buffer.c +1 -1

@@ -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.

Quoted 60 lines

kernel/trace/ring_buffer.c +11 ~~-11~~

 }
 
 /**

@@ -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);

kernel/trace/ring_buffer.c rb_iter_head_event(struct ring_buffer_it · line 1397

Quoted 5 lines

kernel/trace/ring_buffer.c +1 -1

@@ -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.

Quoted 50 lines

kernel/trace/ring_buffer.c +8 -8

 }
 
 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

kernel/trace/ring_buffer.c rb_reset_tail(struct ring_buffer_per_cpu · line 1744

Quoted 5 lines

kernel/trace/ring_buffer.c +1 -1

@@ -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

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Christoph Lameter

author

2010-01-07 17:16 UTC #13 lore raw

Re: [RFC local_t removal V1 2/4] Replace local_t use in trace subsystem

Quoted 8 lines · Mathieu Desnoyers

kernel/trace/ring_buffer.c struct buffer_page { · line 352
Quoted 4 lines
static void rb_init_page(struct buffer_data_page *bpage) {
kernel/trace/ring_buffer.c +1 -1
-	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.

Quoted 7 lines

kernel/trace/ring_buffer.c rb_iter_head_event(struct ring_buffer_it · line 1397
Quoted 4 lines
static inline unsigned long rb_page_write(struct buffer_page *bpage) {
kernel/trace/ring_buffer.c +1 -1
-	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.

Quoted 9 lines

kernel/trace/ring_buffer.c rb_reset_tail(struct ring_buffer_per_cpu · line 1744
Quoted 3 lines
kernel/trace/ring_buffer.c +1 -1
 	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.

Quoted 4 lines

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.

Quoted 7 lines

- 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.

Steven Rostedt

maintainer

2010-01-14 02:56 UTC #14 lore raw

Re: [RFC local_t removal V1 2/4] Replace local_t use in trace subsystem

In reply to Christoph Lameter

Please Cc me on changes to the ring buffer.

Quoted 10 lines · Christoph Lameter

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 <[email protected]>

kernel/trace/ring_buffer.c rb_reset_tail(struct ring_buffer_per_cpu · line 1744

Quoted 5 lines

kernel/trace/ring_buffer.c +1 -1

@@ -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))

kernel/trace/ring_buffer.c rb_move_tail(struct ring_buffer_per_cpu · line 1804

Quoted 12 lines

(quoted diff) +0 -0

 		return;
 	}

static struct ring_buffer_event *

kernel/trace/ring_buffer.c +1 -1

@@ -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)

Quoted 6 lines

kernel/trace/ring_buffer.c +0 -0
 		goto out_reset;
 	}
 
again:

kernel/trace/ring_buffer.c static void rb_end_commit(struct ring_bu · line 2052

Quoted 10 lines

kernel/trace/ring_buffer.c +3 -3

-	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)

kernel/trace/ring_buffer.c static void rb_end_commit(struct ring_bu · line 2061

Quoted 7 lines

kernel/trace/ring_buffer.c +0 -0

 	/* 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

Christoph Lameter

author

2010-01-14 14:54 UTC #15 lore raw

Re: [RFC local_t removal V1 2/4] Replace local_t use in trace subsystem

Please have a look at V2 which has these changes.

Christoph Lameter

author

2010-01-05 22:05 UTC #16 lore raw

RFC 3/4 Optimized add_local()

In reply to Christoph Lameter

Use XADD to implement add_local().

Signed-off-by Christoph Lameter <[email protected]>

---
 arch/x86/include/asm/add-local.h |   56 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

arch/x86/include/asm/add-local.h +55 -1

Index: linux-2.6/arch/x86/include/asm/add-local.h
===================================================================
@@ -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

2010-01-05 23:04 UTC #17 lore raw

Re: [RFC local_t removal V1 3/4] Optimized add_local()

Quoted 1 line · * Christoph Lameter ([email protected]) wrote:

Use XADD to implement add_local().

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

Quoted 71 lines

Signed-off-by Christoph Lameter <[email protected]>

---
 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
===================================================================

arch/x86/include/asm/add-local.h +55 -1

@@ -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

Signature · 1 line

--

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Christoph Lameter

author

2010-01-07 17:17 UTC #18 lore raw

Re: [RFC local_t removal V1 3/4] Optimized add_local()

Quoted 2 lines · Mathieu Desnoyers

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

author

2010-01-05 22:06 UTC #19 lore raw

RFC 4/4 Remove local_t support

In reply to Christoph Lameter

No user of local_t remains.

Signed-off-by Christoph Lameter <[email protected]>

---
 Documentation/DocBook/kernel-locking.tmpl |    8 
 Documentation/atomic_ops.txt              |    7 
 Documentation/local_ops.txt               |  186 ----------------
 arch/alpha/include/asm/local.h            |  101 --------
 arch/arm/include/asm/local.h              |    1 
 arch/avr32/include/asm/local.h            |    6 
 arch/blackfin/include/asm/local.h         |    1 
 arch/cris/include/asm/local.h             |    1 
 arch/frv/include/asm/local.h              |    6 
 arch/frv/kernel/local.h                   |   59 -----
 arch/h8300/include/asm/local.h            |    6 
 arch/ia64/include/asm/local.h             |    1 
 arch/m32r/include/asm/local.h             |  341 ------------------------------
 arch/m68k/include/asm/local.h             |    6 
 arch/microblaze/include/asm/local.h       |    1 
 arch/mips/include/asm/local.h             |  196 -----------------
 arch/mn10300/include/asm/local.h          |    1 
 arch/parisc/include/asm/local.h           |    1 
 arch/powerpc/include/asm/local.h          |  175 ---------------
 arch/s390/include/asm/local.h             |    1 
 arch/score/include/asm/local.h            |    6 
 arch/sh/include/asm/local.h               |    7 
 arch/sparc/include/asm/local.h            |    6 
 arch/x86/include/asm/local.h              |  198 -----------------
 arch/xtensa/include/asm/local.h           |   16 -
 include/asm-generic/local.h               |   55 ----
 26 files changed, 4 insertions(+), 1389 deletions(-)

arch/frv/kernel/local.h deleted +0 ~~-59~~

Index: linux-2.6/arch/frv/kernel/local.h
===================================================================
@@ -1,59 +0,0 @@
-/* local.h: local definitions
- *
- * Copyright (C) 2004 Red Hat, Inc. All Rights Reserved.
- * Written by David Howells ([email protected])
- *
- * 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.
- */
-
-#ifndef _FRV_LOCAL_H
-#define _FRV_LOCAL_H
-
-#include <asm/sections.h>
-
-#ifndef __ASSEMBLY__
-
-/* dma.c */
-extern unsigned long frv_dma_inprogress;
-
-extern void frv_dma_pause_all(void);
-extern void frv_dma_resume_all(void);
-
-/* sleep.S */
-extern asmlinkage void frv_cpu_suspend(unsigned long);
-extern asmlinkage void frv_cpu_core_sleep(void);
-
-/* setup.c */
-extern unsigned long __nongprelbss pdm_suspend_mode;
-extern void determine_clocks(int verbose);
-extern int __nongprelbss clock_p0_current;
-extern int __nongprelbss clock_cm_current;
-extern int __nongprelbss clock_cmode_current;
-
-#ifdef CONFIG_PM
-extern int __nongprelbss clock_cmodes_permitted;
-extern unsigned long __nongprelbss clock_bits_settable;
-#define CLOCK_BIT_CM		0x0000000f
-#define CLOCK_BIT_CM_H		0x00000001	/* CLKC.CM can be set to 0 */
-#define CLOCK_BIT_CM_M		0x00000002	/* CLKC.CM can be set to 1 */
-#define CLOCK_BIT_CM_L		0x00000004	/* CLKC.CM can be set to 2 */
-#define CLOCK_BIT_P0		0x00000010	/* CLKC.P0 can be changed */
-#define CLOCK_BIT_CMODE		0x00000020	/* CLKC.CMODE can be changed */
-
-extern void (*__power_switch_wake_setup)(void);
-extern int  (*__power_switch_wake_check)(void);
-extern void (*__power_switch_wake_cleanup)(void);
-#endif
-
-/* time.c */
-extern void time_divisor_init(void);
-
-/* cmode.S */
-extern asmlinkage void frv_change_cmode(int);
-
-
-#endif /* __ASSEMBLY__ */
-#endif /* _FRV_LOCAL_H */

arch/alpha/include/asm/local.h deleted +0 ~~-101~~

101 lines not on this page · raw message

arch/arm/include/asm/local.h deleted +0 -1

Index: linux-2.6/arch/arm/include/asm/local.h
===================================================================
@@ -1 +0,0 @@
-#include <asm-generic/local.h>

arch/avr32/include/asm/local.h deleted +0 -6

Index: linux-2.6/arch/avr32/include/asm/local.h
===================================================================
@@ -1,6 +0,0 @@
-#ifndef __ASM_AVR32_LOCAL_H
-#define __ASM_AVR32_LOCAL_H
-
-#include <asm-generic/local.h>
-
-#endif /* __ASM_AVR32_LOCAL_H */

arch/blackfin/include/asm/local.h deleted +0 -1

Index: linux-2.6/arch/blackfin/include/asm/local.h
===================================================================
@@ -1 +0,0 @@
-#include <asm-generic/local.h>

arch/cris/include/asm/local.h deleted +0 -1

Index: linux-2.6/arch/cris/include/asm/local.h
===================================================================
@@ -1 +0,0 @@
-#include <asm-generic/local.h>

arch/frv/include/asm/local.h deleted +0 -6

Index: linux-2.6/arch/frv/include/asm/local.h
===================================================================
@@ -1,6 +0,0 @@
-#ifndef _ASM_LOCAL_H
-#define _ASM_LOCAL_H
-
-#include <asm-generic/local.h>
-
-#endif /* _ASM_LOCAL_H */

arch/h8300/include/asm/local.h deleted +0 -6

Index: linux-2.6/arch/h8300/include/asm/local.h
===================================================================
@@ -1,6 +0,0 @@
-#ifndef _H8300_LOCAL_H_
-#define _H8300_LOCAL_H_
-
-#include <asm-generic/local.h>
-
-#endif

arch/ia64/include/asm/local.h deleted +0 -1

Index: linux-2.6/arch/ia64/include/asm/local.h
===================================================================
@@ -1 +0,0 @@
-#include <asm-generic/local.h>

arch/m32r/include/asm/local.h deleted +0 ~~-341~~

341 lines not on this page · raw message

arch/m68k/include/asm/local.h deleted +0 -6

Index: linux-2.6/arch/m68k/include/asm/local.h
===================================================================
@@ -1,6 +0,0 @@
-#ifndef _ASM_M68K_LOCAL_H
-#define _ASM_M68K_LOCAL_H
-
-#include <asm-generic/local.h>
-
-#endif /* _ASM_M68K_LOCAL_H */

arch/microblaze/include/asm/local.h deleted +0 -1

Index: linux-2.6/arch/microblaze/include/asm/local.h
===================================================================
@@ -1 +0,0 @@
-#include <asm-generic/local.h>

arch/mips/include/asm/local.h deleted +0 ~~-196~~

196 lines not on this page · raw message

arch/mn10300/include/asm/local.h deleted +0 -1

Index: linux-2.6/arch/mn10300/include/asm/local.h
===================================================================
@@ -1 +0,0 @@
-#include <asm-generic/local.h>

arch/parisc/include/asm/local.h deleted +0 -1

Index: linux-2.6/arch/parisc/include/asm/local.h
===================================================================
@@ -1 +0,0 @@
-#include <asm-generic/local.h>

arch/powerpc/include/asm/local.h deleted +0 ~~-175~~

175 lines not on this page · raw message

arch/s390/include/asm/local.h deleted +0 -1

Index: linux-2.6/arch/s390/include/asm/local.h
===================================================================
@@ -1 +0,0 @@
-#include <asm-generic/local.h>

arch/score/include/asm/local.h deleted +0 -6

Index: linux-2.6/arch/score/include/asm/local.h
===================================================================
@@ -1,6 +0,0 @@
-#ifndef _ASM_SCORE_LOCAL_H
-#define _ASM_SCORE_LOCAL_H
-
-#include <asm-generic/local.h>
-
-#endif /* _ASM_SCORE_LOCAL_H */

arch/sh/include/asm/local.h deleted +0 -7

Index: linux-2.6/arch/sh/include/asm/local.h
===================================================================
@@ -1,7 +0,0 @@
-#ifndef __ASM_SH_LOCAL_H
-#define __ASM_SH_LOCAL_H
-
-#include <asm-generic/local.h>
-
-#endif /* __ASM_SH_LOCAL_H */
-

arch/sparc/include/asm/local.h deleted +0 -6

Index: linux-2.6/arch/sparc/include/asm/local.h
===================================================================
@@ -1,6 +0,0 @@
-#ifndef _SPARC_LOCAL_H
-#define _SPARC_LOCAL_H
-
-#include <asm-generic/local.h>
-
-#endif

arch/x86/include/asm/local.h deleted +0 ~~-198~~

198 lines not on this page · raw message

arch/xtensa/include/asm/local.h deleted +0 ~~-16~~

Index: linux-2.6/arch/xtensa/include/asm/local.h
===================================================================
@@ -1,16 +0,0 @@
-/*
- * include/asm-xtensa/local.h
- *
- * This file is subject to the terms and conditions of the GNU General Public
- * License.  See the file "COPYING" in the main directory of this archive
- * for more details.
- *
- * Copyright (C) 2001 - 2005 Tensilica Inc.
- */
-
-#ifndef _XTENSA_LOCAL_H
-#define _XTENSA_LOCAL_H
-
-#include <asm-generic/local.h>
-
-#endif /* _XTENSA_LOCAL_H */

Documentation/atomic_ops.txt +4 -3

Index: linux-2.6/Documentation/atomic_ops.txt
===================================================================
@@ -17,9 +17,10 @@ suffice:
 Historically, counter has been declared volatile.  This is now discouraged.
 See Documentation/volatile-considered-harmful.txt for the complete rationale.
 
-local_t is very similar to atomic_t. If the counter is per CPU and only
-updated by one CPU, local_t is probably more appropriate. Please see
-Documentation/local_ops.txt for the semantics of local_t.
+this_cpu operations are very similar to atomic_t. If the counter is per CPU
+and only updated by one CPU, then the use of the per cpu allocator and
+this cpu operations is more appropriate. Please see include/linux/percpu.h
+for details.
 
 The first operations to implement for atomic_t's are the initializers and
 plain reads.

Documentation/local_ops.txt deleted +0 ~~-186~~

186 lines not on this page · raw message

Documentation/DocBook/kernel-locking.tmpl +0 -8

Index: linux-2.6/Documentation/DocBook/kernel-locking.tmpl
===================================================================
@@ -1810,14 +1810,6 @@ machines due to caching.
     </para>
 
     <para>
-      Of particular use for simple per-cpu counters is the
-      <type>local_t</type> type, and the
-      <function>cpu_local_inc()</function> and related functions,
-      which are more efficient than simple code on some architectures
-      (<filename class="headerfile">include/asm/local.h</filename>).
-    </para>
-
-    <para>
       Note that there is no simple, reliable way of getting an exact
       value of such a counter, without introducing more locks.  This
       is not a problem for some uses.

include/asm-generic/local.h deleted +0 ~~-55~~

55 lines not on this page · raw message

Mathieu Desnoyers

2010-01-05 22:23 UTC #20 lore raw

In reply to Christoph Lameter

Quoted 10 lines · * Christoph Lameter ([email protected]) wrote:

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.

Quoted 6 lines

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

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Christoph Lameter

author

2010-01-05 22:35 UTC #21 lore raw

Quoted 3 lines · 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.

Quoted 2 lines

- 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.

Quoted 9 lines

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().

Quoted 6 lines

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.

Quoted 3 lines

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.

Mathieu Desnoyers

2010-01-05 22:45 UTC #22 lore raw

Quoted 7 lines · * Christoph Lameter ([email protected]) wrote:

Quoted 3 lines · 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.

Quoted 6 lines

Quoted 3 lines
- 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.

Quoted 25 lines

Quoted 10 lines
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().
Quoted 6 lines
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.

Quoted 8 lines

AS I said this is an RFC. I can easily generate all these things from the existing local.hs for the architectures.
Quoted 3 lines
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.

OK :)

Mathieu

Quoted 2 lines

Signature · 2 lines

Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Christoph Lameter

author

2010-01-07 17:06 UTC #23 lore raw

Quoted 6 lines · Mathieu Desnoyers

Quoted 1 line
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.