jump_label: align jump_entry table to at least 4-bytes

Commit Message

David Daney Feb. 27, 2017, 10:21 p.m. UTC

On 02/27/2017 02:09 PM, Steven Rostedt wrote:
> On Mon, 27 Feb 2017 13:41:13 -0800
> David Daney <[e-mail]> wrote:
>
>> On 02/27/2017 01:06 PM, Steven Rostedt wrote:
>>> On Mon, 27 Feb 2017 11:59:50 -0800
>>> David Daney <[e-mail]> 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?

See attached for mips.  It seems to do the right thing.

I leave it as an exercise to the reader to fix the other architectures.

Consult your own  binutils experts to verify that what I say is true.


David Daney

Comments

Steven Rostedt Feb. 27, 2017, 10:36 p.m. UTC | #1

On Mon, 27 Feb 2017 14:21:21 -0800
David Daney <[e-mail]> wrote:

> See attached for mips.  It seems to do the right thing.
> 
> I leave it as an exercise to the reader to fix the other architectures.
> 
> Consult your own  binutils experts to verify that what I say is true.

It may still just be safer to do the pointers instead. That way we
don't need to worry about some strange arch or off by one binutils
messing it up.

-- Steve

David Daney Feb. 27, 2017, 10:45 p.m. UTC | #2

On 02/27/2017 02:36 PM, Steven Rostedt wrote:
> On Mon, 27 Feb 2017 14:21:21 -0800
> David Daney <[e-mail]> wrote:
>
>> See attached for mips.  It seems to do the right thing.
>>
>> I leave it as an exercise to the reader to fix the other architectures.
>>
>> Consult your own  binutils experts to verify that what I say is true.
>
> It may still just be safer to do the pointers instead. That way we
> don't need to worry about some strange arch or off by one binutils
> messing it up.

Obviously it is your choice, but this is bog standard ELF linking.  In 
theory even the arrays of power-of-2 sized objects should also supply an 
entity size.  Think __ex_table and its ilk.


The benefit of supplying an entsize is that you don't have to change the 
structure of the existing code and risk breaking something in the process.

David Daney

Steven Rostedt Feb. 27, 2017, 10:58 p.m. UTC | #3

On Mon, 27 Feb 2017 14:45:37 -0800
David Daney <[e-mail]> wrote:

> On 02/27/2017 02:36 PM, Steven Rostedt wrote:
> > On Mon, 27 Feb 2017 14:21:21 -0800
> > David Daney <[e-mail]> wrote:
> >  
> >> See attached for mips.  It seems to do the right thing.
> >>
> >> I leave it as an exercise to the reader to fix the other architectures.
> >>
> >> Consult your own  binutils experts to verify that what I say is true.  
> >
> > It may still just be safer to do the pointers instead. That way we
> > don't need to worry about some strange arch or off by one binutils
> > messing it up.  
> 
> Obviously it is your choice, but this is bog standard ELF linking.  In 
> theory even the arrays of power-of-2 sized objects should also supply an 
> entity size.  Think __ex_table and its ilk.
> 
> 
> The benefit of supplying an entsize is that you don't have to change the 
> structure of the existing code and risk breaking something in the process.

I agree that this may be the better answer. The issue tracepoints had
is that they were defined in C with a "section" attribute. I'm not sure
you can pass various section attributes via a gcc section attribute.

-- Steve

kernel test robot March 6, 2017, 2:16 a.m. UTC | #4

Hi David,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc1 next-20170303]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/David-Daney/MIPS-jump_lable-Give-__jump_table-elements-an-entsize/20170228-231935
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All error/warnings (new ones prefixed by >>):

   {standard input}: Assembler messages:
>> {standard input}:968: Warning: entity size for SHF_MERGE not specified
>> {standard input}:968: Error: junk at end of line, first unrecognized character is `@'
>> {standard input}:968: Warning: ignoring changed section attributes for __jump_table
   {standard input}:987: Warning: entity size for SHF_MERGE not specified
   {standard input}:987: Error: junk at end of line, first unrecognized character is `@'
   {standard input}:987: Warning: ignoring changed section attributes for __jump_table
   {standard input}:1092: Warning: entity size for SHF_MERGE not specified
   {standard input}:1092: Error: junk at end of line, first unrecognized character is `@'
   {standard input}:1092: Warning: ignoring changed section attributes for __jump_table
   {standard input}:1113: Warning: entity size for SHF_MERGE not specified
   {standard input}:1113: Error: junk at end of line, first unrecognized character is `@'
   {standard input}:1113: Warning: ignoring changed section attributes for __jump_table

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Patch

From 484111a11bb5187a4c190cd80b4a03a18ee7047b Mon Sep 17 00:00:00 2001
From: David Daney <[e-mail]>
Date: Mon, 27 Feb 2017 14:14:30 -0800
Subject: [PATCH] MIPS: jump_lable: Give __jump_table elements an entsize.

Since the __jump_table is a big array of non power-of-2 sized
elements, tell the linker the size so they can be properly packed.
Since each element will have unique "code" and "key" fields, no
merging will ever occur, but it should end up properly packed.

Signed-off-by: David Daney <[e-mail]>
---
 arch/mips/include/asm/jump_label.h | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/mips/include/asm/jump_label.h b/arch/mips/include/asm/jump_label.h
index e776725..c9a695f 100644
--- a/arch/mips/include/asm/jump_label.h
+++ b/arch/mips/include/asm/jump_label.h
@@ -26,14 +26,26 @@ 
 #define NOP_INSN "nop"
 #endif
 
+#ifdef CONFIG_64BIT
+typedef u64 jump_label_t;
+#else
+typedef u32 jump_label_t;
+#endif
+
+struct jump_entry {
+	jump_label_t code;
+	jump_label_t target;
+	jump_label_t key;
+};
+
 static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
 	asm_volatile_goto("1:\t" NOP_INSN "\n\t"
 		"nop\n\t"
-		".pushsection __jump_table,  \"aw\"\n\t"
+		".pushsection __jump_table, \"awM\",@progbits, %1\n\t"
 		WORD_INSN " 1b, %l[l_yes], %0\n\t"
 		".popsection\n\t"
-		: :  "i" (&((char *)key)[branch]) : : l_yes);
+		: :  "i" (&((char *)key)[branch]), "i" (sizeof(struct jump_entry)): : l_yes);
 
 	return false;
 l_yes:
@@ -44,27 +56,15 @@  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"
+		".pushsection __jump_table, \"awM\"@progbits, %1\n\t"
 		WORD_INSN " 1b, %l[l_yes], %0\n\t"
 		".popsection\n\t"
-		: :  "i" (&((char *)key)[branch]) : : l_yes);
+		: :  "i" (&((char *)key)[branch]), "i" (sizeof(struct jump_entry)) : : l_yes);
 
 	return false;
 l_yes:
 	return true;
 }
 
-#ifdef CONFIG_64BIT
-typedef u64 jump_label_t;
-#else
-typedef u32 jump_label_t;
-#endif
-
-struct jump_entry {
-	jump_label_t code;
-	jump_label_t target;
-	jump_label_t key;
-};
-
 #endif  /* __ASSEMBLY__ */
 #endif /* _ASM_MIPS_JUMP_LABEL_H */
-- 
2.9.3