linux-kernel - Re: [RFC PATCH] tracing: Merge irqflags + preemt counter, add RT bits

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]

Message-ID: <20210112125823.gyrqof6frjtgccgm@linutronix.de>
Date:   Tue, 12 Jan 2021 13:58:23 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [RFC PATCH] tracing: Merge irqflags + preemt counter, add RT bits

On 2021-01-11 18:04:36 [-0500], Steven Rostedt wrote:
> > The actual preemption value is not used except for the tracing record.
> > The `irqflags' is also not used except for the _irqsave() locking in a
> > few spots.
> 
> Which spots?

Hmm. So I had memory of code sequences like
	spin_lock_irqsave(, flags);
	trace_buffer_lock_reserve(,, flags);

but I can't find them anymore. The only users of
_tracing_gen_ctx_flags() pass `0' as irqflags so I kept that instead
reading the actual irqfalgs.

> > As part of the patch I added __ to trace_event_buffer_commit() while
> > evaluating trace_event_buffer() for the struct trace_event_buffer usage
> > regarding the `pc' and `flags' members. It appears that those two can
> > also be merged into the `trace_ctx' integer.
> 
> Looks like you did change the trace_event_buffer. I don't understand why
> the "__" was added?

I added __ to trace_event_buffer_commit__() to ensure that I covered
every user and have not overseen one (which would result in a compile
error). This can can be reverted in the final submission.

> > -void
> > -tracing_generic_entry_update(struct trace_entry *entry, unsigned short type,
> > -			     unsigned long flags, int pc)
> > +static unsigned int __tracing_gen_ctx_flags(unsigned long irqflags)
> >  {
> > -	struct task_struct *tsk = current;
> > +	unsigned int trace_flags = 0;
> > +	unsigned int pc;
> > +
> > +	pc = preempt_count();
> >  
> > -	entry->preempt_count		= pc & 0xff;
> > -	entry->pid			= (tsk) ? tsk->pid : 0;
> > -	entry->type			= type;
> > -	entry->flags =
> >  #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
> > -		(irqs_disabled_flags(flags) ? TRACE_FLAG_IRQS_OFF : 0) |
> > +	if (irqs_disabled_flags(irqflags))
> > +		trace_flags |= TRACE_FLAG_IRQS_OFF;
> >  #else
> > -		TRACE_FLAG_IRQS_NOSUPPORT |
> > +		trace_flags |= TRACE_FLAG_IRQS_NOSUPPORT;
> >  #endif
> > -		((pc & NMI_MASK    ) ? TRACE_FLAG_NMI     : 0) |
> > -		((pc & HARDIRQ_MASK) ? TRACE_FLAG_HARDIRQ : 0) |
> > -		((pc & SOFTIRQ_OFFSET) ? TRACE_FLAG_SOFTIRQ : 0) |
> > -		(tif_need_resched() ? TRACE_FLAG_NEED_RESCHED : 0) |
> > -		(test_preempt_need_resched() ? TRACE_FLAG_PREEMPT_RESCHED : 0);
> 
> Note, the above was a result of playing around with seeing how the compiler
> optimized the above. Have you seen how the below logic looks as assembly?
> This is a very hot path.

So it does what it did before but differently in different spots.
tracing_generic_entry_update() itself got smaller since it has been
reduced to value assignment only. The trace flags computing changed /
moved to a different place.

The old tracing_generic_entry_update() went from 0x1da0 to 0x1e25 which
are 0x85 bytes. The new __tracing_gen_ctx_flags() (without any of the RT
bits) goes from 0x1f70 to 0x1ff1 which are 0x81 bytes (x86-64, gcc-10).

On x86 the preemption counter is read twice. Once due to preempt_count()
at the top and once due to tif_need_resched(). This did not change.
It reads the `current' pointer once due to test_preempt_need_resched().
Previously it was read twice as well, once due to
test_preempt_need_resched() and another time due to current->pid. The
pid assignment moved to the other function.
The old code seemed to `or' the results into `eax' before the
assignment. Now there are a few `cmove' instructions stretched across
the whole function and returned at the end.

While looking at it just now I don't understand why it checks for
current == NULL. I traced it back to commit
   72829bc3d63cd ("ftrace: move enums to ftrace.h and make helper function global")

and it might have been added by accident.

> > +
> > +	if (pc & NMI_MASK)
> > +		trace_flags |= TRACE_FLAG_NMI;
> > +	if (pc & HARDIRQ_MASK)
> > +		trace_flags |= TRACE_FLAG_HARDIRQ;
> > +
> > +	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
> > +		if (in_serving_softirq())
> > +			trace_flags |= TRACE_FLAG_SOFTIRQ;
> > +	} else {
> > +		if (pc & SOFTIRQ_OFFSET)
> > +			trace_flags |= TRACE_FLAG_SOFTIRQ;
> > +	}
> > +	if (tif_need_resched())
> > +		trace_flags |= TRACE_FLAG_NEED_RESCHED;
> > +	if (test_preempt_need_resched())
> > +		trace_flags |= TRACE_FLAG_PREEMPT_RESCHED;
> > +	return (trace_flags << 16) | (pc & 0xff);
> > +}
> 
> Can you break this into two patches. One that adds the trace_ctx and merges
> the flags and pc, and another patch that only updates to add the RT bits.

Sure, I just wanted you to see the whole picture.
Thank you.

> Thanks!
> 
> -- Steve

Sebastian