Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200720120348.2406588-1-elver@google.com> <20200728113044.GA233444@gmail.com>
In-Reply-To: <20200728113044.GA233444@gmail.com>
From:   Marco Elver <elver@google.com>
Date:   Tue, 28 Jul 2020 17:14:21 +0200
Message-ID: <CANpmjNOMQ09N4+W9Dt53j=GKAuj0Sd__agRtqpDkubZEkULAJg@mail.gmail.com>
Subject: Re: [PATCH tip/locking/core] kcsan: Improve IRQ state trace reporting
To:     Ingo Molnar <mingo@kernel.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Borislav Petkov <bp@alien8.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Dmitry Vyukov <dvyukov@google.com>,
        kasan-dev <kasan-dev@googlegroups.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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