All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jason Baron <jbaron@akamai.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: akpm@linux-foundation.org, joe@perches.com, peterz@infradead.org,
	linux-kernel@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	sparclinux@vger.kernel.org, cmetcalf@mellanox.com
Subject: Re: [PATCH v2 4/4] dynamic_debug: add jump label support
Date: Fri, 10 Jun 2016 21:28:40 +0000	[thread overview]
Message-ID: <575B3108.1020409@akamai.com> (raw)
In-Reply-To: <3796097.4dlb9d7Id8@wuerfel>

On 06/10/2016 05:54 AM, Arnd Bergmann wrote:
> On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
>> Although dynamic debug is often only used for debug builds, sometimes its
>> enabled for production builds as well. Minimize its impact by using jump
>> labels. This reduces the text section by 7000+ bytes in the kernel image
>> below. It does increase data, but this should only be referenced when
>> changing the direction of the branches, and hence usually not in cache.
>>
>>    text	   data	    bss	    dec	    hex	filename
>> 8194852	4879776	 925696	14000324	 d5a0c4	vmlinux.pre
>> 8187337	4960224	 925696	14073257	 d6bda9	vmlinux.post
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> ---
> 
> This causes problems for some of my randconfig builds, when a dynamic
> debug call is used inside of an __exit function:
> 
> `.exit.text' referenced in section `__jump_table' of drivers/built-in.o: defined in discarded section `.exit.text' of drivers/built-in.o
> `.exit.text' referenced in section `__jump_table' of drivers/built-in.o: defined in discarded section `.exit.text' of drivers/built-in.o
> 
> 	Arnd
> 

Hi Arnd,

Ok, I managed to reproduce this on tile and sparc64 by adding
static_branch_[un]likely() to __exit functions as you mentioned.
Although I didn't find the actual broken config.

I think its only an issue on those 2 arches b/c they have jump
label support and discard __exit text at build time (most
arches seem to do it at run-time). Thus, we can end up with
references in the __jump_table to addresses that may be in an
__exit section. The jump label code already protects itself
from touch code in the init sections after it has been freed.
Thus, simply having functions marked with __exit in the init
section is sufficient here.

I tried the following patches on tile and sparc and they
at least now compile. If anybody has real h/w that would
be helpful :)

Thanks,

-Jason

--- a/arch/sparc/kernel/vmlinux.lds.S
+++ b/arch/sparc/kernel/vmlinux.lds.S
@@ -151,6 +151,12 @@ SECTIONS
        PERCPU_SECTION(SMP_CACHE_BYTES)

        . = ALIGN(PAGE_SIZE);
+       .exit.text : {
+               EXIT_TEXT
+       }
+
+       . = ALIGN(PAGE_SIZE);
+
        __init_end = .;
        BSS_SECTION(0, 0, 0)
        _end = . ;

--- a/arch/tile/kernel/vmlinux.lds.S
+++ b/arch/tile/kernel/vmlinux.lds.S
@@ -58,7 +58,21 @@ SECTIONS
   _etext = .;

   /* "Init" is divided into two areas with very different virtual
addresses. */
+  . = ALIGN(PAGE_SIZE);
+  .init.begin : AT(ADDR(.init.begin) - LOAD_OFFSET) {
+     __init_begin = .; /* paired with __init_end */
+  }
+
   INIT_TEXT_SECTION(PAGE_SIZE)
+  .exit.text : AT(ADDR(.exit.text) - LOAD_OFFSET) {
+       EXIT_TEXT
+  }
+
+  . = ALIGN(PAGE_SIZE);
+  /* freed after init ends here */
+  .init.end : AT(ADDR(.init.end) - LOAD_OFFSET) {
+       __init_end = .;
+  }

   /* Now we skip back to PAGE_OFFSET for the data. */
   . = (. - TEXT_OFFSET + PAGE_OFFSET);


WARNING: multiple messages have this Message-ID (diff)
From: Jason Baron <jbaron@akamai.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: <akpm@linux-foundation.org>, <joe@perches.com>,
	<peterz@infradead.org>, <linux-kernel@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	<sparclinux@vger.kernel.org>, <cmetcalf@mellanox.com>
Subject: Re: [PATCH v2 4/4] dynamic_debug: add jump label support
Date: Fri, 10 Jun 2016 17:28:40 -0400	[thread overview]
Message-ID: <575B3108.1020409@akamai.com> (raw)
In-Reply-To: <3796097.4dlb9d7Id8@wuerfel>

On 06/10/2016 05:54 AM, Arnd Bergmann wrote:
> On Friday, May 20, 2016 5:16:36 PM CEST Jason Baron wrote:
>> Although dynamic debug is often only used for debug builds, sometimes its
>> enabled for production builds as well. Minimize its impact by using jump
>> labels. This reduces the text section by 7000+ bytes in the kernel image
>> below. It does increase data, but this should only be referenced when
>> changing the direction of the branches, and hence usually not in cache.
>>
>>    text	   data	    bss	    dec	    hex	filename
>> 8194852	4879776	 925696	14000324	 d5a0c4	vmlinux.pre
>> 8187337	4960224	 925696	14073257	 d6bda9	vmlinux.post
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> ---
> 
> This causes problems for some of my randconfig builds, when a dynamic
> debug call is used inside of an __exit function:
> 
> `.exit.text' referenced in section `__jump_table' of drivers/built-in.o: defined in discarded section `.exit.text' of drivers/built-in.o
> `.exit.text' referenced in section `__jump_table' of drivers/built-in.o: defined in discarded section `.exit.text' of drivers/built-in.o
> 
> 	Arnd
> 

Hi Arnd,

Ok, I managed to reproduce this on tile and sparc64 by adding
static_branch_[un]likely() to __exit functions as you mentioned.
Although I didn't find the actual broken config.

I think its only an issue on those 2 arches b/c they have jump
label support and discard __exit text at build time (most
arches seem to do it at run-time). Thus, we can end up with
references in the __jump_table to addresses that may be in an
__exit section. The jump label code already protects itself
from touch code in the init sections after it has been freed.
Thus, simply having functions marked with __exit in the init
section is sufficient here.

I tried the following patches on tile and sparc and they
at least now compile. If anybody has real h/w that would
be helpful :)

Thanks,

-Jason

--- a/arch/sparc/kernel/vmlinux.lds.S
+++ b/arch/sparc/kernel/vmlinux.lds.S
@@ -151,6 +151,12 @@ SECTIONS
        PERCPU_SECTION(SMP_CACHE_BYTES)

        . = ALIGN(PAGE_SIZE);
+       .exit.text : {
+               EXIT_TEXT
+       }
+
+       . = ALIGN(PAGE_SIZE);
+
        __init_end = .;
        BSS_SECTION(0, 0, 0)
        _end = . ;

--- a/arch/tile/kernel/vmlinux.lds.S
+++ b/arch/tile/kernel/vmlinux.lds.S
@@ -58,7 +58,21 @@ SECTIONS
   _etext = .;

   /* "Init" is divided into two areas with very different virtual
addresses. */
+  . = ALIGN(PAGE_SIZE);
+  .init.begin : AT(ADDR(.init.begin) - LOAD_OFFSET) {
+     __init_begin = .; /* paired with __init_end */
+  }
+
   INIT_TEXT_SECTION(PAGE_SIZE)
+  .exit.text : AT(ADDR(.exit.text) - LOAD_OFFSET) {
+       EXIT_TEXT
+  }
+
+  . = ALIGN(PAGE_SIZE);
+  /* freed after init ends here */
+  .init.end : AT(ADDR(.init.end) - LOAD_OFFSET) {
+       __init_end = .;
+  }

   /* Now we skip back to PAGE_OFFSET for the data. */
   . = (. - TEXT_OFFSET + PAGE_OFFSET);

  parent reply	other threads:[~2016-06-10 21:28 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-20 21:16 [PATCH v2 0/4] convert dynamic_debug to use jump labels Jason Baron
2016-05-20 21:16 ` [PATCH v2 1/4] jump_label: remove bug.h, atomic.h dependencies for HAVE_JUMP_LABEL Jason Baron
2016-05-20 21:16 ` [PATCH v2 2/4] powerpc: add explicit #include <asm/asm-compat.h> for jump label Jason Baron
2016-05-20 21:16 ` [PATCH v2 3/4] s390: add explicit <linux/stringify.h> " Jason Baron
2016-05-20 21:16 ` [PATCH v2 4/4] dynamic_debug: add jump label support Jason Baron
2016-06-10  9:54   ` Arnd Bergmann
2016-06-10 15:33     ` Jason Baron
2016-06-13 16:05       ` Arnd Bergmann
2016-06-13 20:23         ` Arnd Bergmann
2016-06-13 20:23           ` Arnd Bergmann
2016-06-13 20:32           ` Jason Baron
2016-06-13 20:32             ` Jason Baron
2016-07-01 20:45             ` Arnd Bergmann
2016-07-01 20:45               ` Arnd Bergmann
     [not found]               ` <5786613E.6010509@akamai.com>
2016-07-13 16:03                 ` Arnd Bergmann
2016-06-10 21:28     ` Jason Baron [this message]
2016-06-10 21:28       ` Jason Baron
2016-07-01 19:30       ` Chris Metcalf
2016-07-01 19:30         ` Chris Metcalf
2016-07-05 20:57         ` Jason Baron
2016-07-05 20:57           ` Jason Baron
2016-07-06 16:52           ` Chris Metcalf
2016-07-06 16:52             ` Chris Metcalf

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=575B3108.1020409@akamai.com \
    --to=jbaron@akamai.com \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@arndb.de \
    --cc=cmetcalf@mellanox.com \
    --cc=davem@davemloft.net \
    --cc=joe@perches.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=sparclinux@vger.kernel.org \
    /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: link
Be 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.