[PATCH] Fix memory order issue inside pv spinlock

[PATCH] Fix memory order issue inside pv spinlock

[Yang, Xiaowei]

barrier() can't prevent reads after it not being reordered with older writes to 
different locates before it. Because of it, I can't bring up > 4 HVM guests on 
one SMP machine. Use mb() instead.

Signed-off-by: Yang Xiaowei <xiaowei.yang@intel.com>

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 5601506..9dee5f8 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -324,7 +325,7 @@ static void xen_spin_unlock(struct raw_spinlock *lock)
         xl->lock = 0;           /* release lock */

         /* make sure unlock happens before kick */
-       barrier();
+       mb();

         if (unlikely(xl->spinners))
                 xen_spin_unlock_slow(xl);

Thanks,
Xiaowei

Re: [PATCH] Fix memory order issue inside pv spinlock

[Jeremy Fitzhardinge]

On 09/07/09 00:40, Yang, Xiaowei wrote:
> barrier() can't prevent reads after it not being reordered with older
> writes to different locates before it. Because of it, I can't bring up
> > 4 HVM guests on one SMP machine. Use mb() instead.

Which read is happening too early?  Is it "xl->spinners"?  How does it fail?

Thanks,
    J

>
> Signed-off-by: Yang Xiaowei <xiaowei.yang@intel.com>
>
> diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
> index 5601506..9dee5f8 100644
> --- a/arch/x86/xen/spinlock.c
> +++ b/arch/x86/xen/spinlock.c
> @@ -324,7 +325,7 @@ static void xen_spin_unlock(struct raw_spinlock
> *lock)
>         xl->lock = 0;           /* release lock */
>
>         /* make sure unlock happens before kick */
> -       barrier();
> +       mb();
>
>         if (unlikely(xl->spinners))
>                 xen_spin_unlock_slow(xl);
>
> Thanks,
> Xiaowei
>

Re: [PATCH] Fix memory order issue inside pv spinlock

[Yang, Xiaowei]

Jeremy Fitzhardinge wrote:
> On 09/07/09 00:40, Yang, Xiaowei wrote:
>> barrier() can't prevent reads after it not being reordered with older
>> writes to different locates before it. Because of it, I can't bring up
>>> 4 HVM guests on one SMP machine. Use mb() instead.
> 
> Which read is happening too early?  Is it "xl->spinners"?  How does it fail?

Yes. If read of xl->spinners happens earlier than write 0 to xl->lock, 
notifications to wake up other spinners can be omitted incorrectly, resulting in 
others polling indefinitely (because of poll evtchn not pending) with the lock 
is uncontended.

Thanks,
xiaowei

> 
> Thanks,
>     J
> 
>> Signed-off-by: Yang Xiaowei <xiaowei.yang@intel.com>
>>
>> diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
>> index 5601506..9dee5f8 100644
>> --- a/arch/x86/xen/spinlock.c
>> +++ b/arch/x86/xen/spinlock.c
>> @@ -324,7 +325,7 @@ static void xen_spin_unlock(struct raw_spinlock
>> *lock)
>>         xl->lock = 0;           /* release lock */
>>
>>         /* make sure unlock happens before kick */
>> -       barrier();
>> +       mb();
>>
>>         if (unlikely(xl->spinners))
>>                 xen_spin_unlock_slow(xl);
>>
>> Thanks,
>> Xiaowei
>>
> 

Re: [PATCH] Fix memory order issue inside pv spinlock

[Jeremy Fitzhardinge]

On 09/09/09 09:30, Yang, Xiaowei wrote:
> Jeremy Fitzhardinge wrote:
>> On 09/07/09 00:40, Yang, Xiaowei wrote:
>>> barrier() can't prevent reads after it not being reordered with older
>>> writes to different locates before it. Because of it, I can't bring up
>>>> 4 HVM guests on one SMP machine. Use mb() instead.
>>
>> Which read is happening too early?  Is it "xl->spinners"?  How does
>> it fail?
>
> Yes. If read of xl->spinners happens earlier than write 0 to xl->lock,
> notifications to wake up other spinners can be omitted incorrectly,
> resulting in others polling indefinitely (because of poll evtchn not
> pending) with the lock is uncontended.

OK.  And the CPU only guarantees that, without explicit barriers,
write-read ordering is only maintained between accesses to the same
memory location, not separate locations?

    J

Re: [PATCH] Fix memory order issue inside pv spinlock

[Yang, Xiaowei]

Jeremy Fitzhardinge wrote:

> On 09/09/09 09:30, Yang, Xiaowei wrote:
>> Jeremy Fitzhardinge wrote:
>>> On 09/07/09 00:40, Yang, Xiaowei wrote:
>>>> barrier() can't prevent reads after it not being reordered with older
>>>> writes to different locates before it. Because of it, I can't bring up
>>>>> 4 HVM guests on one SMP machine. Use mb() instead.
>>> Which read is happening too early?  Is it "xl->spinners"?  How does
>>> it fail?
>> Yes. If read of xl->spinners happens earlier than write 0 to xl->lock,
>> notifications to wake up other spinners can be omitted incorrectly,
>> resulting in others polling indefinitely (because of poll evtchn not
>> pending) with the lock is uncontended.
> 
> OK.  And the CPU only guarantees that, without explicit barriers,
> write-read ordering is only maintained between accesses to the same
> memory location, not separate locations?
> 

Exactly! For more details please refer to Chapter 8.2 of Intel SDM 3A.

Thanks,
Xiaowei

Re: [PATCH] Fix memory order issue inside pv spinlock

[Jeremy Fitzhardinge]

On 09/09/09 18:41, Yang, Xiaowei wrote:
> Jeremy Fitzhardinge wrote:
>
>> On 09/09/09 09:30, Yang, Xiaowei wrote:
>>> Jeremy Fitzhardinge wrote:
>>>> On 09/07/09 00:40, Yang, Xiaowei wrote:
>>>>> barrier() can't prevent reads after it not being reordered with older
>>>>> writes to different locates before it. Because of it, I can't
>>>>> bring up
>>>>>> 4 HVM guests on one SMP machine. Use mb() instead.
>>>> Which read is happening too early?  Is it "xl->spinners"?  How does
>>>> it fail?
>>> Yes. If read of xl->spinners happens earlier than write 0 to xl->lock,
>>> notifications to wake up other spinners can be omitted incorrectly,
>>> resulting in others polling indefinitely (because of poll evtchn not
>>> pending) with the lock is uncontended.
>>
>> OK.  And the CPU only guarantees that, without explicit barriers,
>> write-read ordering is only maintained between accesses to the same
>> memory location, not separate locations?
>>
>
> Exactly! For more details please refer to Chapter 8.2 of Intel SDM 3A.

Thanks.  I just submitted both fixes to upstream.

    J