On Tue, 28 Jul 2020 at 13:30, Ingo Molnar <[email protected]> wrote: > > > * Marco Elver <[email protected]> wrote: > > > To improve the general usefulness of the IRQ state trace information > > with KCSAN enabled, save and restore the trace information when entering > > and exiting the KCSAN runtime as well as when generating a KCSAN report. > > > > Without this, reporting the IRQ state trace (whether via a KCSAN report > > or outside of KCSAN via a lockdep report) is rather useless due to > > continuously being touched by KCSAN. This is because if KCSAN is > > enabled, every instrumented memory access causes changes to IRQ state > > tracking information (either by KCSAN disabling/enabling interrupts or > > taking report_lock when generating a report). > > > > Before "lockdep: Prepare for NMI IRQ state tracking", KCSAN avoided > > touching the IRQ state trace via raw_local_irq_save/restore() and > > lockdep_off/on(). > > > > Fixes: 248591f5d257 ("kcsan: Make KCSAN compatible with new IRQ state > > tracking") > > Signed-off-by: Marco Elver <[email protected]> > > --- [...] > > +void kcsan_restore_irqtrace(struct task_struct *task) > > +{ > > +#ifdef CONFIG_TRACE_IRQFLAGS > > + task->irq_events = task->kcsan_save_irqtrace.irq_events; > > + task->hardirq_enable_ip = task->kcsan_save_irqtrace.hardirq_enable_ip; > > + task->hardirq_disable_ip = > > task->kcsan_save_irqtrace.hardirq_disable_ip; > > + task->hardirq_enable_event = > > task->kcsan_save_irqtrace.hardirq_enable_event; > > + task->hardirq_disable_event = > > task->kcsan_save_irqtrace.hardirq_disable_event; > > + task->softirq_disable_ip = > > task->kcsan_save_irqtrace.softirq_disable_ip; > > + task->softirq_enable_ip = task->kcsan_save_irqtrace.softirq_enable_ip; > > + task->softirq_disable_event = > > task->kcsan_save_irqtrace.softirq_disable_event; > > + task->softirq_enable_event = > > task->kcsan_save_irqtrace.softirq_enable_event; > > +#endif > > Please, make such type of assignment blocks cleaner by using a local > helper variable, and by aligning the right side vertically as well. > > Also, would it make sense to unify the layout between the fields in > task struct and the new one you introduced? That would allow a simple > structure copy.
Makes sense, thanks for the suggestion. I think we could introduce a new struct 'irqtrace_events'. I currently have something that adds this struct in <linux/irqtrace.h>. AFAIK it also adds readability improvements on initialization and use of the fields. I'll send a v2 with 2 patches. Thanks, -- Marco
