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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 10 Feb 2022 11:54:22 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
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()

On Thu, 10 Feb 2022 15:05:51 +0000
Christophe Leroy <christophe.leroy@...roup.eu> wrote:

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

Yes you are correct.

> 
> 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 ?

Actually, the condition should be:

 	if (!(eflags & EVENT_FILE_FL_TRIGGER_COND) &&
	    (eflags & (EVENT_FILE_FL_TRIGGER_MODE |
		       EVENT_FILE_FL_SOFT_DISABLE |
		       EVENT_FILE_FL_PID_FILTER)) {
		return outlined_trace_trigger_soft_disabled(...);
	}

	return false;

As we only want to call the function when TRIGGER_COND is not set and one
of those bits are. Because the most common case is !eflags, which your
version would call the function every time.

Maybe even switch the condition, to the most common case:

 	if ((eflags & (EVENT_FILE_FL_TRIGGER_MODE |
		       EVENT_FILE_FL_SOFT_DISABLE |
		       EVENT_FILE_FL_PID_FILTER) &&
	    !(eflags & EVENT_FILE_FL_TRIGGER_COND)) {

Because if one of those bits are not set, no reason to look further. And
the TRIGGER_COND being set is actually the unlikely case, so it should be
checked last.

Would that work for you?

-- Steve