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
| ||
|
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
Powered by blists - more mailing lists