linux-kernel - Re: [PATCH] tracing: uninline trace_trigger_soft

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d60a60ff-cc16-75b0-2415-482cb70a2e5e@csgroup.eu>
Date:   Thu, 10 Feb 2022 15:05:51 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Steven Rostedt <rostedt@...dmis.org>
CC:     Ingo Molnar <mingo@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH] tracing: uninline trace_trigger_soft_disabled()



Le 10/02/2022 à 15:26, Steven Rostedt a écrit :
> On Thu, 10 Feb 2022 09:47:52 +0100
> Christophe Leroy <christophe.leroy@...roup.eu> wrote:
> 
>> On a ppc32 build with CONFIG_CC_OPTIMISE_FOR_SIZE,
>> trace_trigger_soft_disabled() appears more than 50 times in vmlinux.
>>
>> That function is rather big for an inlined function, and
>> it doesn't benefit much from inlining as its only parameter
>> is a pointer to a struct in memory:
> 
> The number of parameters is not the reason for it being inlined. It's in a
> *very* hot path, and a function call causes a noticeable performance hit.

Fair enough

> 
> 
>>
>>
>> It doesn't benefit much from inlining as its only parameter is a
>> pointer to a struct in memory so no constant folding is involved.
>>
>> Uninline it and move it into kernel/trace/trace_events_trigger.c
>>
>> It reduces the size of vmlinux by approximately 10 kbytes.
> 
> If you have an issue with the size, perhaps the function can be modified to
> condense it. I'm happy to have a size reduction, but I will NACK making it
> into a function call.
> 

Well, my first issue is that I get it duplicated 50 times because GCC 
find it too big to get inlined. So it is a function call anyway.

For instance, when building arch/powerpc/kernel/irq.o which -Winline, I get:

./include/linux/perf_event.h:1169:20: error: inlining failed in call to 
'perf_fetch_caller_regs': call is unlikely and code size would grow 
[-Werror=inline]
./include/linux/perf_event.h:1169:20: error: inlining failed in call to 
'perf_fetch_caller_regs': call is unlikely and code size would grow 
[-Werror=inline]
./include/linux/perf_event.h:1169:20: error: inlining failed in call to 
'perf_fetch_caller_regs': call is unlikely and code size would grow 
[-Werror=inline]
./include/linux/perf_event.h:1169:20: error: inlining failed in call to 
'perf_fetch_caller_regs': call is unlikely and code size would grow 
[-Werror=inline]
./include/linux/trace_events.h:712:1: error: inlining failed in call to 
'trace_trigger_soft_disabled': call is unlikely and code size would grow 
[-Werror=inline]
./include/linux/trace_events.h:712:1: error: inlining failed in call to 
'trace_trigger_soft_disabled': call is unlikely and code size would grow 
[-Werror=inline]
./include/linux/trace_events.h:712:1: error: inlining failed in call to 
'trace_trigger_soft_disabled': call is unlikely and code size would grow 
[-Werror=inline]
./include/linux/trace_events.h:712:1: error: inlining failed in call to 
'trace_trigger_soft_disabled': call is unlikely and code size would grow 
[-Werror=inline]



If having it a function call is really an issue, then it should be 
__always_inline

I will see the impact on size when we do so.


What is in the hot path really ? It is the entire function or only the 
first test ?

What about:

static inline bool
trace_trigger_soft_disabled(struct trace_event_file *file)
{
	unsigned long eflags = file->flags;

	if (!(eflags & EVENT_FILE_FL_TRIGGER_COND))
		return outlined_trace_trigger_soft_disabled(...);
	return false;
}


Or is there more in the hot path ?

Thanks
Christophe