Re: [PATCH] tracing: uninline trace_trigger_soft_disabled()

[Steven Rostedt]

On Thu, 10 Feb 2022 15:05:51 +0000
Christophe Leroy <christophe.leroy@xxxxxxxxxx> 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