Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 6 Nov 2019 20:15:25 +0100 (CET)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Eric Dumazet <edumazet@google.com>
cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Eric Dumazet <eric.dumazet@gmail.com>,
        syzbot <syzkaller@googlegroups.com>
Subject: Re: [PATCH] hrtimer: Annotate lockless access to timer->state
In-Reply-To: <CANn89iK12QGBagUiNr+j-ToawJ9J1behtySyL9vLattYPAD-7w@mail.gmail.com>
Message-ID: <alpine.DEB.2.21.1911062002490.1869@nanos.tec.linutronix.de>
References: <20191106174804.74723-1-edumazet@google.com> <alpine.DEB.2.21.1911061908070.1869@nanos.tec.linutronix.de> <CANn89iK12QGBagUiNr+j-ToawJ9J1behtySyL9vLattYPAD-7w@mail.gmail.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 6 Nov 2019, Eric Dumazet wrote:
> On Wed, Nov 6, 2019 at 10:09 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Wed, 6 Nov 2019, Eric Dumazet wrote:
> > > @@ -1013,8 +1013,9 @@ static void __remove_hrtimer(struct hrtimer *timer,
> > >  static inline int
> > >  remove_hrtimer(struct hrtimer *timer, struct hrtimer_clock_base *base, bool restart)
> > >  {
> > > -     if (hrtimer_is_queued(timer)) {
> > > -             u8 state = timer->state;
> > > +     u8 state = timer->state;
> >
> > Shouldn't that be a read once then at least for consistency sake?
> 
> We own the lock here, this is not really needed ?
> 
> Note they are other timer->state reads I chose to leave unchanged.
> 
> But no big deal if you prefer I can add a READ_ONCE()

Nah. I can add it myself if at all.

I know that the READ_ONCE() is not required there, but I really hate to
twist my brain when staring at code why some places use it and some not.

So at least some commentry helps to avoid that. Something like the
below. If you have no objections I just queue the patch with this folded
in.

Thanks,

	tglx

8<-------------
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -456,12 +456,18 @@ extern u64 hrtimer_next_event_without(co
 
 extern bool hrtimer_active(const struct hrtimer *timer);
 
-/*
- * Helper function to check, whether the timer is on one of the queues
+/**
+ * hrtimer_is_queued = check, whether the timer is on one of the queues
+ * @timer:	Timer to check
+ *
+ * Returns: True if the timer is queued, false otherwise
+ *
+ * The function can be used lockless, but it gives only a momentary snapshot.
  */
-static inline int hrtimer_is_queued(struct hrtimer *timer)
+static inline bool hrtimer_is_queued(struct hrtimer *timer)
 {
-	return READ_ONCE(timer->state) & HRTIMER_STATE_ENQUEUED;
+	/* The READ_ONCE pairs with the update functions of timer->state */
+	return !!READ_ONCE(timer->state) & HRTIMER_STATE_ENQUEUED;
 }
 
 /*
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -966,6 +966,7 @@ static int enqueue_hrtimer(struct hrtime
 
 	base->cpu_base->active_bases |= 1 << base->index;
 
+	/* Pairs with the lockless read in hrtimer_is_queued() */
 	WRITE_ONCE(timer->state, HRTIMER_STATE_ENQUEUED);
 
 	return timerqueue_add(&base->active, &timer->node);
@@ -988,6 +989,7 @@ static void __remove_hrtimer(struct hrti
 	struct hrtimer_cpu_base *cpu_base = base->cpu_base;
 	u8 state = timer->state;
 
+	/* Pairs with the lockless read in hrtimer_is_queued() */
 	WRITE_ONCE(timer->state, newstate);
 	if (!(state & HRTIMER_STATE_ENQUEUED))
 		return;