| Message ID | 1488221364-13905-1-git-send-email-jbaron@akamai.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 02/27/2017 01:57 PM, David Daney wrote: > On 02/27/2017 10:49 AM, Jason Baron wrote: >> The core jump_label code makes use of the 2 lower bits of the >> static_key::[type|entries|next] field. Thus, ensure that the jump_entry >> table is at least 4-byte aligned. >> > [...] >> diff --git a/arch/mips/include/asm/jump_label.h >> b/arch/mips/include/asm/jump_label.h >> index e77672539e8e..243791f3ae71 100644 >> --- a/arch/mips/include/asm/jump_label.h >> +++ b/arch/mips/include/asm/jump_label.h >> @@ -31,6 +31,7 @@ static __always_inline bool >> arch_static_branch(struct static_key *key, bool bran >> asm_volatile_goto("1:\t" NOP_INSN "\n\t" >> "nop\n\t" >> ".pushsection __jump_table, \"aw\"\n\t" >> + ".balign 4\n\t" >> WORD_INSN " 1b, %l[l_yes], %0\n\t" >> ".popsection\n\t" >> : : "i" (&((char *)key)[branch]) : : l_yes); >> @@ -45,6 +46,7 @@ static __always_inline bool >> arch_static_branch_jump(struct static_key *key, bool >> asm_volatile_goto("1:\tj %l[l_yes]\n\t" >> "nop\n\t" >> ".pushsection __jump_table, \"aw\"\n\t" >> + ".balign 4\n\t" >> WORD_INSN " 1b, %l[l_yes], %0\n\t" >> ".popsection\n\t" >> : : "i" (&((char *)key)[branch]) : : l_yes); > > > I will speak only for the MIPS part. > > If the section is not already properly aligned, this change will add > padding, which is probably not what we want. > > Have you ever seen a problem with misalignment in the real world? > Hi, Yes, there was a WARN_ON() reported on POWER here: https://lkml.org/lkml/2017/2/19/85 The WARN_ON() triggers if either of the 2 lsb are set. More specifically, its coming from 'static_key_set_entries()' from the following patch, which was recently added to linux-next: https://lkml.org/lkml/2017/2/3/558 So this was not seen on mips, but I included all arches in this patch that I though might be affected. > If so, I think a better approach might be to set properties on the > __jump_table section to force the proper alignment, or do something in > the linker script. > So in include/asm-generic/vmlinux.lds.h, we are already setting 'ALIGN(8)', but that does not appear to be sufficient for POWER... Also, I checked the size of the vmlinux generated after this change on all 4 arches, and it was rather minimal. I think POWER increased the most, but the other arches increased by only a few bytes. Thanks, -Jason
On 02/27/2017 11:18 AM, Jason Baron wrote: > > > On 02/27/2017 01:57 PM, David Daney wrote: >> On 02/27/2017 10:49 AM, Jason Baron wrote: >>> The core jump_label code makes use of the 2 lower bits of the >>> static_key::[type|entries|next] field. Thus, ensure that the jump_entry >>> table is at least 4-byte aligned. >>> >> [...] >>> diff --git a/arch/mips/include/asm/jump_label.h >>> b/arch/mips/include/asm/jump_label.h >>> index e77672539e8e..243791f3ae71 100644 >>> --- a/arch/mips/include/asm/jump_label.h >>> +++ b/arch/mips/include/asm/jump_label.h >>> @@ -31,6 +31,7 @@ static __always_inline bool >>> arch_static_branch(struct static_key *key, bool bran >>> asm_volatile_goto("1:\t" NOP_INSN "\n\t" >>> "nop\n\t" >>> ".pushsection __jump_table, \"aw\"\n\t" >>> + ".balign 4\n\t" >>> WORD_INSN " 1b, %l[l_yes], %0\n\t" >>> ".popsection\n\t" >>> : : "i" (&((char *)key)[branch]) : : l_yes); >>> @@ -45,6 +46,7 @@ static __always_inline bool >>> arch_static_branch_jump(struct static_key *key, bool >>> asm_volatile_goto("1:\tj %l[l_yes]\n\t" >>> "nop\n\t" >>> ".pushsection __jump_table, \"aw\"\n\t" >>> + ".balign 4\n\t" >>> WORD_INSN " 1b, %l[l_yes], %0\n\t" >>> ".popsection\n\t" >>> : : "i" (&((char *)key)[branch]) : : l_yes); >> >> >> I will speak only for the MIPS part. >> >> If the section is not already properly aligned, this change will add >> padding, which is probably not what we want. >> >> Have you ever seen a problem with misalignment in the real world? >> > > Hi, > > Yes, there was a WARN_ON() reported on POWER here: > > https://lkml.org/lkml/2017/2/19/85 > > The WARN_ON() triggers if either of the 2 lsb are set. More > specifically, its coming from 'static_key_set_entries()' from the > following patch, which was recently added to linux-next: > > https://lkml.org/lkml/2017/2/3/558 > > So this was not seen on mips, but I included all arches in this patch > that I though might be affected. > >> If so, I think a better approach might be to set properties on the >> __jump_table section to force the proper alignment, or do something in >> the linker script. >> > > So in include/asm-generic/vmlinux.lds.h, we are already setting > 'ALIGN(8)', but that does not appear to be sufficient for POWER... Yes, I just looked at that. The ALIGN(8), combined with the fact that on MIPS, WORD_INSN will always be of size 4 or 8 (32-bit vs. 64-bit kernel), seems to make any additional .balign completely unnecessary. Really, I think you need to figure out why on powerpc the alignments are getting screwed up. The struct jump_entry entries are on a stride of 12 (for a 32-bit kernel) or 24 (for a 64-bit kernel), any alignment padding added by .balign 4 will surely screw that up. This patch seems like it is papering over a bigger issue that is not understood. I think a proper fix would be to fix whatever is causing the powerpc JUMP_ENTRY_TYPE to misbehave. > > Also, I checked the size of the vmlinux generated after this change on > all 4 arches, and it was rather minimal. I think POWER increased the > most, but the other arches increased by only a few bytes. For me the size is not the important issue, it is the alignment of the struct jump_entry entries in the table. I don't understand how your patch helps, and I cannot Acked-by unless I understand what is being done and can see that it is both correct and necessary. David.
On Mon, 27 Feb 2017 11:59:50 -0800 David Daney <ddaney@caviumnetworks.com> wrote: > For me the size is not the important issue, it is the alignment of the > struct jump_entry entries in the table. I don't understand how your > patch helps, and I cannot Acked-by unless I understand what is being > done and can see that it is both correct and necessary. You brought up a very good point and I'm glad that I had Jason Cc all the arch maintainers in one patch. I think jump_labels may be much more broken than we think, and Jason's fix doesn't fix anything. We had this same issues with tracepoints. I'm looking at jump_label_init, and how we iterate over an array of struct jump_entry's that was put together by the linker. The problem is that jump_entry is not a power of 2 in size. struct jump_entry { jump_label_t code; jump_label_t target; jump_label_t key; }; When putting together arrays of this kind, the linker is in its right to add padding for alignment, in the middle of the array! It has no idea that this is an array, and there's nothing stopping the linker from messing it up. For those structs that are a power of 2 in size, there's no reason for the linker to do anything else, and it "just works". There's plenty of instances in the kernel that depend on this. I'm thinking that the sort algorithm either hid the problem or fixed it somehow (I'm guessing it hid the problem). I hit the same issue with trace event structures. The solution was to create the array of pointers to each structure, and dereference the structures from the array. See commit e4a9ea5ee ("tracing: Replace trace_event struct array with pointer array") -- Steve
On 02/27/2017 01:06 PM, Steven Rostedt wrote: > On Mon, 27 Feb 2017 11:59:50 -0800 > David Daney <ddaney@caviumnetworks.com> wrote: > >> For me the size is not the important issue, it is the alignment of the >> struct jump_entry entries in the table. I don't understand how your >> patch helps, and I cannot Acked-by unless I understand what is being >> done and can see that it is both correct and necessary. > > You brought up a very good point and I'm glad that I had Jason Cc all > the arch maintainers in one patch. > > I think jump_labels may be much more broken than we think, and Jason's > fix doesn't fix anything. We had this same issues with tracepoints. > > I'm looking at jump_label_init, and how we iterate over an array of > struct jump_entry's that was put together by the linker. The problem is > that jump_entry is not a power of 2 in size. > ELF sections may have an ENTSIZE property exactly for arrays. Since each jump_entry will have a unique value they cannot be merged, but we can tell the assembler they are an array and get them properly packed. Perhaps something like (untested): .pushsection __jump_table, \"awM\",@progbits,24 FOO .popsection > struct jump_entry { > jump_label_t code; > jump_label_t target; > jump_label_t key; > }; > > When putting together arrays of this kind, the linker is in its right > to add padding for alignment, in the middle of the array! It has no > idea that this is an array, and there's nothing stopping the linker > from messing it up. > > For those structs that are a power of 2 in size, there's no reason for > the linker to do anything else, and it "just works". There's plenty of > instances in the kernel that depend on this. > > I'm thinking that the sort algorithm either hid the problem or fixed it > somehow (I'm guessing it hid the problem). > > I hit the same issue with trace event structures. The solution was to > create the array of pointers to each structure, and dereference the > structures from the array. > > See commit e4a9ea5ee ("tracing: Replace trace_event struct array with > pointer array") > > -- Steve >
On Mon, 27 Feb 2017 13:41:13 -0800 David Daney <ddaney@caviumnetworks.com> wrote: > On 02/27/2017 01:06 PM, Steven Rostedt wrote: > > On Mon, 27 Feb 2017 11:59:50 -0800 > > David Daney <ddaney@caviumnetworks.com> wrote: > > > >> For me the size is not the important issue, it is the alignment of the > >> struct jump_entry entries in the table. I don't understand how your > >> patch helps, and I cannot Acked-by unless I understand what is being > >> done and can see that it is both correct and necessary. > > > > You brought up a very good point and I'm glad that I had Jason Cc all > > the arch maintainers in one patch. > > > > I think jump_labels may be much more broken than we think, and Jason's > > fix doesn't fix anything. We had this same issues with tracepoints. > > > > I'm looking at jump_label_init, and how we iterate over an array of > > struct jump_entry's that was put together by the linker. The problem is > > that jump_entry is not a power of 2 in size. > > > > ELF sections may have an ENTSIZE property exactly for arrays. Since > each jump_entry will have a unique value they cannot be merged, but we > can tell the assembler they are an array and get them properly packed. > Perhaps something like (untested): > > .pushsection __jump_table, \"awM\",@progbits,24 > FOO > .popsection > And the linker will honor this too? -- Steve
diff --git a/arch/arm/include/asm/jump_label.h b/arch/arm/include/asm/jump_label.h index 34f7b6980d21..9c017bb04d1c 100644 --- a/arch/arm/include/asm/jump_label.h +++ b/arch/arm/include/asm/jump_label.h @@ -13,6 +13,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran asm_volatile_goto("1:\n\t" WASM(nop) "\n\t" ".pushsection __jump_table, \"aw\"\n\t" + ".balign 4\n\t" ".word 1b, %l[l_yes], %c0\n\t" ".popsection\n\t" : : "i" (&((char *)key)[branch]) : : l_yes); @@ -27,6 +28,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool asm_volatile_goto("1:\n\t" WASM(b) " %l[l_yes]\n\t" ".pushsection __jump_table, \"aw\"\n\t" + ".balign 4\n\t" ".word 1b, %l[l_yes], %c0\n\t" ".popsection\n\t" : : "i" (&((char *)key)[branch]) : : l_yes); diff --git a/arch/mips/include/asm/jump_label.h b/arch/mips/include/asm/jump_label.h index e77672539e8e..243791f3ae71 100644 --- a/arch/mips/include/asm/jump_label.h +++ b/arch/mips/include/asm/jump_label.h @@ -31,6 +31,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran asm_volatile_goto("1:\t" NOP_INSN "\n\t" "nop\n\t" ".pushsection __jump_table, \"aw\"\n\t" + ".balign 4\n\t" WORD_INSN " 1b, %l[l_yes], %0\n\t" ".popsection\n\t" : : "i" (&((char *)key)[branch]) : : l_yes); @@ -45,6 +46,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool asm_volatile_goto("1:\tj %l[l_yes]\n\t" "nop\n\t" ".pushsection __jump_table, \"aw\"\n\t" + ".balign 4\n\t" WORD_INSN " 1b, %l[l_yes], %0\n\t" ".popsection\n\t" : : "i" (&((char *)key)[branch]) : : l_yes); diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h index 9a287e0ac8b1..bfe83496b590 100644 --- a/arch/powerpc/include/asm/jump_label.h +++ b/arch/powerpc/include/asm/jump_label.h @@ -24,6 +24,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran asm_volatile_goto("1:\n\t" "nop # arch_static_branch\n\t" ".pushsection __jump_table, \"aw\"\n\t" + ".balign 4\n\t" JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t" ".popsection \n\t" : : "i" (&((char *)key)[branch]) : : l_yes); @@ -38,6 +39,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool asm_volatile_goto("1:\n\t" "b %l[l_yes] # arch_static_branch_jump\n\t" ".pushsection __jump_table, \"aw\"\n\t" + ".balign 4\n\t" JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t" ".popsection \n\t" : : "i" (&((char *)key)[branch]) : : l_yes); @@ -63,6 +65,7 @@ struct jump_entry { #define ARCH_STATIC_BRANCH(LABEL, KEY) \ 1098: nop; \ .pushsection __jump_table, "aw"; \ + .balign 4; \ FTR_ENTRY_LONG 1098b, LABEL, KEY; \ .popsection #endif diff --git a/arch/tile/include/asm/jump_label.h b/arch/tile/include/asm/jump_label.h index cde7573f397b..a964e6135ea3 100644 --- a/arch/tile/include/asm/jump_label.h +++ b/arch/tile/include/asm/jump_label.h @@ -25,6 +25,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, asm_volatile_goto("1:\n\t" "nop" "\n\t" ".pushsection __jump_table, \"aw\"\n\t" + ".balign 4\n\t" ".quad 1b, %l[l_yes], %0 + %1 \n\t" ".popsection\n\t" : : "i" (key), "i" (branch) : : l_yes); @@ -39,6 +40,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, asm_volatile_goto("1:\n\t" "j %l[l_yes]" "\n\t" ".pushsection __jump_table, \"aw\"\n\t" + ".balign 4\n\t" ".quad 1b, %l[l_yes], %0 + %1 \n\t" ".popsection\n\t" : : "i" (key), "i" (branch) : : l_yes);
The core jump_label code makes use of the 2 lower bits of the static_key::[type|entries|next] field. Thus, ensure that the jump_entry table is at least 4-byte aligned. Reported-and-tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@kernel.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Anton Blanchard <anton@samba.org> Cc: Rabin Vincent <rabin@rab.in> Cc: Russell King <linux@armlinux.org.uk> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Chris Metcalf <cmetcalf@mellanox.com> Cc: Zhigang Lu <zlu@ezchip.com> Cc: David Daney <ddaney@caviumnetworks.com> Signed-off-by: Jason Baron <jbaron@akamai.com> --- arch/arm/include/asm/jump_label.h | 2 ++ arch/mips/include/asm/jump_label.h | 2 ++ arch/powerpc/include/asm/jump_label.h | 3 +++ arch/tile/include/asm/jump_label.h | 2 ++ 4 files changed, 9 insertions(+)