[Linux-ia64] spin_unlock() problem

[Linux-ia64] spin_unlock() problem

[Jes Sorensen]

Hi,

I have been tracing a problem with tty->count hitting an unidenfied
state and I am starting to ponder if our current spin_unlock()
implementation is sufficient.

Currently the spin_unlock() implementation looks like this:

#define spin_unlock(x)		do { barrier(); ((spinlock_t *) x)->lock = 0;} while (0)

barrier() doesn't guarantee memory ordering, in other words, we are not
guaranteed that writes have been flushed to physical memory on exit. Now
Jesse pointed out to me that spin_lock() uses aquire semantics which
should take care of this, however this is only the case if the other CPU
grabs a spin lock before reading the variable we wrote while holding the
lock.

Consider the following example:

cpu1()
{
   spin_lock(&bleh);
   *a = foo;
   *b = bar;
   spin_unlock(&bleh);
}

cpu2()
{
   if (*b = bar)
      boink(*a);
}

With our weak memory ordering, b might have been written back to memory
while a still hasn't made it out. Or am I missing something here?

The question is, shouldn't our spin_unlock() implementation call wmb()
instead of barrier()? I noticed Alpha calls mb() in their spin_unlock()
implementation.

Cheers,
Jes

Re: [Linux-ia64] spin_unlock() problem

[Jes Sorensen]

>>>>> "Jes" = Jes Sorensen <jes@wildopensource.com> writes:

Jes> Hi, I have been tracing a problem with tty->count hitting an
Jes> unidenfied state and I am starting to ponder if our current
Jes> spin_unlock() implementation is sufficient.

[snip]

Jes> Consider the following example:

Actually that example isn't valid, this one should be:

cpu1()
{
   spin_lock(&bleh);
   *a = foo;
   spin_unlock(&bleh);
   *b = bar;
}

cpu2()
{
   if (*b = bar)
      boink(*a);
}

Jes

RE: [Linux-ia64] spin_unlock() problem

[Van Maren, Kevin]

Hi Jes,

I think I have a decent understanding, but every time I start to
think that my brain explodes on another case...

> I have been tracing a problem with tty->count hitting an unidenfied
> state and I am starting to ponder if our current spin_unlock()
> implementation is sufficient.
>
> Currently the spin_unlock() implementation looks like this:

> #define spin_unlock(x)  do { barrier(); ((spinlock_t *) x)->lock
= 0;} while (0)

> barrier() doesn't guarantee memory ordering, in other words, we are not
> guaranteed that writes have been flushed to physical memory on exit. Now
> Jesse pointed out to me that spin_lock() uses aquire semantics which
> should take care of this, however this is only the case if the other CPU
> grabs a spin lock before reading the variable we wrote while holding the
> lock.

Yes, I think spin_unlock should have release semantics.
barrier() is used to prevent the compiler from reordering loads/stores,
but does not stop the processor from doing so.

...

> With our weak memory ordering, b might have been written back to memory
> while a still hasn't made it out. Or am I missing something here?

> The question is, shouldn't our spin_unlock() implementation call wmb()
> instead of barrier()? I noticed Alpha calls mb() in their spin_unlock()
> implementation.

The first example you gave certainly should work (but may not due to
the missing release on unlock); the second one you gave is NOT guaranteed
to work without you changing the code: the store to b can pass
the unlock, even with release semantics, and hence also the store to a,
(although it cannot pass the spin_lock).  (Note: if you use a wbm,
it will work, since that implements fence semantics instead of just release
semantics)

Kevin

RE: [Linux-ia64] spin_unlock() problem

[Van Maren, Kevin]

Bleh.  I should have _looked at_ the code:

=
Consider the following example:

cpu1()
{
   spin_lock(&bleh);
   *a = foo;
   *b = bar;
   spin_unlock(&bleh);
}

cpu2()
{
   if (*b = bar)
      boink(*a);
}

=
No, this will not work: if you need it to work, you need to add a wmb
between the store to a and the store to b -- because cpu2 is NOT
acquiring "bleh", so it CAN see the writes out of order.  If you
were using "bleh" "properly" (ie, always acquiring it to access a/b)
then yes, it should work because the lock/unlock enforce memory
consistency.

Kevin

Re: [Linux-ia64] spin_unlock() problem

[Jes Sorensen]

>>>>> "Kevin" = Van Maren, Kevin <kevin.vanmaren@unisys.com> writes:

Kevin> Bleh.  I should have _looked at_ the code: =[snip]
Kevin> No, this will not work: if you need it to work, you need to add
Kevin> a wmb between the store to a and the store to b -- because cpu2
Kevin> is NOT acquiring "bleh", so it CAN see the writes out of order.
Kevin> If you were using "bleh" "properly" (ie, always acquiring it to
Kevin> access a/b) then yes, it should work because the lock/unlock
Kevin> enforce memory consistency.

Hi Kevin,

The first example I sent out was clearly bogus, please ignore it. I am
still pondering the second one. Jack pointed out in private email that
the release semantics of setting a volatible variable should take care
of it, but I am still trying to grasp it.

Now where did I leave my stash of aspirin.

Cheers,
Jes

Re: [Linux-ia64] spin_unlock() problem

[David Mosberger]

>>>>> On Thu, 3 Apr 2003 23:51:04 -0500, Jes Sorensen <jes@wildopensource.com> said:

  Jes> Hi, I have been tracing a problem with tty->count hitting an
  Jes> unidenfied state and I am starting to ponder if our current
  Jes> spin_unlock() implementation is sufficient.

  Jes> Currently the spin_unlock() implementation looks like this:

  Jes> #define spin_unlock(x) do { barrier(); ((spinlock_t *) x)->lock
  Jes> = 0;} while (0)

  Jes> barrier() doesn't guarantee memory ordering, in other words, we
  Jes> are not guaranteed that writes have been flushed to physical
  Jes> memory on exit.

Note that "lock" is declared as "volatile", so clearing it will use
"release" semantics.  (Since I know you have a copy of the ia64 linux
kernel book: this is described in detail starting on page 302).

	--david

Re: [Linux-ia64] spin_unlock() problem

[David Mosberger]

>>>>> On 04 Apr 2003 10:13:25 -0500, Jes Sorensen <jes@wildopensource.com> said:
  Jes> The first example I sent out was clearly bogus, please ignore
  Jes> it. I am still pondering the second one. Jack pointed out in
  Jes> private email that the release semantics of setting a volatible
  Jes> variable should take care of it, but I am still trying to grasp
  Jes> it.

In your example:

cpu1()
{
   spin_lock(&bleh);
   *a = foo;
   spin_unlock(&bleh);
   *b = bar;
}

cpu2()
{
   if (*b = bar)
      boink(*a);
}

*a is guaranteed to have the value "foo" when *b observes bar.
spin_unlock() has release semantics and that ensures that all
subsequent (in program order) memory operations are observed after the
store with release semantics.

	--david

Re: [Linux-ia64] spin_unlock() problem

[Jes Sorensen]

>>>>> "David" = David Mosberger <davidm@napali.hpl.hp.com> writes:

David> *a is guaranteed to have the value "foo" when *b observes bar.
David> spin_unlock() has release semantics and that ensures that all
David> subsequent (in program order) memory operations are observed
David> after the store with release semantics.

Hi David,

Back to getting a sore head from this one ;-(

Thanks for the book reference, I went and read it and then went on to
the Intel IA-64 Architecture Software Developer's Manual, vol #1. I
only have an old version 1.1 dated July 2000, so it's possible that
there is an updated version out with further clarifications.

Quoting section 4.4.7 "Memory Access Ordering", 2nd paragraph, it says
" .... Release data accesses guarantee that all previous data accesses
are made visible prior to being made visible themselves." The way I
understand that, it is possible that a store performed after an st.rel
may become visible prior to the st.rel, like in this example:

  st [r2] = r32
  st.rel [r3] = r33
  st [r4] = r34

In other words we are only guarantied that [r2] is valid when [r3]
appears but have no guarantie that [r4] doesn't show up on the bus
prior to [r3]?

Sorry for continuing to pester you about this one, it's the only
explanation I can find for the problems some people have been seeing
for tty->count getting messed up in some cases (it is always modified
while holding a spin lock).

Cheers,
Jes

RE: [Linux-ia64] spin_unlock() problem

[Luck, Tony]

> Thanks for the book reference, I went and read it and then went on to
> the Intel IA-64 Architecture Software Developer's Manual, vol #1. I
> only have an old version 1.1 dated July 2000, so it's possible that
> there is an updated version out with further clarifications.

Newer versions of the manuals can be found online at developer.intel.com
(follow links: h/w design > processors > server processors > Itanium family")

In particular you might be interested in "A Formal Specification of Intel®
Itanium® Processor Family Memory Ordering"

http://developer.intel.com/design/itanium/downloads/251429.htm

[But it might just make your head hurt even more].

-Tony Luck

Re: [Linux-ia64] spin_unlock() problem

[David Mosberger]

>>>>> On 07 Apr 2003 18:09:44 -0400, Jes Sorensen <jes@wildopensource.com> said:

  Jes> Quoting section 4.4.7 "Memory Access Ordering", 2nd paragraph,
  Jes> it says " .... Release data accesses guarantee that all
  Jes> previous data accesses are made visible prior to being made
  Jes> visible themselves."

Oops, sorry, I got it exactly backwards. ;-(
So much for giving a "quick" reply...

  Jes> The way I understand that, it is possible that a store
  Jes> performed after an st.rel may become visible prior to the
  Jes> st.rel, like in this example:

  Jes>   st [r2] = r32		(1)
  Jes>   st.rel [r3] = r33	(2)
  Jes>   st [r4] = r34		(3)

  Jes> In other words we are only guarantied that [r2] is valid when
  Jes> [r3] appears but have no guarantie that [r4] doesn't show up on
  Jes> the bus prior to [r3]?

I wouldn't use the word "valid" here, but yes, (2) and (3) are NOT
ordered.

	--david

Re: [Linux-ia64] spin_unlock() problem

[Jes Sorensen]

>>>>> "David" = David Mosberger <davidm@napali.hpl.hp.com> writes:

>>>>> On 07 Apr 2003 18:09:44 -0400, Jes Sorensen <jes@wildopensource.com> said:
David> Oops, sorry, I got it exactly backwards. ;-( So much for giving
David> a "quick" reply...

Heh, for a quick answer you sure were very convincing. I have
convinced myself for and against this one several times so far ;-)

Jes> In other words we are only guarantied that [r2] is valid when
Jes> [r3] appears but have no guarantie that [r4] doesn't show up on
Jes> the bus prior to [r3]?

David> I wouldn't use the word "valid" here, but yes, (2) and (3) are
David> NOT ordered.

This is the situation I was trying to fix, adding a wmb() to
spin_unlock() seems the only way to get around it as far as I can see.
I take it you agree then?

Cheers,
Jes

RE: [Linux-ia64] spin_unlock() problem

[Jim Hull]

David wrote:

> Oops, sorry, I got it exactly backwards. ;-(
> So much for giving a "quick" reply...

Right.  In Jes's example:

cpu1()
{
   spin_lock(&bleh);
   *a = foo;
   spin_unlock(&bleh);
   *b = bar;
}

cpu2()
{
   if (*b = bar)
      boink(*a);
}

For cpu1(), there are 2 ways to guarantee the store to *b is made
visible after to the store to *a:

1) Execute an "mf" instruction between them (probably via a change to
the spin_unlock macro), or

2) Make sure the store to *b is a release store (probably by making its
type "volatile").

There's also a problem in cpu2().  At present, there's nothing
guaranteeing the ordering of the loads of *b and *a.  Again, there are 2
ways to guarantee this:

1) Add an explicit "mf" between them (no simple change to a spin macro
will help here), or

2) Make sure the load of *b is an acquire load (probably by making its
type "volatile").

Note that change number (2), making *b volatile, fixes both problems, so
it is what I would recommend.  Of course this is probably a much harder
change, perhaps to architecture-independent code, instead of just
"fixing" an architecture-dependent macro, but it is the only thing that
will fix both halves of the problem.

 -- Jim

RE: [Linux-ia64] spin_unlock() problem

[Chen, Kenneth W]

I'm confused with the original example:

cpu1()
{
   spin_lock(&bleh);
   *a = foo;
   *b = bar;
   spin_unlock(&bleh);
}

cpu2()
{
   if (*b == bar)
      boink(*a);
}

*b is protected by spin_lock bleh, then in cpu2() one need a spin_lock
to access *b.  To me, the code above has bug in it.


Then the discussion flows into following example:

cpu1()
{
   spin_lock(&bleh);
   *a = foo;
   spin_unlock(&bleh);
   *b = bar;
}

cpu2()
{
   if (*b == bar)
      boink(*a);
}

Which also doesn't gareentee the order of *b because it is outside a
spin_lock and there is no explicit memory ordering in the code.

To make it to work correctly, I think one needs something like the
following:
--- a   Mon Apr  7 16:34:51 2003
+++ b   Mon Apr  7 16:35:11 2003
@@ -3,11 +3,11 @@
    spin_lock(&bleh);
    *a = foo;
    spin_unlock(&bleh);
-   *b = bar;
+   REL_SEMANTICS(*b) = bar;
 }
 
 cpu2()
 {
-   if (*b == bar)
+   if (ACQ_SEMANTICS(*b) == bar)
       boink(*a);
 }

Again, this is a program bug to me.

- Ken

-----Original Message-----
From: Jes Sorensen [mailto:jes@wildopensource.com] 
Sent: Monday, April 07, 2003 4:14 PM
To: davidm@hpl.hp.com
Cc: 'linux-ia64@linuxia64.org '; 'wildos@sgi.com '
Subject: Re: [Linux-ia64] spin_unlock() problem

>>>>> "David" == David Mosberger <davidm@napali.hpl.hp.com> writes:

>>>>> On 07 Apr 2003 18:09:44 -0400, Jes Sorensen
<jes@wildopensource.com> said:
David> Oops, sorry, I got it exactly backwards. ;-( So much for giving
David> a "quick" reply...

Heh, for a quick answer you sure were very convincing. I have
convinced myself for and against this one several times so far ;-)

Jes> In other words we are only guarantied that [r2] is valid when
Jes> [r3] appears but have no guarantie that [r4] doesn't show up on
Jes> the bus prior to [r3]?

David> I wouldn't use the word "valid" here, but yes, (2) and (3) are
David> NOT ordered.

This is the situation I was trying to fix, adding a wmb() to
spin_unlock() seems the only way to get around it as far as I can see.
I take it you agree then?

Cheers,
Jes

_______________________________________________
Linux-IA64 mailing list
Linux-IA64@linuxia64.org
http://lists.linuxia64.org/lists/listinfo/linux-ia64

Re: [Linux-ia64] spin_unlock() problem

[David Mosberger]

>>>>> On 07 Apr 2003 19:13:40 -0400, Jes Sorensen <jes@wildopensource.com> said:

  Jes> This is the situation I was trying to fix, adding a wmb() to
  Jes> spin_unlock() seems the only way to get around it as far as I can see.
  Jes> I take it you agree then?

As Jim pointed out, there is an anologous problem on the read-side.
Declaring the pointer "volatile" fixes the problem for ia64 and is
arguably the right (logical) thing to do anyhow.

However, it won't fix the problem for Alpha though: there you'd need
to add rmb() between the two reads on cpu2() (read_barrier_depends()
only guarantees ordering for data-dependent reads, so it won't work
here).

So, the most portable way to fix this is to add a wmb() on the side of
cpu1() and rmb() on the read side.

	--david

RE: [Linux-ia64] spin_unlock() problem

[David Mosberger]

>>>>> On Mon, 7 Apr 2003 16:38:03 -0700, "Chen, Kenneth W" <kenneth.w.chen@intel.com> said:

  Ken> To make it to work correctly, I think one needs something like the
  Ken> following:
  Ken> --- a   Mon Apr  7 16:34:51 2003
  Ken> +++ b   Mon Apr  7 16:35:11 2003
  Ken> @@ -3,11 +3,11 @@
  Ken> spin_lock(&bleh);
  Ken> *a = foo;
  Ken> spin_unlock(&bleh);
  Ken> -   *b = bar;
  Ken> +   REL_SEMANTICS(*b) = bar;
  Ken> }

  Ken> cpu2()
  Ken> {
  Ken> -   if (*b = bar)
  Ken> +   if (ACQ_SEMANTICS(*b) = bar)
  Ken> boink(*a);
  Ken> }

Which is exactly what "volatile" would do.  Unfortunately, this
"trick" works only on ia64, so it does not solve the problem for all
Linux platforms.

	--david