[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZyFKUU1LpFfLrVXb@elver.google.com>
Date: Tue, 29 Oct 2024 21:49:21 +0100
From: Marco Elver <elver@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>,
"Paul E. McKenney" <paulmck@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Mark Rutland <mark.rutland@....com>,
Dmitry Vyukov <dvyukov@...gle.com>, kasan-dev@...glegroups.com,
linux-kernel@...r.kernel.org,
Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH] kcsan, seqlock: Support seqcount_latch_t
On Tue, Oct 29, 2024 at 02:46PM +0100, Peter Zijlstra wrote:
> On Tue, Oct 29, 2024 at 02:05:38PM +0100, Marco Elver wrote:
> > On Tue, 29 Oct 2024 at 12:49, Peter Zijlstra <peterz@...radead.org> wrote:
> > >
> > > On Tue, Oct 29, 2024 at 09:36:29AM +0100, Marco Elver wrote:
> > > > Reviewing current raw_write_seqcount_latch() callers, the most common
> > > > patterns involve only few memory accesses, either a single plain C
> > > > assignment, or memcpy;
> > >
> > > Then I assume you've encountered latch_tree_{insert,erase}() in your
> > > travels, right?
> >
> > Oops. That once certainly exceeds the "8 memory accesses".
> >
> > > Also, I note that update_clock_read_data() seems to do things
> > > 'backwards' and will completely elide your proposed annotation.
> >
> > Hmm, for the first access, yes. This particular oddity could be
> > "fixed" by surrounding the accesses by
> > kcsan_nestable_atomic_begin/end(). I don't know if it warrants adding
> > a raw_write_seqcount_latch_begin().
> >
> > Preferences?
>
> I *think* it is doable to flip it around to the 'normal' order, but
> given I've been near cross-eyed with a head-ache these past two days,
> I'm not going to attempt a patch for you, since I'm bound to get it
> wrong :/
Something like this?
------ >8 ------
Author: Marco Elver <elver@...gle.com>
Date: Tue Oct 29 21:16:21 2024 +0100
time/sched_clock: Swap update_clock_read_data() latch writes
Swap the writes to the odd and even copies to make the writer critical
section look like all other seqcount_latch writers.
With that, we can also add the raw_write_seqcount_latch_end() to clearly
denote the end of the writer section.
Signed-off-by: Marco Elver <elver@...gle.com>
diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index 68d6c1190ac7..311c90a0e86e 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -119,9 +119,6 @@ unsigned long long notrace sched_clock(void)
*/
static void update_clock_read_data(struct clock_read_data *rd)
{
- /* update the backup (odd) copy with the new data */
- cd.read_data[1] = *rd;
-
/* steer readers towards the odd copy */
raw_write_seqcount_latch(&cd.seq);
@@ -130,6 +127,11 @@ static void update_clock_read_data(struct clock_read_data *rd)
/* switch readers back to the even copy */
raw_write_seqcount_latch(&cd.seq);
+
+ /* update the backup (odd) copy with the new data */
+ cd.read_data[1] = *rd;
+
+ raw_write_seqcount_latch_end(&cd.seq);
}
/*
------ >8 ------
I also noticed your d16317de9b41 ("seqlock/latch: Provide
raw_read_seqcount_latch_retry()") to get rid of explicit instrumentation
in noinstr.
Not sure how to resolve that. We have that objtool support to erase
calls in noinstr code (is_profiling_func), but that's x86 only.
I could also make kcsan_atomic_next(0) noinstr compatible by checking if
the ret IP is in noinstr, and immediately return if it is.
Preferences?
Powered by blists - more mailing lists