Messages in this thread | | | Date | Tue, 18 Feb 2020 19:01:44 -0500 | From | Joel Fernandes <> | Subject | Re: [PATCH tip/core/rcu 1/3] rcu-tasks: *_ONCE() for rcu_tasks_cbs_head |
| |
On Tue, Feb 18, 2020 at 02:54:55PM -0800, Paul E. McKenney wrote: > On Tue, Feb 18, 2020 at 05:45:03PM -0500, Steven Rostedt wrote: > > On Tue, 18 Feb 2020 12:22:26 -0800 > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > On Tue, Feb 18, 2020 at 09:11:42PM +0100, Peter Zijlstra wrote: > > > > On Tue, Feb 18, 2020 at 08:27:19AM -0800, Paul E. McKenney wrote: > > > > > On Tue, Feb 18, 2020 at 08:56:48AM +0100, Peter Zijlstra wrote: > > > > > > > > > > I just took offence at the Changelog wording. It seems to suggest there > > > > > > actually is a problem, there is not. > > > > > > > > > > Quoting the changelog: "Not appropriate for backporting due to failure > > > > > being unlikely." > > > > > > > > That implies there is failure, however unlikely. > > > > > > > > In this particular case there is absolutely no failure, except perhaps > > > > in KCSAN. This patch is a pure annotation such that KCSAN can understand > > > > the code. > > > > > > > > Like said, I don't object to the actual patch, but I do think it is > > > > important to call out false negatives or to describe the actual problem > > > > found. > > > > > > I don't feel at all comfortable declaring that there is absolutely > > > no possibility of failure. > > > > Perhaps wording it like so: > > > > "There's know known issue with the current code, but the *_ONCE() > > annotations here makes KCSAN happy, allowing us to focus on KCSAN > > warnings that can help bring about known issues in other code that we > > can fix, without being distracted by KCSAN warnings that we do not see > > a problem with." > > > > ? > > That sounds more like something I might put in rcutodo.html as a statement > of the RCU approach to KCSAN reports. > > But switching to a different situation (for variety, if nothing else), > what about the commit shown below? > > Thanx, Paul > > ------------------------------------------------------------------------ > > commit 35bc02b04a041f32470ae6d959c549bcce8483db > Author: Paul E. McKenney <paulmck@kernel.org> > Date: Tue Feb 18 13:41:02 2020 -0800 > > rcutorture: Mark data-race potential for rcu_barrier() test statistics > > The n_barrier_successes, n_barrier_attempts, and > n_rcu_torture_barrier_error variables are updated (without access > markings) by the main rcu_barrier() test kthread, and accessed (also > without access markings) by the rcu_torture_stats() kthread. This of > course can result in KCSAN complaints. > > Because the accesses are in diagnostic prints, this commit uses > data_race() to excuse the diagnostic prints from the data race. If this > were to ever cause bogus statistics prints (for example, due to store > tearing), any misleading information would be disambiguated by the > presence or absence of an rcutorture splat. > > This data race was reported by KCSAN. Not appropriate for backporting > due to failure being unlikely and due to the mild consequences of the > failure, namely a confusing rcutorture console message. > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c > index 5453bd5..b3301f3 100644 > --- a/kernel/rcu/rcutorture.c > +++ b/kernel/rcu/rcutorture.c > @@ -1444,9 +1444,9 @@ rcu_torture_stats_print(void) > atomic_long_read(&n_rcu_torture_timers)); > torture_onoff_stats(); > pr_cont("barrier: %ld/%ld:%ld\n", > - n_barrier_successes, > - n_barrier_attempts, > - n_rcu_torture_barrier_error); > + data_race(n_barrier_successes), > + data_race(n_barrier_attempts), > + data_race(n_rcu_torture_barrier_error));
Would it be not worth just fixing the data-race within rcutorture itself?
thanks,
- Joel
| |