This is the generic portion of lockless gettimeofday. It defines frlock
and changes locking of xtime_lock from rwlock to frlock.
On Tue, Jan 28, 2003 at 03:42:21PM -0800, Stephen Hemminger wrote:
> +static inline void fr_write_begin(frlock_t *rw)
> +{
> + preempt_disable();
> + rw->pre_sequence++;
> + wmb();
> +}
> +
> +static inline void fr_write_end(frlock_t *rw)
> +{
> + wmb();
> + rw->post_sequence++;
These need to be mb(), not wmb(), if you want the bits in between
to actually happen in between, as with your xtime example. At
present there's nothing stoping xtime from being *read* before
your read from pre_sequence happens.
r~
> These need to be mb(), not wmb(), if you want the bits in between
> to actually happen in between, as with your xtime example. At
> present there's nothing stoping xtime from being *read* before
> your read from pre_sequence happens.
But with frlocks we synchronise writers with a spinlock, so shouldnt it
provide that synchronisation?
Anton
Anton Blanchard <[email protected]> wrote:
>
>
> > These need to be mb(), not wmb(), if you want the bits in between
> > to actually happen in between, as with your xtime example. At
> > present there's nothing stoping xtime from being *read* before
> > your read from pre_sequence happens.
>
> But with frlocks we synchronise writers with a spinlock, so shouldnt it
> provide that synchronisation?
>
Richard is referring to the new fr_write_begin/end code, which doesn't take a
spinlock because it assumes that writer serialisation has been provided by
external means.
On Tue, 2003-01-28 at 23:06, Richard Henderson wrote:
> On Tue, Jan 28, 2003 at 03:42:21PM -0800, Stephen Hemminger wrote:
> > +static inline void fr_write_begin(frlock_t *rw)
> > +{
> > + preempt_disable();
> > + rw->pre_sequence++;
> > + wmb();
> > +}
> > +
> > +static inline void fr_write_end(frlock_t *rw)
> > +{
> > + wmb();
> > + rw->post_sequence++;
>
> These need to be mb(), not wmb(), if you want the bits in between
> to actually happen in between, as with your xtime example. At
> present there's nothing stoping xtime from being *read* before
> your read from pre_sequence happens.
First, write_begin/end can only be safely used when there is separate
writer synchronization such as a spin_lock or semaphore.
As far as I know, semaphore or spin_lock guarantees a barrier.
So xtime or anything else can not be read before the spin_lock.
Using mb() is more paranoid than necessary.
On Wed, Jan 29, 2003 at 05:15:55PM -0800, Stephen Hemminger wrote:
> On Tue, 2003-01-28 at 23:06, Richard Henderson wrote:
> > On Tue, Jan 28, 2003 at 03:42:21PM -0800, Stephen Hemminger wrote:
> > > +static inline void fr_write_begin(frlock_t *rw)
> > > +{
> > > + preempt_disable();
> > > + rw->pre_sequence++;
> > > + wmb();
> > > +}
> > > +
> > > +static inline void fr_write_end(frlock_t *rw)
> > > +{
> > > + wmb();
> > > + rw->post_sequence++;
> >
> > These need to be mb(), not wmb(), if you want the bits in between
> > to actually happen in between, as with your xtime example. At
> > present there's nothing stoping xtime from being *read* before
> > your read from pre_sequence happens.
>
>
> First, write_begin/end can only be safely used when there is separate
> writer synchronization such as a spin_lock or semaphore.
> As far as I know, semaphore or spin_lock guarantees a barrier.
> So xtime or anything else can not be read before the spin_lock.
>
> Using mb() is more paranoid than necessary.
yes, it should only generate a superflous lock on x86.
it shouldn't even be necessary in fr_write_trylock.
Andrea
On Wed, Jan 29, 2003 at 05:15:55PM -0800, Stephen Hemminger wrote:
> First, write_begin/end can only be safely used when there is separate
> writer synchronization such as a spin_lock or semaphore.
> As far as I know, semaphore or spin_lock guarantees a barrier.
> So xtime or anything else can not be read before the spin_lock.
>
> Using mb() is more paranoid than necessary.
If you want stuff to happen *between* the write_begin/end, or
indeed for the begin/end not to be interleaved, then mb() is
absolutely necessary. The most likely dynamic reordering of
//begin
t1 = rw->pre_sequence
t1 += 1
rw->pre_sequence = t1
wmb()
//stuff
xtimensec = xtime.tv_nsec
//end
wmb()
t2 = rw->post_sequence
t2 += 1
rw->post_sequence = t2
is
t1 = rw->pre_sequence
t2 = rw->post_sequence
xtimensec = xtime.tv_nsec
t1 += 1;
t2 += 2;
rw->pre_sequence = t1
wmb()
wmb()
rw->post_sequence = t2
Why? Because pre_sequence and post_sequence are in the same
cache line, and both reads could be satisfied in the same
cycle by the same line fill from main memory.
If you don't care about stuff happening in between the
write_begin/end, then why are you using them at all?
r~
On Wed, Jan 29, 2003 at 05:41:33PM -0800, Richard Henderson wrote:
> //begin
> t1 = rw->pre_sequence
> t1 += 1
> rw->pre_sequence = t1
> wmb()
>
> //stuff
> xtimensec = xtime.tv_nsec
>
> //end
> wmb()
> t2 = rw->post_sequence
> t2 += 1
> rw->post_sequence = t2
>
> is
>
> t1 = rw->pre_sequence
> t2 = rw->post_sequence
> xtimensec = xtime.tv_nsec
> t1 += 1;
> t2 += 2;
> rw->pre_sequence = t1
> wmb()
> wmb()
> rw->post_sequence = t2
No it's:
t1 = rw->pre_sequence
t2 = rw->post_sequence
t1 += 1;
t2 += 2;
rw->pre_sequence = t1
wmb()
xtimensec = xtime.tv_nsec
wmb()
rw->post_sequence = t2
you're missing xtimensec is a write.
or this if you prefer:
spin_lock() / now xtime can't change under us
t1 = rw->pre_sequence
t2 = rw->post_sequence
t3 = xtime.tv_nsec
t1 += 1;
t2 += 2;
rw->pre_sequence = t1
wmb()
xtimensec = t3
wmb()
rw->post_sequence = t2
spin_unlock() / now xtime can change again
and the above is the optimal implementation of the write-side. We
definitely don't want to forbid those reoderings. if gcc or cpu thinks
it's worthwhile they must be allowed to optimize it since it's legal.
I believe wmb() is correct, and mb() is overkill.
Andrea
On Thu, Jan 30, 2003 at 02:52:19AM +0100, Andrea Arcangeli wrote:
> you're missing xtimensec is a write.
Eh? xtimensec is a register.
r~
On Thu, Jan 30, 2003 at 04:41:20PM -0800, Richard Henderson wrote:
> On Thu, Jan 30, 2003 at 02:52:19AM +0100, Andrea Arcangeli wrote:
> > you're missing xtimensec is a write.
>
> Eh? xtimensec is a register.
then it's fine, the register will be flushed to ram eventually and it
will be within the two wmb(), just write in the example also the line
where the xtimensec ""register"" is flushed to ram and it will be in the
right place
if the register isn't flushed to ram eventually, it will be discared and
the whole critical section is a noop from the point of view of the other
cpus and no wmb() or rmb() or mb() would be needed in the first place
Andrea