ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

[shan kang]

Hello,
   I wonder why smp_mb() is not needed in the "__mutex_fastpath_lock"
and "__mutex_fastpath_unlock" functions which are located in the
"arch/arm/include/asm/mutex.h"?
   I think "dmb" instruction is necessary there.

ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

[shan kang]

Hello,
   I wonder why smp_mb() is not needed in the "__mutex_fastpath_lock"
and "__mutex_fastpath_unlock" functions which are located in the
"arch/arm/include/asm/mutex.h"?
   I think "dmb" instruction is necessary there.

Re: ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

[Li Haifeng]

Hi Shan,

2012/7/12 shan kang <kangshan0910@gmail.com>:
> Hello,
>    I wonder why smp_mb() is not needed in the "__mutex_fastpath_lock"
> and "__mutex_fastpath_unlock" functions which are located in the
> "arch/arm/include/asm/mutex.h"?
>    I think "dmb" instruction is necessary there.

Why necessary? Could you explain more detailed?

>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

[Li Haifeng]

Hi Shan,

2012/7/12 shan kang <kangshan0910@gmail.com>:
> Hello,
>    I wonder why smp_mb() is not needed in the "__mutex_fastpath_lock"
> and "__mutex_fastpath_unlock" functions which are located in the
> "arch/arm/include/asm/mutex.h"?
>    I think "dmb" instruction is necessary there.

Why necessary? Could you explain more detailed?

>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

[shan kang]

For example, in the following scenario, Process2  may get the wrong value;
Process1:
mutex_lock(&lock);
write data; (store operation)
mutex_unlock(&lock);

Process2:
mutex_lock(&lock);
read data; (load operation)
mutex_unlock(&lock);

Suppose Process1 gets the lock first, write data and unlock. If the
store operation completes very slowly, the load operation of the
Process2 will fail to get the new value.
Since there are no dmb instructions in the mutex_lock and
mutex_unlock, which doesn't make sure that after Process2 gets the
lock, the result of the Process1's store operation will be seen by the
Process2.



2012/7/13 Li Haifeng <omycle@gmail.com>:
> Hi Shan,
>
> 2012/7/12 shan kang <kangshan0910@gmail.com>:
>> Hello,
>>    I wonder why smp_mb() is not needed in the "__mutex_fastpath_lock"
>> and "__mutex_fastpath_unlock" functions which are located in the
>> "arch/arm/include/asm/mutex.h"?
>>    I think "dmb" instruction is necessary there.
>
> Why necessary? Could you explain more detailed?
>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

[shan kang]

For example, in the following scenario, Process2  may get the wrong value;
Process1:
mutex_lock(&lock);
write data; (store operation)
mutex_unlock(&lock);

Process2:
mutex_lock(&lock);
read data; (load operation)
mutex_unlock(&lock);

Suppose Process1 gets the lock first, write data and unlock. If the
store operation completes very slowly, the load operation of the
Process2 will fail to get the new value.
Since there are no dmb instructions in the mutex_lock and
mutex_unlock, which doesn't make sure that after Process2 gets the
lock, the result of the Process1's store operation will be seen by the
Process2.



2012/7/13 Li Haifeng <omycle@gmail.com>:
> Hi Shan,
>
> 2012/7/12 shan kang <kangshan0910@gmail.com>:
>> Hello,
>>    I wonder why smp_mb() is not needed in the "__mutex_fastpath_lock"
>> and "__mutex_fastpath_unlock" functions which are located in the
>> "arch/arm/include/asm/mutex.h"?
>>    I think "dmb" instruction is necessary there.
>
> Why necessary? Could you explain more detailed?
>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

[Will Deacon]

On Fri, Jul 13, 2012 at 10:10:52AM +0100, shan kang wrote:
> For example, in the following scenario, Process2  may get the wrong value;
> Process1:
> mutex_lock(&lock);
> write data; (store operation)
> mutex_unlock(&lock);
> 
> Process2:
> mutex_lock(&lock);
> read data; (load operation)
> mutex_unlock(&lock);

Yes, it looks like we can screw things up in the uncontended case (where
nobody blocks on the mutex). We could add an smp_mb after the lock operation
and another one before the unlock, but I'm tempted just to use
asm-generic/mutex-dec.h instead. The latter approach will subtly change the
current behaviour, so I'll post a patch when I'm happy with it.

Curious: did you find this by inspection or did you observe it going wrong?

Cheers,

Will

ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

[Will Deacon]

On Fri, Jul 13, 2012 at 10:10:52AM +0100, shan kang wrote:
> For example, in the following scenario, Process2  may get the wrong value;
> Process1:
> mutex_lock(&lock);
> write data; (store operation)
> mutex_unlock(&lock);
> 
> Process2:
> mutex_lock(&lock);
> read data; (load operation)
> mutex_unlock(&lock);

Yes, it looks like we can screw things up in the uncontended case (where
nobody blocks on the mutex). We could add an smp_mb after the lock operation
and another one before the unlock, but I'm tempted just to use
asm-generic/mutex-dec.h instead. The latter approach will subtly change the
current behaviour, so I'll post a patch when I'm happy with it.

Curious: did you find this by inspection or did you observe it going wrong?

Cheers,

Will

Re: ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

[shan kang]

Yes, a customer reported an issue to me.
After my investigation, I found the problem may be the lack of "dmb"
in mutex_lock and mutex_unlock functions.
Moreover, the issue could be resolved after the "dmb" was added.

2012/7/13 Will Deacon <will.deacon@arm.com>:
> On Fri, Jul 13, 2012 at 10:10:52AM +0100, shan kang wrote:
>> For example, in the following scenario, Process2  may get the wrong value;
>> Process1:
>> mutex_lock(&lock);
>> write data; (store operation)
>> mutex_unlock(&lock);
>>
>> Process2:
>> mutex_lock(&lock);
>> read data; (load operation)
>> mutex_unlock(&lock);
>
> Yes, it looks like we can screw things up in the uncontended case (where
> nobody blocks on the mutex). We could add an smp_mb after the lock operation
> and another one before the unlock, but I'm tempted just to use
> asm-generic/mutex-dec.h instead. The latter approach will subtly change the
> current behaviour, so I'll post a patch when I'm happy with it.
>
> Curious: did you find this by inspection or did you observe it going wrong?
>
> Cheers,
>
> Will

ARM: why smp_mb() is not needed in the "__mutex_fastpath_lock" and "__mutex_fastpath_unlock" functions

[shan kang]

Yes, a customer reported an issue to me.
After my investigation, I found the problem may be the lack of "dmb"
in mutex_lock and mutex_unlock functions.
Moreover, the issue could be resolved after the "dmb" was added.

2012/7/13 Will Deacon <will.deacon@arm.com>:
> On Fri, Jul 13, 2012 at 10:10:52AM +0100, shan kang wrote:
>> For example, in the following scenario, Process2  may get the wrong value;
>> Process1:
>> mutex_lock(&lock);
>> write data; (store operation)
>> mutex_unlock(&lock);
>>
>> Process2:
>> mutex_lock(&lock);
>> read data; (load operation)
>> mutex_unlock(&lock);
>
> Yes, it looks like we can screw things up in the uncontended case (where
> nobody blocks on the mutex). We could add an smp_mb after the lock operation
> and another one before the unlock, but I'm tempted just to use
> asm-generic/mutex-dec.h instead. The latter approach will subtly change the
> current behaviour, so I'll post a patch when I'm happy with it.
>
> Curious: did you find this by inspection or did you observe it going wrong?
>
> Cheers,
>
> Will