lkml.org 
[lkml]   [2014]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 54/55] timekeeping: Provide fast and NMI safe access to CLOCK_MONOTONIC[_RAW]
    ----- Original Message -----
    > From: "Thomas Gleixner" <tglx@linutronix.de>
    > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
    > Cc: "LKML" <linux-kernel@vger.kernel.org>, "John Stultz" <john.stultz@linaro.org>, "Peter Zijlstra"
    > <peterz@infradead.org>, "Steven Rostedt" <rostedt@goodmis.org>
    > Sent: Saturday, July 12, 2014 4:04:59 PM
    > Subject: Re: [patch 54/55] timekeeping: Provide fast and NMI safe access to CLOCK_MONOTONIC[_RAW]
    >
    > On Sat, 12 Jul 2014, Thomas Gleixner wrote:
    > > On Sat, 12 Jul 2014, Mathieu Desnoyers wrote:
    > > > I'm perhaps missing something here, but what happens with the
    > > > following scenario ?
    > > >
    > > > Initial conditions:
    > > >
    > > > tkf->seq = 0
    > > > tkf->base[0] and tkf->base[1] are initialized.
    > > >
    > > > CPU 0 CPU 1
    > > > ------------ ----------------
    > > > update:
    > > > tkf->seq++
    > > > smb_wmb()
    > > > tkf->seq++ (reordered before update)
    > > > reader:
    > > > seq = tkf->seq (reads 2)
    > > > smp_rmb()
    > > > idx = seq & 0x01
    > > > now = now(tkf->base[idx]
    > > > (reads base[0])
    > > > update(tkf->base[0], tk) (racy concurrent update)
    > > > smp_rmb()
    > > > while (seq != tkf->seq) (they
    > > > are equal)
    > > >
    > > > So AFAIU, we end up returning a corrupted value. Adding a
    > > > smp_wmb() between update of base[0] and increment of seq,
    > > > as well as between update of base[1] and the _following_
    > > > increment of seq (next update call) would fix this.
    > > >
    > > > Thoughts ?
    >
    > Second thoughts :)
    >
    > > Well, the actual implementation does:
    > >
    > > + /* Force readers off to base[1] */
    > > + raw_write_seqcount_begin(&tkf->seq);
    >
    > i.e:
    > seq++;
    > smp_wmb();
    >
    > > +
    > > + /* Update base[0] */
    > > + base->clock = clk;
    > > + base->cycle_last = clk->cycle_last;
    > > + base->base = tbase;
    > > + base->shift = shift;
    > > + base->mult = mult;
    > > +
    > > + /* Force readers back to base[0] */
    > > + raw_write_seqcount_end(&tkf->seq);
    >
    > i.e:
    > smp_wmb();
    > seq++;
    >
    > So while this orders against the update of base0, it does not prevent
    > reordering against the update of base1. So you're right, we need a
    >
    > smp_wmb();
    >
    > before we start updating base1.
    >
    > > + /* Update base[1] */
    > > + base++;
    > > + base->clock = clk;
    > > + base->cycle_last = clk->cycle_last;
    > > + base->base = tbase;
    > > + base->shift = shift;
    > > + base->mult = mult;
    >
    > So as a consequence we need another one here:
    >
    > smp_wmb();
    >
    > to protect against the unlikely, but possible seq++ at the begin of
    > the update. Debatable whether this can happen without another wmb()
    > between the two calls, but yes for sanity reasons we should add it
    > until we can prove that the actual call chains prevent this.
    >
    > Nice catch!

    Thanks! Yep, the barriers you propose are what appears
    to be missing,

    Mathieu

    >
    > tglx
    >

    --
    Mathieu Desnoyers
    EfficiOS Inc.
    http://www.efficios.com


    \
     
     \ /
      Last update: 2014-07-12 23:21    [W:4.170 / U:0.340 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site