lkml.org 
[lkml]   [2019]   [Nov]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] hrtimer: Annotate lockless access to timer->state
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;
\
 
 \ /
  Last update: 2019-11-06 20:15    [W:0.062 / U:1.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site