Messages in this thread | | | Date | Tue, 12 Jan 2021 13:58:23 +0100 | From | Sebastian Andrzej Siewior <> | 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
| |