[v2,4/4] dynamic_debug: add jump label support

On 6/10/2016 5:28 PM, Jason Baron wrote:
> 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.

It seems plausible to me to only include the exit text in with the init text
under an #ifdef CONFIG_JUMP_TABLE (with a suitable comment) in any
case, because if we don't need to include it in the image, then why do so?
It adds about 7KB to the loaded size of the vmlinux image for no gain
(on a typical tilegx configuration).

> --- 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);

This doesn't look right to me; we already have an __init_begin
symbol defined a few lines further down in vmlinux.lds.S.
How does this patch work instead?

On 07/01/2016 03:30 PM, Chris Metcalf wrote:
> On 6/10/2016 5:28 PM, Jason Baron wrote:
>> 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.
> 
> It seems plausible to me to only include the exit text in with the init
> text
> under an #ifdef CONFIG_JUMP_TABLE (with a suitable comment) in any
> case, because if we don't need to include it in the image, then why do so?
> It adds about 7KB to the loaded size of the vmlinux image for no gain
> (on a typical tilegx configuration).
> 
>> --- 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);
> 
> This doesn't look right to me; we already have an __init_begin
> symbol defined a few lines further down in vmlinux.lds.S.
> How does this patch work instead?
> 
> diff --git a/arch/tile/kernel/vmlinux.lds.S
> b/arch/tile/kernel/vmlinux.lds.S
> index 378f5d8d1ec8..5e83d2689def 100644
> --- a/arch/tile/kernel/vmlinux.lds.S
> +++ b/arch/tile/kernel/vmlinux.lds.S
> @@ -58,7 +58,15 @@ SECTIONS
>    _etext = .;
> 
>    /* "Init" is divided into two areas with very different virtual
> addresses. */
> -  INIT_TEXT_SECTION(PAGE_SIZE)
> +  . = ALIGN(PAGE_SIZE);
> +  .init.text : AT(ADDR(.init.text) - LOAD_OFFSET) {
> +    VMLINUX_SYMBOL(_sinittext) = .;
> +    INIT_TEXT
> +#ifdef CONFIG_JUMP_LABEL  /* __jump_table may reference __exit text */
> +    EXIT_TEXT
> +#endif
> +    VMLINUX_SYMBOL(_einittext) = .;
> +  }
> 
>    /* Now we skip back to PAGE_OFFSET for the data. */
>    . = (. - TEXT_OFFSET + PAGE_OFFSET);
> 


Hi Chris,

Thanks for the patch. I can confirm that it resolves the following
compilation issue:

`.exit.text' referenced in section `__jump_table' of fs/built-in.o:
defined in discarded section `.exit.text' of fs/built-in.o
`.exit.text' referenced in section `__jump_table' of fs/built-in.o:
defined in discarded section `.exit.text' of fs/built-in.o

I was able to create the issue by simply adding a
static_key_unlikely() branch to an __exit section that ends
up being built-in (non-module).

So this issue is really independent of this patch series...
Should I add your patch to my series when I re-post?

Thanks,

-Jason
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 7/5/2016 4:57 PM, Jason Baron wrote:
> On 07/01/2016 03:30 PM, Chris Metcalf wrote:
>> On 6/10/2016 5:28 PM, Jason Baron wrote:
>>> 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.
>> How does this patch work instead?
>>
>> diff --git a/arch/tile/kernel/vmlinux.lds.S
>> b/arch/tile/kernel/vmlinux.lds.S
>> index 378f5d8d1ec8..5e83d2689def 100644
>> --- a/arch/tile/kernel/vmlinux.lds.S
>> +++ b/arch/tile/kernel/vmlinux.lds.S
>> @@ -58,7 +58,15 @@ SECTIONS
>>     _etext = .;
>>
>>     /* "Init" is divided into two areas with very different virtual
>> addresses. */
>> -  INIT_TEXT_SECTION(PAGE_SIZE)
>> +  . = ALIGN(PAGE_SIZE);
>> +  .init.text : AT(ADDR(.init.text) - LOAD_OFFSET) {
>> +    VMLINUX_SYMBOL(_sinittext) = .;
>> +    INIT_TEXT
>> +#ifdef CONFIG_JUMP_LABEL  /* __jump_table may reference __exit text */
>> +    EXIT_TEXT
>> +#endif
>> +    VMLINUX_SYMBOL(_einittext) = .;
>> +  }
>>
>>     /* Now we skip back to PAGE_OFFSET for the data. */
>>     . = (. - TEXT_OFFSET + PAGE_OFFSET);
>>
>
> Hi Chris,
>
> Thanks for the patch. I can confirm that it resolves the following
> compilation issue:
>
> `.exit.text' referenced in section `__jump_table' of fs/built-in.o:
> defined in discarded section `.exit.text' of fs/built-in.o
> `.exit.text' referenced in section `__jump_table' of fs/built-in.o:
> defined in discarded section `.exit.text' of fs/built-in.o
>
> I was able to create the issue by simply adding a
> static_key_unlikely() branch to an __exit section that ends
> up being built-in (non-module).
>
> So this issue is really independent of this patch series...
> Should I add your patch to my series when I re-post?

I'm happy to just take a version of this into the tile tree instead, since
you're right, it's a pre-existing problem.  I'll post the revised change
shortly and cc you.  Thanks!