Re: [PATCH 1/3] tracing: Merge irqflags + preempt counter.

[Steven Rostedt]

On Wed, 13 Jan 2021 00:00:55 +0100
Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> wrote:

> The state of the interrupts (irqflags) and the preemption counter are
> both passed down to tracing_generic_entry_update(). Only one bit of
> irqflags is actually required: The on/off state. The complete 32bit
> of the preemption counter isn't needed. Just whether of the upper bits
> (softirq, hardirq and NMI) are set and the preemption depth is needed.
> 
> The irqflags and the preemption counter could be evaluated early and the
> information stored in an integer `trace_ctx'.
> tracing_generic_entry_update() would use the upper bits as the
> TRACE_FLAG_* and the lower 8bit as the disabled-preemption depth
> (considering that one must be substracted from the counter in one
> special cases).
> 
> The actual preemption value is not used except for the tracing record.
> The `irqflags' variable is mostly used only for the tracing record. An
> exception here is for instance wakeup_tracer_call() or
> probe_wakeup_sched_switch() which explicilty disable interrupts and use
> that `irqflags' to save (and restore) the IRQ state and to record the
> state.
> 
> Struct trace_event_buffer has also the `pc' and flags' members which can
> be replaced with `trace_ctx' since their actual value is not used
> outside of trace recording.
> 
> This will reduce tracing_generic_entry_update() to simply assign values
> to struct trace_entry. The evaluation of the TRACE_FLAG_* bits is moved
> to _tracing_gen_ctx_flags() which replaces preempt_count() and
> local_save_flags() invocations.
> 
> As an example, ftrace_syscall_enter() may invoke:
> - trace_buffer_lock_reserve() -> … -> tracing_generic_entry_update()
> - event_trigger_unlock_commit()
>   -> ftrace_trace_stack() -> … -> tracing_generic_entry_update()
>   -> ftrace_trace_userstack() -> … -> tracing_generic_entry_update()  
> 
> In this case the TRACE_FLAG_* bits were evaluated three times. By using
> the `trace_ctx' they are evaluated once and assigned three times.
> 
> A build with all tracers enabled on x86-64 with and without the patch:
> 
>     text     data      bss      dec      hex    filename
> 21970669 17084168  7639260 46694097  2c87ed1 vmlinux.old
> 21970293 17084168  7639260 46693721  2c87d59 vmlinux.new
> 
> text shrank by 379 bytes, data remained constant.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
>  include/linux/trace_events.h         |  25 +++-
>  kernel/trace/blktrace.c              |  17 +--
>  kernel/trace/trace.c                 | 208 ++++++++++++++-------------
>  kernel/trace/trace.h                 |  38 +++--
>  kernel/trace/trace_branch.c          |   6 +-
>  kernel/trace/trace_event_perf.c      |   5 +-
>  kernel/trace/trace_events.c          |  18 +--
>  kernel/trace/trace_events_inject.c   |   6 +-
>  kernel/trace/trace_functions.c       |  28 ++--
>  kernel/trace/trace_functions_graph.c |  32 ++---
>  kernel/trace/trace_hwlat.c           |   7 +-
>  kernel/trace/trace_irqsoff.c         |  86 +++++------
>  kernel/trace/trace_kprobe.c          |  10 +-
>  kernel/trace/trace_mmiotrace.c       |  14 +-
>  kernel/trace/trace_sched_wakeup.c    |  71 +++++----
>  kernel/trace/trace_syscalls.c        |  20 ++-
>  kernel/trace/trace_uprobe.c          |   4 +-
>  17 files changed, 287 insertions(+), 308 deletions(-)
> 
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index d321fe5ad1a14..aa9123878e126 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -148,17 +148,29 @@ enum print_line_t {
>  
>  enum print_line_t trace_handle_return(struct trace_seq *s);
>  
> -void tracing_generic_entry_update(struct trace_entry *entry,
> -				  unsigned short type,
> -				  unsigned long flags,
> -				  int pc);
> +static inline void tracing_generic_entry_update(struct trace_entry *entry,
> +						unsigned short type,
> +						unsigned int trace_ctx)
> +{
> +	struct task_struct *tsk = current;
> +
> +	entry->preempt_count		= trace_ctx & 0xff;
> +	entry->pid			= (tsk) ? tsk->pid : 0;
> +	entry->type			= type;
> +	entry->flags =			trace_ctx >> 16;
> +}
> +
> +unsigned int _tracing_gen_ctx_flags(unsigned long irqflags);
> +unsigned int tracing_gen_ctx_flags(void);
> +unsigned int tracing_gen_ctx_flags_dect(void);

Since I'm now trying to avoid underscore functions when possible, the above
should be:

_tracing_gen_ctx_flags()   --> tracing_gen_ctx_flags()
tracing_gen_ctx_flags()    --> tracing_gen_ctx()
tracing_gen_ctx_flags_dect --> tracing_gen_ctx_dec()

Note, "_dec()" should be used instead of "_dect()" as "_dec" is commonly
used around the kernel, and I had really no idea what "_dect" was, until I
looked at the two functions.

And looking at the implementation, I wonder if we should make this into
static inlines in the header as follows:

unsigned int tracing_gen_ctx_irq_test(int irqs_status);

#ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
static inline unsigned int tracing_gen_ctx_flags(unsigned long irqflags)
{
	unsigned int irq_status = irqs_disabled(irqflags) ?
		TRACE_FLAG_IRQS_OFF : 0;
	return tracing_gen_ctx_irq_test(irq_status);
}
static inline unsigned int tracing_gen_ctx(void)
{
	unsigned long irqflags;

	local_save_flags(irqflags);
	return tracing_gen_ctx_flags(irqflags);
}
#else
static inline unsigned int tracing_gen_ctx_flags(unsigned long irqflags)
{
	return tracing_gen_ctx_irq_test(TRACE_FLAG_IRQS_NOSUPPORT);
}
static inline unsigned int tracing_gen_ctx(void)
{
	return tracing_gen_ctx_irq_test(TRACE_FLAG_IRQS_NOSUPPORT);
}
#endif

static inline unsigned int tracing_gen_ctx_dec(void)
{
	unsigned int trace_ctx;

	trace_ctx = tracing_gen_ctx();
	/*** Comment about buffer_reserve here ***/
	if (IS_ENABLED(CONFIG_PREEMPTION))
		trace_ctx--;
	return trace_ctx;
}

Then all you need to do is implement the tracing_gen_ctx_irq_test() without
adding and #ifdefs in it, and just or in the "irq_status" to trace_flags,
without any conditionals.

-- Steve

> +
>  struct trace_event_file;
>  
>  struct ring_buffer_event *
>  trace_event_buffer_lock_reserve(struct trace_buffer **current_buffer,
>  				struct trace_event_file *trace_file,
>  				int type, unsigned long len,
> -				unsigned long flags, int pc);
> +				unsigned int trace_ctx);
>  
>  #define TRACE_RECORD_CMDLINE	BIT(0)
>  #define TRACE_RECORD_TGID	BIT(1)
> @@ -232,8 +244,7 @@ struct trace_event_buffer {
>  	struct ring_buffer_event	*event;
>  	struct trace_event_file		*trace_file;
>  	void				*entry;
> -	unsigned long			flags;
> -	int				pc;
> +	unsigned int			trace_ctx;
>  	struct pt_regs			*regs;
>  };
>