From: David Daney <ddaney@caviumnetworks.com> To: Jason Baron <jbaron@akamai.com>, rostedt@goodmis.org Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mips@linux-mips.org, linuxppc-dev@lists.ozlabs.org, Ingo Molnar <mingo@kernel.org>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, Michael Ellerman <mpe@ellerman.id.au>, Anton Blanchard <anton@samba.org>, Rabin Vincent <rabin@rab.in>, Russell King <linux@armlinux.org.uk>, Ralf Baechle <ralf@linux-mips.org>, Chris Metcalf <cmetcalf@mellanox.com>, Zhigang Lu <zlu@ezchip.com> Subject: Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Date: Mon, 27 Feb 2017 11:59:50 -0800 [thread overview] Message-ID: <aa139c18-1b04-2c20-2e22-89d74503b3cf@caviumnetworks.com> (raw) In-Reply-To: <93219edf-0f6d-5cc7-309c-c998f16fe7ac@akamai.com> 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.
WARNING: multiple messages have this Message-ID (diff)
From: ddaney@caviumnetworks.com (David Daney) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] jump_label: align jump_entry table to at least 4-bytes Date: Mon, 27 Feb 2017 11:59:50 -0800 [thread overview] Message-ID: <aa139c18-1b04-2c20-2e22-89d74503b3cf@caviumnetworks.com> (raw) In-Reply-To: <93219edf-0f6d-5cc7-309c-c998f16fe7ac@akamai.com> 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.
next prev parent reply other threads:[~2017-02-27 20:37 UTC|newest] Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-02-27 18:49 [PATCH] jump_label: align jump_entry table to at least 4-bytes Jason Baron 2017-02-27 18:49 ` Jason Baron 2017-02-27 18:57 ` David Daney 2017-02-27 18:57 ` David Daney 2017-02-27 19:18 ` Jason Baron 2017-02-27 19:18 ` Jason Baron 2017-02-27 19:59 ` David Daney [this message] 2017-02-27 19:59 ` David Daney 2017-02-27 21:06 ` Steven Rostedt 2017-02-27 21:06 ` Steven Rostedt 2017-02-27 21:41 ` David Daney 2017-02-27 21:41 ` David Daney 2017-02-27 22:09 ` Steven Rostedt 2017-02-27 22:09 ` Steven Rostedt 2017-02-27 22:21 ` David Daney 2017-02-27 22:21 ` David Daney 2017-02-27 22:21 ` David Daney 2017-02-27 22:36 ` Steven Rostedt 2017-02-27 22:36 ` Steven Rostedt 2017-02-27 22:45 ` David Daney 2017-02-27 22:45 ` David Daney 2017-02-27 22:50 ` Jason Baron 2017-02-27 22:50 ` Jason Baron 2017-02-27 23:34 ` David Daney 2017-02-27 23:34 ` David Daney 2017-02-28 4:55 ` Sachin Sant 2017-02-28 16:21 ` Steven Rostedt 2017-02-28 16:21 ` Steven Rostedt 2017-02-28 18:16 ` David Daney 2017-02-28 18:16 ` David Daney 2017-02-28 18:39 ` Jason Baron 2017-02-28 18:39 ` Jason Baron 2017-02-28 19:05 ` David Daney 2017-02-28 19:05 ` David Daney 2017-02-28 19:22 ` David Daney 2017-02-28 19:22 ` David Daney 2017-02-28 19:34 ` Jason Baron 2017-02-28 19:34 ` Jason Baron 2017-02-28 20:15 ` David Daney 2017-02-28 20:15 ` David Daney 2017-02-28 20:15 ` David Daney 2017-02-28 22:41 ` Jason Baron 2017-02-28 22:41 ` Jason Baron 2017-03-01 6:34 ` Michael Ellerman 2017-03-01 6:34 ` Michael Ellerman 2017-03-01 16:40 ` David Daney 2017-03-01 16:40 ` David Daney 2017-03-01 20:02 ` Jason Baron 2017-03-01 20:02 ` Jason Baron 2017-03-01 21:12 ` David Daney 2017-03-01 21:12 ` David Daney 2017-03-01 7:12 ` Sachin Sant 2017-03-01 7:12 ` Sachin Sant 2017-02-27 22:58 ` Steven Rostedt 2017-02-27 22:58 ` Steven Rostedt 2017-03-06 2:16 ` [PATCH] MIPS: jump_lable: Give __jump_table elements an entsize kbuild test robot 2017-03-06 2:16 ` kbuild test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=aa139c18-1b04-2c20-2e22-89d74503b3cf@caviumnetworks.com \ --to=ddaney@caviumnetworks.com \ --cc=anton@samba.org \ --cc=benh@kernel.crashing.org \ --cc=cmetcalf@mellanox.com \ --cc=jbaron@akamai.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mips@linux-mips.org \ --cc=linux@armlinux.org.uk \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mingo@kernel.org \ --cc=mpe@ellerman.id.au \ --cc=paulus@samba.org \ --cc=rabin@rab.in \ --cc=ralf@linux-mips.org \ --cc=rostedt@goodmis.org \ --cc=zlu@ezchip.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.