jump label: update for .39

 arch/mips/include/asm/jump_label.h  |   22 +-
 arch/mips/kernel/jump_label.c       |    2 +-
 arch/sparc/include/asm/jump_label.h |   25 +-
 arch/x86/include/asm/alternative.h  |    3 +-
 arch/x86/include/asm/jump_label.h   |   26 +-
 arch/x86/kernel/alternative.c       |    2 +-
 arch/x86/kernel/jump_label.c        |    2 +-
 arch/x86/kernel/module.c            |    1 +
 include/asm-generic/vmlinux.lds.h   |   14 +-
 include/linux/dynamic_debug.h       |    2 -
 include/linux/jump_label.h          |   86 ++++---
 include/linux/jump_label_ref.h      |   44 ---
 include/linux/perf_event.h          |   26 +-
 include/linux/tracepoint.h          |   22 +-
 kernel/jump_label.c                 |  537 ++++++++++++++---------------------
 kernel/perf_event.c                 |    4 +-
 kernel/tracepoint.c                 |   23 +-
 17 files changed, 352 insertions(+), 489 deletions(-)
 delete mode 100644 include/linux/jump_label_ref.h

arch/sparc/include/asm/jump_label.h +7 -11

index 427d468..fc73a82 100644
@@ -7,17 +7,20 @@
 
 #define JUMP_LABEL_NOP_SIZE 4
 
-#define JUMP_LABEL(key, label)					\
-	do {							\
-		asm goto("1:\n\t"				\
-			 "nop\n\t"				\
-			 "nop\n\t"				\
-			 ".pushsection __jump_table,  \"a\"\n\t"\
-			 ".align 4\n\t"				\
-			 ".word 1b, %l[" #label "], %c0\n\t"	\
-			 ".popsection \n\t"			\
-			 : :  "i" (key) :  : label);\
-	} while (0)
+static __always_inline bool arch_static_branch(struct jump_label_key *key)
+{
+		asm goto("1:\n\t"
+			 "nop\n\t"
+			 "nop\n\t"
+			 ".pushsection __jump_table,  \"aw\"\n\t"
+			 ".align 4\n\t"

 arch/mips/include/asm/jump_label.h  |   22 +-
 arch/mips/kernel/jump_label.c       |    2 +-
 arch/sparc/include/asm/jump_label.h |   25 +-
 arch/x86/include/asm/alternative.h  |    3 +-
 arch/x86/include/asm/jump_label.h   |   26 +-
 arch/x86/kernel/alternative.c       |    2 +-
 arch/x86/kernel/jump_label.c        |    2 +-
 arch/x86/kernel/module.c            |    1 +
 include/asm-generic/vmlinux.lds.h   |   14 +-
 include/linux/dynamic_debug.h       |    2 -
 include/linux/jump_label.h          |   86 ++++---
 include/linux/jump_label_ref.h      |   44 ---
 include/linux/perf_event.h          |   26 +-
 include/linux/tracepoint.h          |   22 +-
 kernel/jump_label.c                 |  537 ++++++++++++++---------------------
 kernel/perf_event.c                 |    4 +-
 kernel/tracepoint.c                 |   23 +-
 17 files changed, 352 insertions(+), 489 deletions(-)
 delete mode 100644 include/linux/jump_label_ref.h

arch/mips/include/asm/jump_label.h +12 -10

index 7622ccf..1881b31 100644
@@ -20,16 +20,18 @@
 #define WORD_INSN ".word"
 #endif
 
-#define JUMP_LABEL(key, label)						\
-	do {								\
-		asm goto("1:\tnop\n\t"					\
-			"nop\n\t"					\
-			".pushsection __jump_table,  \"a\"\n\t"		\
-			WORD_INSN " 1b, %l[" #label "], %0\n\t"		\
-			".popsection\n\t"				\
-			: :  "i" (key) :  : label);			\
-	} while (0)
-
+static __always_inline bool arch_static_branch(struct jump_label_key *key)
+{
+	asm goto("1:\tnop\n\t"
+		"nop\n\t"
+		".pushsection __jump_table,  \"aw\"\n\t"
+		WORD_INSN " 1b, %l[l_yes], %0\n\t"
+		".popsection\n\t"
+		: :  "i" (key) : : l_yes);
+	return false;
+l_yes:
+	return true;
+}
 
 #endif /* __KERNEL__ */
 

arch/mips/kernel/jump_label.c +0 -0

index 6001610..09ac7ca 100644
@@ -6,11 +6,11 @@
  * Copyright (c) 2010 Cavium Networks, Inc.
  */

Quoted 69 lines · Jason Baron

 arch/mips/include/asm/jump_label.h  |   22 +-
 arch/mips/kernel/jump_label.c       |    2 +-
 arch/sparc/include/asm/jump_label.h |   25 +-
 arch/x86/include/asm/alternative.h  |    3 +-
 arch/x86/include/asm/jump_label.h   |   26 +-
 arch/x86/kernel/alternative.c       |    2 +-
 arch/x86/kernel/jump_label.c        |    2 +-
 arch/x86/kernel/module.c            |    1 +
 include/asm-generic/vmlinux.lds.h   |   14 +-
 include/linux/dynamic_debug.h       |    2 -
 include/linux/jump_label.h          |   86 ++++---
 include/linux/jump_label_ref.h      |   44 ---
 include/linux/perf_event.h          |   26 +-
 include/linux/tracepoint.h          |   22 +-
 kernel/jump_label.c                 |  537 ++++++++++++++---------------------
 kernel/perf_event.c                 |    4 +-
 kernel/tracepoint.c                 |   23 +-
 17 files changed, 352 insertions(+), 489 deletions(-)
 delete mode 100644 include/linux/jump_label_ref.h

arch/mips/include/asm/jump_label.h +12 -10

index 7622ccf..1881b31 100644
@@ -20,16 +20,18 @@
 #define WORD_INSN ".word"
 #endif
 
-#define JUMP_LABEL(key, label)						\
-	do {								\
-		asm goto("1:\tnop\n\t"					\
-			"nop\n\t"					\
-			".pushsection __jump_table,  \"a\"\n\t"		\
-			WORD_INSN " 1b, %l[" #label "], %0\n\t"		\
-			".popsection\n\t"				\
-			: :  "i" (key) :  : label);			\
-	} while (0)
-
+static __always_inline bool arch_static_branch(struct jump_label_key *key)
+{
+	asm goto("1:\tnop\n\t"
+		"nop\n\t"
+		".pushsection __jump_table,  \"aw\"\n\t"
+		WORD_INSN " 1b, %l[l_yes], %0\n\t"
+		".popsection\n\t"
+		: :  "i" (key) : : l_yes);
+	return false;
+l_yes:
+	return true;
+}
 
 #endif /* __KERNEL__ */
 

arch/mips/kernel/jump_label.c +1 -1

index 6001610..09ac7ca 100644
@@ -6,11 +6,11 @@
  * Copyright (c) 2010 Cavium Networks, Inc.
  */
 
-#include <linux/jump_label.h>
 #include <linux/kernel.h>
 #include <linux/memory.h>
 #include <linux/mutex.h>
 #include <linux/types.h>
+#include <linux/jump_label.h>
 #include <linux/cpu.h>
 

arch/mips/kernel/jump_label.c line 16

Quoted 1 line

arch/mips/kernel/jump_label.c +0 -0

 #include <asm/cacheflush.h>

Hi Ralf,

Can I get your Acked-by on this patch.

I'm happy with the change to arch/mips/include/asm/jump_label.h and assuming that the change to arch/mips/kernel/jump_label.c will be dropped

Acked-by Ralf Baechle <[email protected]>

Quoted 19 lines · Jason Baron

arch/x86/kernel/jump_label.c +1 -1

index 961b6b3..dfa4c3c 100644
@@ -4,13 +4,13 @@
  * Copyright (C) 2009 Jason Baron <[email protected]>
  *
  */
-#include <linux/jump_label.h>
 #include <linux/memory.h>
 #include <linux/uaccess.h>
 #include <linux/module.h>
 #include <linux/list.h>
 #include <linux/jhash.h>
 #include <linux/cpu.h>
+#include <linux/jump_label.h>
 #include <asm/kprobes.h>
 #include <asm/alternative.h>
 

arch/x86/kernel/module.c

Quoted 1 line

arch/x86/kernel/module.c +0 -0

Jason,

I'm curious, is there a reason for this header change? Clean up? Maybe it could go into a separate patch. If anything, just to make this one slightly smaller.

Quoted 4 lines

I'm curious, is there a reason for this header change? Clean up? Maybe it could go into a separate patch. If anything, just to make this one slightly smaller.

hmm...I did that to get types.h for the new atomic_t usage.

However, now that I look at it again its bogus. The right fix is to explicitly include <linux/types.h> from jump_label.h.

Hi,

Re-fresh of updates against latest -tip tree.

I've tried to split this update up somewhat, but I've only succeeded to split out the dynamic debug bits. The interface changes and re-write are quite intertwined.

I believe this update should address all the comments from the previous posting except for Mathieu's request for a section of jump label pointers that point to the jump label structures (since the compiler might leave gaps in the jump label structures).

I've got a prototype patch to address this issue but its somewhat
invasive, and thus I'd like to leave it as a follow-up item. I have to date, not seen this issue in practice.

thanks,

-Jason

Jason Baron (2):
jump label: introduce static_branch() interface
dynamic debug: add jump label support

 arch/mips/include/asm/jump_label.h  |   22 +-
 arch/mips/kernel/jump_label.c       |    2 +-

 arch/sparc/include/asm/jump_label.h |   25 +-

 arch/x86/include/asm/alternative.h  |    3 +-
 arch/x86/include/asm/jump_label.h   |   26 +-
 arch/x86/kernel/alternative.c       |    2 +-
 arch/x86/kernel/jump_label.c        |    2 +-
 arch/x86/kernel/module.c            |    1 +
 include/asm-generic/vmlinux.lds.h   |   14 +-
 include/linux/dynamic_debug.h       |   44 +++-
 include/linux/jump_label.h          |   86 ++++---
 include/linux/jump_label_ref.h      |   44 ---
 include/linux/perf_event.h          |   26 +-
 include/linux/tracepoint.h          |   22 +-
 kernel/jump_label.c                 |  537 ++++++++++++++---------------------
 kernel/perf_event.c                 |    4 +-
 kernel/tracepoint.c                 |   23 +-
 lib/dynamic_debug.c                 |    4 +-
 18 files changed, 391 insertions(+), 496 deletions(-)
 delete mode 100644 include/linux/jump_label_ref.h

Quoted 3 lines · Jason Baron

Hi,

Re-fresh of updates against latest -tip tree.

Thanks Jason,

I started looking at them, I should have comments tomorrow (if I have any comments ;)

Quoted 5 lines

I've tried to split this update up somewhat, but I've only succeeded to split out the dynamic debug bits. The interface changes and re-write are quite intertwined.

Quoted 4 lines

I believe this update should address all the comments from the previous posting except for Mathieu's request for a section of jump label pointers that point to the jump label structures (since the compiler might leave gaps in the jump label structures).

The jump label structures is a list of 3 pointers, correct? I doubt that gcc would place any holes in it as they are all aligned by natural word size.

Quoted 3 lines

Hi,

Re-fresh of updates against latest -tip tree.

Thanks Jason,

I started looking at them, I should have comments tomorrow (if I have any comments ;)

Quoted 5 lines

I've tried to split this update up somewhat, but I've only succeeded to split out the dynamic debug bits. The interface changes and re-write are quite intertwined.

Quoted 4 lines

I believe this update should address all the comments from the previous posting except for Mathieu's request for a section of jump label pointers that point to the jump label structures (since the compiler might leave gaps in the jump label structures).

The jump label structures is a list of 3 pointers, correct? I doubt that gcc would place any holes in it as they are all aligned by natural word size.

Quoted 9 lines

I've got a prototype patch to address this issue but its somewhat
invasive, and thus I'd like to leave it as a follow-up item. I have to date, not seen this issue in practice.

thanks,

-Jason

Quoted 6 lines

Jason Baron (2):
jump label: introduce static_branch() interface
dynamic debug: add jump label support

 arch/mips/include/asm/jump_label.h  |   22 +-
 arch/mips/kernel/jump_label.c       |    2 +-

Could we get Ralf's acked-by for the mips code.

Quoted 1 line

 arch/sparc/include/asm/jump_label.h |   25 +-

Could we get David Miller's Acked-by for sparc.

Thanks,

-- Steve

Quoted 15 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 3 lines · Jason Baron

Hi,

Re-fresh of updates against latest -tip tree.

Thanks Jason,

I started looking at them, I should have comments tomorrow (if I have any comments ;)

Quoted 5 lines

I've tried to split this update up somewhat, but I've only succeeded to split out the dynamic debug bits. The interface changes and re-write are quite intertwined.

Quoted 9 lines

Quoted 4 lines

I believe this update should address all the comments from the previous posting except for Mathieu's request for a section of jump label pointers that point to the jump label structures (since the compiler might leave gaps in the jump label structures).

The jump label structures is a list of 3 pointers, correct? I doubt that gcc would place any holes in it as they are all aligned by natural word size.

Hi Steven,

Can you explain what would prevent gcc from aligning these 3 pointers (total of 24 bytes on 64-bit architectures) on 32-bytes ? Also, could you point out what would refrain the linker from aligning the start of object sections on the next 32-bytes (thus power of two) address multiple ?

I think we need to be a bit more strict in our interpretation of what guarantee gcc/ld provide and don't provide with respect to section and structure alignment.

As it stands now, the section alignment of jump labels looks half-broken on most architectures, and this *is* a big deal. I would really like to see a patch for this (it can be a separate patch) going in for .39.

Thank you,

Mathieu

Quoted 26 lines · Mathieu Desnoyers

Quoted 24 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 3 lines · Jason Baron

Hi,

Re-fresh of updates against latest -tip tree.

Thanks Jason,

I started looking at them, I should have comments tomorrow (if I have any comments ;)

Quoted 9 lines

I've tried to split this update up somewhat, but I've only succeeded to split out the dynamic debug bits. The interface changes and re-write are quite intertwined.

I believe this update should address all the comments from the previous posting except for Mathieu's request for a section of jump label pointers that point to the jump label structures (since the compiler might leave gaps in the jump label structures).

The jump label structures is a list of 3 pointers, correct? I doubt that gcc would place any holes in it as they are all aligned by natural word size.

Quoted 4 lines

Hi Steven,

Can you explain what would prevent gcc from aligning these 3 pointers (total of 24 bytes on 64-bit architectures) on 32-bytes ?

Quoted 4 lines

Also, could
you point out what would refrain the linker from aligning the start of object sections on the next 32-bytes (thus power of two) address multiple ?

Maybe it would be just easier to add another long ;)

Seriously, it would. Then it would be 32 bytes on 64bit and 16 bytes on 32bit. Then I guess we can have our guarantee without doing a large change to have this indirect pointer and still waste sizeof(long) bytes in having it.

Just insert a long "Reserved" word.

Quoted 2 lines · Steven Rostedt

Quoted 2 lines

Can you explain what would prevent gcc from aligning these 3 pointers (total of 24 bytes on 64-bit architectures) on 32-bytes ?

I can:

http://www.x86-64.org/documentation/abi.pdf Section 3.1.2:

Aggregates and Unions

Structures and unions assume the alignment of their most strictly aligned component. Each member is assigned to the lowest available offset with the appropriate alignment. The size of any object is always a multiple of the object‘s alignment.

An array uses the same alignment as its elements, except that a local or global array variable of length at least 16 bytes or a C99 variable-length array variable always has alignment of at least 16 bytes.

Structure and union objects can require padding to meet size and alignment constraints. The contents of any padding is undefined.

I don't think it is explicitly stated, but it is also true that the size is the smallest value that meets the above constraints.

Quoted 7 lines

Quoted 6 lines

Also, could
you point out what would refrain the linker from aligning the start of object sections on the next 32-bytes (thus power of two) address multiple ?

The rules of the ABI are quite specific. It would be a toolchain bug if this were messed up.

Quoted 1 line

Maybe it would be just easier to add another long ;)

Maybe we should audit all the data structures in the entire kernel and add manual padding to power of 2 boundaries.

Quoted 7 lines

Seriously, it would. Then it would be 32 bytes on 64bit and 16 bytes on 32bit. Then I guess we can have our guarantee without doing a large change to have this indirect pointer and still waste sizeof(long) bytes in having it.

Quoted 2 lines

Just insert a long "Reserved" word.

I disagree. Wasting memory to work around non-existent hypothetical bugs seems wrong to me.

Quoted 10 lines · David Daney

Quoted 2 lines · Steven Rostedt

Quoted 2 lines

Can you explain what would prevent gcc from aligning these 3 pointers (total of 24 bytes on 64-bit architectures) on 32-bytes ?

I can:

http://www.x86-64.org/documentation/abi.pdf Section 3.1.2:

Aggregates and Unions

Note, we are not dealing with C or arrays, but with inline assembly, and the linker.

(quoted diff) +12 -0

+static __always_inline bool arch_static_branch(struct jump_label_key *key)
+{
+       asm goto("1:\tnop\n\t"
+               "nop\n\t"
+               ".pushsection __jump_table,  \"aw\"\n\t"
+               WORD_INSN " 1b, %l[l_yes], %0\n\t"
+               ".popsection\n\t"
+               : :  "i" (key) : : l_yes);
+       return false;
+l_yes:
+       return true;
+}

That push/pop section part creates the structure we are talking about. It's made up of three pointers. The address of the nop, the address of the label l_yes and the address of the key.

Now its up to the linker to decide where to place that element. Can we guarantee that it will always be on an 8 byte boundery? Hmm, I wonder if we could add a .ALIGN sizeof(long) before that?

On Thu, Mar 10, 2011 at 01:04:01PM -0500, Steven Rostedt wrote:
Now its up to the linker to decide where to place that element. Can we

Quoted 3 lines

guarantee that it will always be on an 8 byte boundery? Hmm, I wonder if we could add a .ALIGN sizeof(long) before that?

we can. Sparc does it, see: arch/sparc/include/asm/jump_label.h.

So I guess it would be .align 8 for 64-bit and .align 4 for 32-bit...

Quoted 6 lines · Jason Baron

On Thu, Mar 10, 2011 at 01:04:01PM -0500, Steven Rostedt wrote:
Now its up to the linker to decide where to place that element. Can we

Quoted 3 lines

guarantee that it will always be on an 8 byte boundery? Hmm, I wonder if we could add a .ALIGN sizeof(long) before that?

Quoted 3 lines

we can. Sparc does it, see: arch/sparc/include/asm/jump_label.h.

So I guess it would be .align 8 for 64-bit and .align 4 for 32-bit...

Now what about the vmlinux.ld? That has a align 8. Is that a one time short (align to 8 bytes here), or will it carry through aligning the rest of the section. If not, perhaps we may be OK.

The alignment requested by the assembler will have to satisfy *all* the requested alignments, so manually forcing everything to .align 8 (or .align 4 for 32-bit) should ensure that the linker doesn't put in any holes.

Quoted 3 lines · David Daney

The alignment requested by the assembler will have to satisfy *all* the requested alignments, so manually forcing everything to .align 8 (or .align 4 for 32-bit) should ensure that the linker doesn't put in any holes.

I would agree with the assessment although, I don't know that it is documented anywhere that this is what happens. As the previous "bug" with the trace_events was solved by me adding .align(4) everywhere, I would think that .align(sizeof(long)) would work here too.

It may be a good ideal to force this alignment, and not add wasted space. If anything, if this (hypothetical) bug appears, it will most likely show up as a crash on boot up. I'm not too concerned about it.

Quoted 11 lines · Steven Rostedt

Quoted 3 lines · David Daney

The alignment requested by the assembler will have to satisfy *all* the requested alignments, so manually forcing everything to .align 8 (or .align 4 for 32-bit) should ensure that the linker doesn't put in any holes.

I would agree with the assessment although, I don't know that it is documented anywhere that this is what happens. As the previous "bug" with the trace_events was solved by me adding .align(4) everywhere, I would think that .align(sizeof(long)) would work here too.

Quoted 4 lines

It may be a good ideal to force this alignment, and not add wasted space. If anything, if this (hypothetical) bug appears, it will most likely show up as a crash on boot up. I'm not too concerned about it.

If the linker put in gratuitous holes, things like __ex_table would break too.

Quoted 17 lines · David Daney

Quoted 15 lines · Steven Rostedt

Quoted 3 lines · David Daney

The alignment requested by the assembler will have to satisfy *all* the requested alignments, so manually forcing everything to .align 8 (or .align 4 for 32-bit) should ensure that the linker doesn't put in any holes.

I would agree with the assessment although, I don't know that it is documented anywhere that this is what happens. As the previous "bug" with the trace_events was solved by me adding .align(4) everywhere, I would think that .align(sizeof(long)) would work here too.

It may be a good ideal to force this alignment, and not add wasted space. If anything, if this (hypothetical) bug appears, it will most likely show up as a crash on boot up. I'm not too concerned about it.

Quoted 2 lines

If the linker put in gratuitous holes, things like __ex_table would break too.

Again, as I have said (although I said extable not __ex_table), there seems to be no problem when the data is of a power of 2 as well. As __ex_table size is a power of 2. We just don't know if the linker will add holes when the size is something other than power of 2.

Quoted 21 lines · Steven Rostedt

Quoted 19 lines · David Daney

Quoted 6 lines · Steven Rostedt

Quoted 3 lines · David Daney

The alignment requested by the assembler will have to satisfy *all* the requested alignments, so manually forcing everything to .align 8 (or .align 4 for 32-bit) should ensure that the linker doesn't put in any holes.

Quoted 9 lines

I would agree with the assessment although, I don't know that it is documented anywhere that this is what happens. As the previous "bug" with the trace_events was solved by me adding .align(4) everywhere, I would think that .align(sizeof(long)) would work here too.

It may be a good ideal to force this alignment, and not add wasted space. If anything, if this (hypothetical) bug appears, it will most likely show up as a crash on boot up. I'm not too concerned about it.

If the linker put in gratuitous holes, things like __ex_table would break too.

Quoted 4 lines

Again, as I have said (although I said extable not __ex_table), there seems to be no problem when the data is of a power of 2 as well. As __ex_table size is a power of 2. We just don't know if the linker will add holes when the size is something other than power of 2.

Anyway, I think the best thing for now is to have Jason add the .align(sizeof(long)) in the inline assembly for all locations and be done with it.

agreed. something like the following is all that's needed for x86. Sparc already has this, so mips just needs something similar. Steve, should I re-post the entire series. Or can I just post this patch separately?

arch/x86/include/asm/jump_label.h +4 -2

index f217cee..57f31ff 100644
@@ -8,6 +8,7 @@
 #include <asm/asm.h>
 
 #define JUMP_LABEL_NOP_SIZE 5
+#define BYTES_PER_LONG (BITS_PER_LONG / 8)
 
 #define JUMP_LABEL_INITIAL_NOP ".byte 0xe9 \n\t .long 0\n\t"
 

@@ -16,9 +17,10 @@ static __always_inline bool arch_static_branch(struct jump_label_key *key)
 	asm goto("1:"
 		JUMP_LABEL_INITIAL_NOP
 		".pushsection __jump_table,  \"aw\" \n\t"
-		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
+		".align %c0\n\t"
+		_ASM_PTR "1b, %l[l_yes], %c1 \n\t"
 		".popsection \n\t"
-		: :  "i" (key) : : l_yes);
+		: : "i" BYTES_PER_LONG, "i" (key) : : l_yes);
 	return false;
 l_yes:
 	return true;

Quoted 21 lines · Steven Rostedt

Quoted 19 lines · David Daney

Quoted 6 lines · Steven Rostedt

Quoted 3 lines · David Daney

The alignment requested by the assembler will have to satisfy *all* the requested alignments, so manually forcing everything to .align 8 (or .align 4 for 32-bit) should ensure that the linker doesn't put in any holes.

Quoted 9 lines

I would agree with the assessment although, I don't know that it is documented anywhere that this is what happens. As the previous "bug" with the trace_events was solved by me adding .align(4) everywhere, I would think that .align(sizeof(long)) would work here too.

It may be a good ideal to force this alignment, and not add wasted space. If anything, if this (hypothetical) bug appears, it will most likely show up as a crash on boot up. I'm not too concerned about it.

If the linker put in gratuitous holes, things like __ex_table would break too.

Quoted 4 lines

Again, as I have said (although I said extable not __ex_table), there seems to be no problem when the data is of a power of 2 as well. As __ex_table size is a power of 2. We just don't know if the linker will add holes when the size is something other than power of 2.

Anyway, I think the best thing for now is to have Jason add the .align(sizeof(long)) in the inline assembly for all locations and be done with it.

-- Steve

Quoted 3 lines

Anyway, I think the best thing for now is to have Jason add the .align(sizeof(long)) in the inline assembly for all locations and be done with it.

You seem to be contradicting yourself here. I'm concerned about having "structures" of a size not power of two. Can we simply either

- Add a padding element at the end
or
- use .align 4*sizeof(long) at the beginning

to make sure the linker won't put any holes when it puts objects together ?

Quoted 6 lines · Mathieu Desnoyers

Quoted 3 lines

Anyway, I think the best thing for now is to have Jason add the .align(sizeof(long)) in the inline assembly for all locations and be done with it.

You seem to be contradicting yourself here. I'm concerned about having "structures" of a size not power of two. Can we simply either

But we don't have structures. We have data that has been allocated in assembly. Come to think of it, it may be best to keep these as ".align 4".

Quoted 8 lines

- Add a padding element at the end
or
- use .align 4*sizeof(long) at the beginning

to make sure the linker won't put any holes when it puts objects together ?

The linker should be dumb and not trying to "optimize", because it has no idea what the content is. If anything, it should try to compact things as best as possible, with the exception of keeping things naturally word aligned. If you added even ".align(4)" on a 64bit system, the linker should be trying to keep everything packed.

If I get time, I could look at the linker code to see exactly what it does, but adding holes into sections that are naturally word align seems more like a bug in the linker than a problem that we need to deal with.

The only issue I could fathom, is if gcc added its own padding in a section. That is, when it created the __jump_table section with one element, it added another 4/8 bytes to make the section size a power of two. Maybe that is a true issue, maybe not. It would seems stupid to do so IMHO, because when you get to bigger numbers, the aligning a power of 2 can get much bigger. But perhaps it does it for small power of 2s?

But that shouldn't matter because we only emit data to the __jump_table section from asm().

GCC is getting a reference to that table (array of structures really) from a global variable, I don't see how it can violate the ABI in this case.

Quoted 4 lines · Mathieu Desnoyers

Quoted 3 lines

Anyway, I think the best thing for now is to have Jason add the .align(sizeof(long)) in the inline assembly for all locations and be done with it.

Quoted 2 lines

You seem to be contradicting yourself here. I'm concerned about having "structures" of a size not power of two. Can we simply either

But we don't have structures. We have data that has been allocated in assembly. Come to think of it, it may be best to keep these as ".align 4".

Quoted 9 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 6 lines · Mathieu Desnoyers

Quoted 3 lines

Anyway, I think the best thing for now is to have Jason add the .align(sizeof(long)) in the inline assembly for all locations and be done with it.

You seem to be contradicting yourself here. I'm concerned about having "structures" of a size not power of two. Can we simply either

Quoted 3 lines

But we don't have structures. We have data that has been allocated in assembly. Come to think of it, it may be best to keep these as ".align 4".

The .align 4 is certainly not the right answer, because it will trigger unaligned accesses on some 64-bit architectures, as we have faced with trace event.

Quoted 10 lines · Mathieu Desnoyers

Quoted 9 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 6 lines · Mathieu Desnoyers

Quoted 3 lines

Anyway, I think the best thing for now is to have Jason add the .align(sizeof(long)) in the inline assembly for all locations and be done with it.

You seem to be contradicting yourself here. I'm concerned about having "structures" of a size not power of two. Can we simply either

Quoted 7 lines

Quoted 3 lines

But we don't have structures. We have data that has been allocated in assembly. Come to think of it, it may be best to keep these as ".align 4".

The .align 4 is certainly not the right answer, because it will trigger unaligned accesses on some 64-bit architectures, as we have faced with trace event.

Will it? Seems that sparc does this regardless.

Remember, this is 3 natural word sized objects, and vmlinux.ld starts the section off with .ALIGN 8, hence, the section is already 8 byte aligned, all objects within this section are 8 bytes (for 64bit archs, and 4 bytes for 32bit), why would saying ".align 4" cause the linker to add holes to make it 4 byte aligned when it is already 8 byte aligned. The ".align 4" should work with both 32bit and 64bit archs.

Quoted 20 lines · Steven Rostedt

Quoted 11 lines · Mathieu Desnoyers

Quoted 10 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 7 lines · Mathieu Desnoyers

Quoted 4 lines

Anyway, I think the best thing for now is to have Jason add the .align(sizeof(long)) in the inline assembly for all locations and be
done with it.

You seem to be contradicting yourself here. I'm concerned about having "structures" of a size not power of two. Can we simply either

Quoted 7 lines

Quoted 3 lines

But we don't have structures. We have data that has been allocated in assembly. Come to think of it, it may be best to keep these as ".align 4".

The .align 4 is certainly not the right answer, because it will trigger unaligned accesses on some 64-bit architectures, as we have faced with trace event.

Quoted 9 lines

Will it? Seems that sparc does this regardless.

Remember, this is 3 natural word sized objects, and vmlinux.ld starts the section off with .ALIGN 8, hence, the section is already 8 byte aligned, all objects within this section are 8 bytes (for 64bit archs, and 4 bytes for 32bit), why would saying ".align 4" cause the linker to add holes to make it 4 byte aligned when it is already 8 byte aligned. The ".align 4" should work with both 32bit and 64bit archs.

It should work, but it hurts my eyes to see the source code forcing a 64-bit word to 32-bit alignment.

Quoted 2 lines

It should work, but it hurts my eyes to see the source code forcing a 64-bit word to 32-bit alignment.

We solved such stuff with macros in other places already.

#ifdef 64bit

# define BLA 8
#else
# define BLA 4
#endif

and then use

.aling BLA

Where is the problem?

Quoted 13 lines · Thomas Gleixner

Quoted 2 lines

It should work, but it hurts my eyes to see the source code forcing a 64-bit word to 32-bit alignment.

We solved such stuff with macros in other places already.

#ifdef 64bit

# define BLA 8
#else
# define BLA 4
#endif

and then use

Quoted 3 lines

.aling BLA

Where is the problem?

That's basically the solution that Jason came up with, and what I expected. What caught my eye was that David Miller had .align 4 for this on sparc which is the one arch that is truly picky about such things. As I understood more about this, I see that would work for both archs.

That doesn't mean I'm against the .align BLA solution.

You seem to be contradicting yourself here. I'm concerned about having "structures" of a size not power of two. Can we simply either

- Add a padding element at the end
or
- use .align 4*sizeof(long) at the beginning

to make sure the linker won't put any holes when it puts objects together ?

Quoted 7 lines · Mathieu Desnoyers

You seem to be contradicting yourself here. I'm concerned about having "structures" of a size not power of two. Can we simply either

- Add a padding element at the end
or
- use .align 4*sizeof(long) at the beginning

Quoted 2 lines

to make sure the linker won't put any holes when it puts objects together ?

It may only be a technicality but but careful with .align. On some architectures .align x will align to a multiple of x; on other architectures to 2^x. Another reason to stick to C wherever possible.

Quoted 7 lines · Mathieu Desnoyers

You seem to be contradicting yourself here. I'm concerned about having "structures" of a size not power of two. Can we simply either

- Add a padding element at the end
or
- use .align 4*sizeof(long) at the beginning

Quoted 2 lines

to make sure the linker won't put any holes when it puts objects together ?

It may only be a technicality but but careful with .align. On some architectures .align x will align to a multiple of x; on other architectures to 2^x. Another reason to stick to C wherever possible.

Ralf

Quoted 3 lines · David Daney

The alignment requested by the assembler will have to satisfy *all* the requested alignments, so manually forcing everything to .align 8 (or .align 4 for 32-bit) should ensure that the linker doesn't put in any holes.

I would agree with the assessment although, I don't know that it is documented anywhere that this is what happens. As the previous "bug" with the trace_events was solved by me adding .align(4) everywhere, I would think that .align(sizeof(long)) would work here too.

It may be a good ideal to force this alignment, and not add wasted space. If anything, if this (hypothetical) bug appears, it will most likely show up as a crash on boot up. I'm not too concerned about it.

Quoted 27 lines · Steven Rostedt

Quoted 26 lines · Mathieu Desnoyers

Quoted 24 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 3 lines · Jason Baron

Hi,

Re-fresh of updates against latest -tip tree.

Thanks Jason,

I started looking at them, I should have comments tomorrow (if I have any comments ;)

Quoted 9 lines

I've tried to split this update up somewhat, but I've only succeeded to split out the dynamic debug bits. The interface changes and re-write are quite intertwined.

I believe this update should address all the comments from the previous posting except for Mathieu's request for a section of jump label pointers that point to the jump label structures (since the compiler might leave gaps in the jump label structures).

The jump label structures is a list of 3 pointers, correct? I doubt that gcc would place any holes in it as they are all aligned by natural word size.

Quoted 4 lines

Quoted 4 lines

Hi Steven,

Can you explain what would prevent gcc from aligning these 3 pointers (total of 24 bytes on 64-bit architectures) on 32-bytes ?

I can:

http://www.x86-64.org/documentation/abi.pdf Section 3.1.2:

Aggregates and Unions

Structures and unions assume the alignment of their most strictly aligned component. Each member is assigned to the lowest available offset with the appropriate alignment. The size of any object is always a multiple of the object‘s alignment.

An array uses the same alignment as its elements, except that a local or global array variable of length at least 16 bytes or a C99 variable-length array variable always has alignment of at least 16 bytes.

Structure and union objects can require padding to meet size and alignment constraints. The contents of any padding is undefined.

I don't think it is explicitly stated, but it is also true that the size is the smallest value that meets the above constraints.

Quoted 7 lines

Quoted 6 lines

Also, could
you point out what would refrain the linker from aligning the start of object sections on the next 32-bytes (thus power of two) address multiple ?

The rules of the ABI are quite specific. It would be a toolchain bug if this were messed up.

Quoted 4 lines

Maybe it would be just easier to add another long ;)

Maybe we should audit all the data structures in the entire kernel and add manual padding to power of 2 boundaries.

Quoted 7 lines

Seriously, it would. Then it would be 32 bytes on 64bit and 16 bytes on 32bit. Then I guess we can have our guarantee without doing a large change to have this indirect pointer and still waste sizeof(long) bytes in having it.

Quoted 2 lines

Just insert a long "Reserved" word.

I disagree. Wasting memory to work around non-existent hypothetical bugs seems wrong to me.

David Daney

Quoted 26 lines · Mathieu Desnoyers

Quoted 15 lines · * Steven Rostedt ([email protected]) wrote:

Quoted 3 lines · Jason Baron

Hi,

Re-fresh of updates against latest -tip tree.

Thanks Jason,

I started looking at them, I should have comments tomorrow (if I have any comments ;)

Quoted 5 lines

I've tried to split this update up somewhat, but I've only succeeded to split out the dynamic debug bits. The interface changes and re-write are quite intertwined.

Quoted 9 lines

Quoted 4 lines

I believe this update should address all the comments from the previous posting except for Mathieu's request for a section of jump label pointers that point to the jump label structures (since the compiler might leave gaps in the jump label structures).

The jump label structures is a list of 3 pointers, correct? I doubt that gcc would place any holes in it as they are all aligned by natural word size.

Quoted 7 lines

Hi Steven,

Can you explain what would prevent gcc from aligning these 3 pointers (total of 24 bytes on 64-bit architectures) on 32-bytes ? Also, could you point out what would refrain the linker from aligning the start of object sections on the next 32-bytes (thus power of two) address multiple ?

Maybe it would be just easier to add another long ;)

Seriously, it would. Then it would be 32 bytes on 64bit and 16 bytes on 32bit. Then I guess we can have our guarantee without doing a large change to have this indirect pointer and still waste sizeof(long) bytes in having it.

Just insert a long "Reserved" word.

-- Steve

Quoted 13 lines

I think we need to be a bit more strict in our interpretation of what guarantee gcc/ld provide and don't provide with respect to section and structure alignment.

As it stands now, the section alignment of jump labels looks half-broken on most architectures, and this *is* a big deal. I would really like to see a patch for this (it can be a separate patch) going in for .39.

Thank you,

Mathieu

Quoted 1 line

Just insert a long "Reserved" word.

I agree that this solution can work, but it's only because the "object" definition is done in assembly in this case (and not in C, like we did for trace event and tracepoints). Padding to power of 2 multiples should make the linker happy. There should be a nice comment beside these padding elements though.

Quoted 7 lines · Mathieu Desnoyers

Quoted 1 line

Just insert a long "Reserved" word.

I agree that this solution can work, but it's only because the "object" definition is done in assembly in this case (and not in C, like we did for trace event and tracepoints). Padding to power of 2 multiples should make the linker happy. There should be a nice comment beside these padding elements though.

Rereading what David and even what you wrote just now, I don't think this is even needed. As you said. The issue with us is that we had defined structs in C as static which lost all bets. Not to mention, these structures were not natural word aligned.

The linker should not be adding holes more than natural word alignment. Why waste space?

Hi,

Re-fresh of updates against latest -tip tree.