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;
MIME-Version: 1.0
References: <20191106174804.74723-1-edumazet@google.com> <alpine.DEB.2.21.1911061908070.1869@nanos.tec.linutronix.de>
 <CANn89iK12QGBagUiNr+j-ToawJ9J1behtySyL9vLattYPAD-7w@mail.gmail.com> <alpine.DEB.2.21.1911062002490.1869@nanos.tec.linutronix.de>
In-Reply-To: <alpine.DEB.2.21.1911062002490.1869@nanos.tec.linutronix.de>
From:   Eric Dumazet <edumazet@google.com>
Date:   Wed, 6 Nov 2019 12:00:42 -0800
Message-ID: <CANn89i+adxqR+L7ArTQhOmvryO0++ajf1FHt7bQcCnUkEkgu+g@mail.gmail.com>
Subject: Re: [PATCH] hrtimer: Annotate lockless access to timer->state
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Eric Dumazet <eric.dumazet@gmail.com>,
        syzbot <syzkaller@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Nov 6, 2019 at 11:15 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> 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.


This looks good to me, thanks !

>
> 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;