Re: [PATCH tip/locking/core] kcsan: Improve IRQ state trace reporting

[Marco Elver]

On Tue, 28 Jul 2020 at 13:30, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>
> * Marco Elver <elver@xxxxxxxxxx> 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 <elver@xxxxxxxxxx>
> > ---
[...]
> > +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