Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1288873 > unrolled thread
| Started by | David Daney <ddaney@caviumnetworks.com> |
|---|---|
| First post | 2015-12-10 20:50 +0100 |
| Last post | 2015-12-11 11:00 +0100 |
| Articles | 20 on this page of 22 — 8 participants |
Back to article view | Back to linux.kernel
Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) David Daney <ddaney@caviumnetworks.com> - 2015-12-10 20:50 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Andrew Pinski <pinskia@gmail.com> - 2015-12-11 04:30 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Andrew Pinski <andrew.pinski@caviumnetworks.com> - 2015-12-11 06:00 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Peter Zijlstra <peterz@infradead.org> - 2015-12-11 09:50 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Will Deacon <will.deacon@arm.com> - 2015-12-11 13:10 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Peter Zijlstra <peterz@infradead.org> - 2015-12-11 13:20 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Will Deacon <will.deacon@arm.com> - 2015-12-11 13:20 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Peter Zijlstra <peterz@infradead.org> - 2015-12-11 13:30 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Will Deacon <will.deacon@arm.com> - 2015-12-11 14:40 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Peter Zijlstra <peterz@infradead.org> - 2015-12-11 14:50 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Will Deacon <will.deacon@arm.com> - 2015-12-11 15:10 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Peter Zijlstra <peterz@infradead.org> - 2015-12-11 18:20 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Will Deacon <will.deacon@arm.com> - 2015-12-11 18:30 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-12-11 23:40 +0100
Re: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2015-12-14 20:00 +0100
Re: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Peter Zijlstra <peterz@infradead.org> - 2015-12-14 21:40 +0100
Re: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-12-15 07:20 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Peter Zijlstra <peterz@infradead.org> - 2015-12-14 21:30 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-12-15 05:40 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Davidlohr Bueso <dave@stgolabs.net> - 2015-12-11 15:20 +0100
Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Peter Zijlstra <peterz@infradead.org> - 2015-12-11 08:40 +0100
Re: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) Will Deacon <will.deacon@arm.com> - 2015-12-11 11:00 +0100
Page 1 of 2 [1] 2 Next page →
| From | David Daney <ddaney@caviumnetworks.com> |
|---|---|
| Date | 2015-12-10 20:50 +0100 |
| Subject | Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEfnY-247-23@gated-at.bofh.it> |
Hi,
We are getting soft lockup OOPs on Cavium CN88XX (A.K.A. ThunderX),
which is an arm64 implementation.
A typical failure shows multiple threads stuck in mutex operations like
this:
.
.
.
[ 68.909873] Task dump for CPU 18:
[ 68.909876] systemd-udevd R running task 0 537 534
0x00000002
[ 68.909877] Call trace:
[ 68.909880] [<fffffe0000088858>] dump_backtrace+0x0/0x17c
[ 68.909883] [<fffffe00000889f8>] show_stack+0x24/0x2c
[ 68.909885] [<fffffe00000c4210>] sched_show_task+0xb0/0x104
[ 68.909888] [<fffffe00000c682c>] dump_cpu_task+0x48/0x54
[ 68.909890] [<fffffe00000ee5e0>] rcu_dump_cpu_stacks+0x9c/0xec
[ 68.909893] [<fffffe00000f2c9c>] rcu_check_callbacks+0x524/0xa18
[ 68.909896] [<fffffe00000f83a0>] update_process_times+0x44/0x74
[ 68.909899] [<fffffe00001078d4>] tick_sched_timer+0x78/0x1ac
[ 68.909901] [<fffffe00000f8b74>] __hrtimer_run_queues+0x148/0x2d4
[ 68.909903] [<fffffe00000f9464>] hrtimer_interrupt+0xb0/0x1f4
[ 68.909906] [<fffffe000056e6e8>] arch_timer_handler_phys+0x3c/0x48
[ 68.909909] [<fffffe00000e7fd4>] handle_percpu_devid_irq+0xb0/0x1b0
[ 68.909912] [<fffffe00000e33c4>] generic_handle_irq+0x34/0x4c
[ 68.909914] [<fffffe00000e3738>] __handle_domain_irq+0x90/0xfc
[ 68.909916] [<fffffe0000081d80>] gic_handle_irq+0x90/0x18c
[ 68.909918] Exception stack(0xfffffe03f14e3920 to 0xfffffe03f14e3a40)
[ 68.909921] 3920: fffffe03fd5c5800 fffffe0000c55800 fffffe03f14e3a80
fffffe00000dabd8
[ 68.909924] 3940: 00000000a0000145 0000000000000015 fffffe03e9602400
fffffe00002fddb0
[ 68.909927] 3960: 0000000000000000 0000000000000000 fffffe03fd5c5810
fffffe03f14e0000
[ 68.909929] 3980: 0000000000000001 ffffffffff000000 fffffe03db307e38
0000000000000000
[ 68.909932] 39a0: 0000000000737973 00000000ffffffff 0000000000000000
000000003b364d50
[ 68.909935] 39c0: 0000000000000018 ffffffffa99641af 0016fd71b6000000
003b9aca00000000
[ 68.909937] 39e0: fffffe00001f1508 000003ff9b9fd028 000003ffed7a0a10
fffffe03fd5c5800
[ 68.909940] 3a00: fffffe0000c55800 fffffe0000cea1c8 fffffe03fd5a5800
fffffe0000ca2eb0
[ 68.909943] 3a20: 0000000000000015 fffffe03e9602400 fffffe0000cea1c8
fffffe0000712000
[ 68.909945] [<fffffe0000084ce8>] el1_irq+0x68/0xd8
[ 68.909948] [<fffffe00000da03c>] mutex_optimistic_spin+0x9c/0x1d0
[ 68.909951] [<fffffe00006fe4b8>] __mutex_lock_slowpath+0x44/0x158
[ 68.909953] [<fffffe00006fe620>] mutex_lock+0x54/0x58
[ 68.909956] [<fffffe0000265efc>] kernfs_iop_permission+0x38/0x70
[ 68.909959] [<fffffe00001fbf50>] __inode_permission+0x88/0xd8
[ 68.909961] [<fffffe00001fbfd0>] inode_permission+0x30/0x6c
[ 68.909964] [<fffffe00001fe26c>] link_path_walk+0x68/0x4d4
[ 68.909966] [<fffffe00001ffa14>] path_openat+0xb4/0x2bc
[ 68.909968] [<fffffe000020123c>] do_filp_open+0x74/0xd0
[ 68.909971] [<fffffe00001f13e4>] do_sys_open+0x14c/0x228
[ 68.909973] [<fffffe00001f1544>] SyS_openat+0x3c/0x48
[ 68.909976] [<fffffe00000851f0>] el0_svc_naked+0x24/0x28
.
.
.
Reverting 81a43adae3b9 (locking/mutex: Use acquire/release semantics)
Makes the problem go away.
At this point it is unknown if this patch is incorrect, or if the
underlying ARM64 atomic_*_{acquire,release} primitives are defective, or
if the problem lies elsewhere.
I am not requesting any specific action with this e-mail, but wanted to
draw attention to the issue. Undoubtedly we will be able to provide
more detailed information about the issue in the coming days.
Thanks,
David Daney
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Andrew Pinski <pinskia@gmail.com> |
|---|---|
| Date | 2015-12-11 04:30 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEmz8-6ZR-27@gated-at.bofh.it> |
| In reply to | #1288873 |
On Thu, Dec 10, 2015 at 11:44 AM, David Danny wrote:
>
> Hi,
>
> We are getting soft lockup OOPs on Cavium CN88XX (A.K.A. ThunderX), which is an arm64 implementation.
I get a slightly different OOPs and reverting
c55a6ffa6285e29f874ed403979472631ec70bff I was able to boot.
What I saw with osq_lock.c was that osq_wait_next is called for both
lock and unlock case so it might need both barriers.
The other question comes does atomic_cmpxchg_release have release
semantics when the compare fails? Right now it does not.
Thanks,
Andrew
>
> A typical failure shows multiple threads stuck in mutex operations like
> this:
>
> .
> .
> .
> [ 68.909873] Task dump for CPU 18:
> [ 68.909876] systemd-udevd R running task 0 537 534
> 0x00000002
> [ 68.909877] Call trace:
> [ 68.909880] [<fffffe0000088858>] dump_backtrace+0x0/0x17c
> [ 68.909883] [<fffffe00000889f8>] show_stack+0x24/0x2c
> [ 68.909885] [<fffffe00000c4210>] sched_show_task+0xb0/0x104
> [ 68.909888] [<fffffe00000c682c>] dump_cpu_task+0x48/0x54
> [ 68.909890] [<fffffe00000ee5e0>] rcu_dump_cpu_stacks+0x9c/0xec
> [ 68.909893] [<fffffe00000f2c9c>] rcu_check_callbacks+0x524/0xa18
> [ 68.909896] [<fffffe00000f83a0>] update_process_times+0x44/0x74
> [ 68.909899] [<fffffe00001078d4>] tick_sched_timer+0x78/0x1ac
> [ 68.909901] [<fffffe00000f8b74>] __hrtimer_run_queues+0x148/0x2d4
> [ 68.909903] [<fffffe00000f9464>] hrtimer_interrupt+0xb0/0x1f4
> [ 68.909906] [<fffffe000056e6e8>] arch_timer_handler_phys+0x3c/0x48
> [ 68.909909] [<fffffe00000e7fd4>] handle_percpu_devid_irq+0xb0/0x1b0
> [ 68.909912] [<fffffe00000e33c4>] generic_handle_irq+0x34/0x4c
> [ 68.909914] [<fffffe00000e3738>] __handle_domain_irq+0x90/0xfc
> [ 68.909916] [<fffffe0000081d80>] gic_handle_irq+0x90/0x18c
> [ 68.909918] Exception stack(0xfffffe03f14e3920 to 0xfffffe03f14e3a40)
> [ 68.909921] 3920: fffffe03fd5c5800 fffffe0000c55800 fffffe03f14e3a80
> fffffe00000dabd8
> [ 68.909924] 3940: 00000000a0000145 0000000000000015 fffffe03e9602400
> fffffe00002fddb0
> [ 68.909927] 3960: 0000000000000000 0000000000000000 fffffe03fd5c5810
> fffffe03f14e0000
> [ 68.909929] 3980: 0000000000000001 ffffffffff000000 fffffe03db307e38
> 0000000000000000
> [ 68.909932] 39a0: 0000000000737973 00000000ffffffff 0000000000000000
> 000000003b364d50
> [ 68.909935] 39c0: 0000000000000018 ffffffffa99641af 0016fd71b6000000
> 003b9aca00000000
> [ 68.909937] 39e0: fffffe00001f1508 000003ff9b9fd028 000003ffed7a0a10
> fffffe03fd5c5800
> [ 68.909940] 3a00: fffffe0000c55800 fffffe0000cea1c8 fffffe03fd5a5800
> fffffe0000ca2eb0
> [ 68.909943] 3a20: 0000000000000015 fffffe03e9602400 fffffe0000cea1c8
> fffffe0000712000
> [ 68.909945] [<fffffe0000084ce8>] el1_irq+0x68/0xd8
> [ 68.909948] [<fffffe00000da03c>] mutex_optimistic_spin+0x9c/0x1d0
> [ 68.909951] [<fffffe00006fe4b8>] __mutex_lock_slowpath+0x44/0x158
> [ 68.909953] [<fffffe00006fe620>] mutex_lock+0x54/0x58
> [ 68.909956] [<fffffe0000265efc>] kernfs_iop_permission+0x38/0x70
> [ 68.909959] [<fffffe00001fbf50>] __inode_permission+0x88/0xd8
> [ 68.909961] [<fffffe00001fbfd0>] inode_permission+0x30/0x6c
> [ 68.909964] [<fffffe00001fe26c>] link_path_walk+0x68/0x4d4
> [ 68.909966] [<fffffe00001ffa14>] path_openat+0xb4/0x2bc
> [ 68.909968] [<fffffe000020123c>] do_filp_open+0x74/0xd0
> [ 68.909971] [<fffffe00001f13e4>] do_sys_open+0x14c/0x228
> [ 68.909973] [<fffffe00001f1544>] SyS_openat+0x3c/0x48
> [ 68.909976] [<fffffe00000851f0>] el0_svc_naked+0x24/0x28
> .
> .
> .
>
> Reverting 81a43adae3b9 (locking/mutex: Use acquire/release semantics) Makes the problem go away.
>
> At this point it is unknown if this patch is incorrect, or if the underlying ARM64 atomic_*_{acquire,release} primitives are defective, or if the problem lies elsewhere.
>
> I am not requesting any specific action with this e-mail, but wanted to draw attention to the issue. Undoubtedly we will be able to provide more detailed information about the issue in the coming days.
>
> Thanks,
> David Daney
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Andrew Pinski <andrew.pinski@caviumnetworks.com> |
|---|---|
| Date | 2015-12-11 06:00 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEnYd-7LJ-3@gated-at.bofh.it> |
| In reply to | #1289099 |
On Thu, Dec 10, 2015 at 7:29 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Thu, Dec 10, 2015 at 11:44 AM, David Danny wrote:
>>
>> Hi,
>>
>> We are getting soft lockup OOPs on Cavium CN88XX (A.K.A. ThunderX), which is an arm64 implementation.
>
> I get a slightly different OOPs and reverting
> c55a6ffa6285e29f874ed403979472631ec70bff I was able to boot.
> What I saw with osq_lock.c was that osq_wait_next is called for both
> lock and unlock case so it might need both barriers.
> The other question comes does atomic_cmpxchg_release have release
> semantics when the compare fails? Right now it does not.
So looking further I think I understand what is going wrong and why
c55a6ffa6285e29f874ed403979472631ec70bff is incorrect.
The compare and swap inside osq_lock needs to be both release and
acquire semantics memory barriers because the stores (to node) need to
be visible to the other cores before the setting of lock->tail
happens.
Because if node->next is is up to date, we might end up in
osq_wait_next and waiting in an infinite loop while waiting on
ourselves.
I think we should revert c55a6ffa6285e29f874ed403979472631ec70bff
fully as mentioned for the reasons above.
Thanks,
Andrew Pinski
>
> Thanks,
> Andrew
>
>
>>
>> A typical failure shows multiple threads stuck in mutex operations like
>> this:
>>
>> .
>> .
>> .
>> [ 68.909873] Task dump for CPU 18:
>> [ 68.909876] systemd-udevd R running task 0 537 534
>> 0x00000002
>> [ 68.909877] Call trace:
>> [ 68.909880] [<fffffe0000088858>] dump_backtrace+0x0/0x17c
>> [ 68.909883] [<fffffe00000889f8>] show_stack+0x24/0x2c
>> [ 68.909885] [<fffffe00000c4210>] sched_show_task+0xb0/0x104
>> [ 68.909888] [<fffffe00000c682c>] dump_cpu_task+0x48/0x54
>> [ 68.909890] [<fffffe00000ee5e0>] rcu_dump_cpu_stacks+0x9c/0xec
>> [ 68.909893] [<fffffe00000f2c9c>] rcu_check_callbacks+0x524/0xa18
>> [ 68.909896] [<fffffe00000f83a0>] update_process_times+0x44/0x74
>> [ 68.909899] [<fffffe00001078d4>] tick_sched_timer+0x78/0x1ac
>> [ 68.909901] [<fffffe00000f8b74>] __hrtimer_run_queues+0x148/0x2d4
>> [ 68.909903] [<fffffe00000f9464>] hrtimer_interrupt+0xb0/0x1f4
>> [ 68.909906] [<fffffe000056e6e8>] arch_timer_handler_phys+0x3c/0x48
>> [ 68.909909] [<fffffe00000e7fd4>] handle_percpu_devid_irq+0xb0/0x1b0
>> [ 68.909912] [<fffffe00000e33c4>] generic_handle_irq+0x34/0x4c
>> [ 68.909914] [<fffffe00000e3738>] __handle_domain_irq+0x90/0xfc
>> [ 68.909916] [<fffffe0000081d80>] gic_handle_irq+0x90/0x18c
>> [ 68.909918] Exception stack(0xfffffe03f14e3920 to 0xfffffe03f14e3a40)
>> [ 68.909921] 3920: fffffe03fd5c5800 fffffe0000c55800 fffffe03f14e3a80
>> fffffe00000dabd8
>> [ 68.909924] 3940: 00000000a0000145 0000000000000015 fffffe03e9602400
>> fffffe00002fddb0
>> [ 68.909927] 3960: 0000000000000000 0000000000000000 fffffe03fd5c5810
>> fffffe03f14e0000
>> [ 68.909929] 3980: 0000000000000001 ffffffffff000000 fffffe03db307e38
>> 0000000000000000
>> [ 68.909932] 39a0: 0000000000737973 00000000ffffffff 0000000000000000
>> 000000003b364d50
>> [ 68.909935] 39c0: 0000000000000018 ffffffffa99641af 0016fd71b6000000
>> 003b9aca00000000
>> [ 68.909937] 39e0: fffffe00001f1508 000003ff9b9fd028 000003ffed7a0a10
>> fffffe03fd5c5800
>> [ 68.909940] 3a00: fffffe0000c55800 fffffe0000cea1c8 fffffe03fd5a5800
>> fffffe0000ca2eb0
>> [ 68.909943] 3a20: 0000000000000015 fffffe03e9602400 fffffe0000cea1c8
>> fffffe0000712000
>> [ 68.909945] [<fffffe0000084ce8>] el1_irq+0x68/0xd8
>> [ 68.909948] [<fffffe00000da03c>] mutex_optimistic_spin+0x9c/0x1d0
>> [ 68.909951] [<fffffe00006fe4b8>] __mutex_lock_slowpath+0x44/0x158
>> [ 68.909953] [<fffffe00006fe620>] mutex_lock+0x54/0x58
>> [ 68.909956] [<fffffe0000265efc>] kernfs_iop_permission+0x38/0x70
>> [ 68.909959] [<fffffe00001fbf50>] __inode_permission+0x88/0xd8
>> [ 68.909961] [<fffffe00001fbfd0>] inode_permission+0x30/0x6c
>> [ 68.909964] [<fffffe00001fe26c>] link_path_walk+0x68/0x4d4
>> [ 68.909966] [<fffffe00001ffa14>] path_openat+0xb4/0x2bc
>> [ 68.909968] [<fffffe000020123c>] do_filp_open+0x74/0xd0
>> [ 68.909971] [<fffffe00001f13e4>] do_sys_open+0x14c/0x228
>> [ 68.909973] [<fffffe00001f1544>] SyS_openat+0x3c/0x48
>> [ 68.909976] [<fffffe00000851f0>] el0_svc_naked+0x24/0x28
>> .
>> .
>> .
>>
>> Reverting 81a43adae3b9 (locking/mutex: Use acquire/release semantics) Makes the problem go away.
>>
>> At this point it is unknown if this patch is incorrect, or if the underlying ARM64 atomic_*_{acquire,release} primitives are defective, or if the problem lies elsewhere.
>>
>> I am not requesting any specific action with this e-mail, but wanted to draw attention to the issue. Undoubtedly we will be able to provide more detailed information about the issue in the coming days.
>>
>> Thanks,
>> David Daney
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-12-11 09:50 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEryN-1NQ-1@gated-at.bofh.it> |
| In reply to | #1289141 |
On Thu, Dec 10, 2015 at 08:51:34PM -0800, Andrew Pinski wrote: > So looking further I think I understand what is going wrong and why > c55a6ffa6285e29f874ed403979472631ec70bff is incorrect. The osq_wait_next() call in osq_lock() is when we fail the lock. This is effectively trylock() semantics and like for cmpxchg a failed trylock has no implied barrier semantics. So from that POV osq_wait_next() does not need to provide ACQUIRE semantics. In osq_unlock() there's an xchg() in front, which implies full barriers and thereby provides RELEASE semantics for that part of osq_unlock(), so again, from this POV osq_wait_next() does not need to provide RELEASE semantics. > The compare and swap inside osq_lock needs to be both release and > acquire semantics memory barriers because the stores (to node) need to > be visible to the other cores before the setting of lock->tail > happens. I'm a wee bit confused on what exactly you mean. Both stores to @node: 1) osq_wait_next(): next = xchg(&node->next, NULL) 2) osq_unlock(): next = xchg(&node->next, NULL) are xchg() calls which imply full ordering (sequential consistency). Similarly the store before osq_wait_next() in osq_lock(), namely: cmpxchg(&prev->node, node, NULL) is fully ordered. So I cannot see any store being delayed past the atomic_cmpxchg_acquire(). Now you mention 'compare and swap inside osq_lock' which I take to be the latter; and it _is_ fully ordered. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Will Deacon <will.deacon@arm.com> |
|---|---|
| Date | 2015-12-11 13:10 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEuGl-43C-3@gated-at.bofh.it> |
| In reply to | #1289325 |
Hi all, On Fri, Dec 11, 2015 at 09:41:33AM +0100, Peter Zijlstra wrote: > On Thu, Dec 10, 2015 at 08:51:34PM -0800, Andrew Pinski wrote: > > > So looking further I think I understand what is going wrong and why > > c55a6ffa6285e29f874ed403979472631ec70bff is incorrect. > > The osq_wait_next() call in osq_lock() is when we fail the lock. This is > effectively trylock() semantics and like for cmpxchg a failed trylock > has no implied barrier semantics. So from that POV osq_wait_next() does > not need to provide ACQUIRE semantics. > > In osq_unlock() there's an xchg() in front, which implies full barriers > and thereby provides RELEASE semantics for that part of osq_unlock(), so > again, from this POV osq_wait_next() does not need to provide RELEASE > semantics. > > > The compare and swap inside osq_lock needs to be both release and > > acquire semantics memory barriers because the stores (to node) need to > > be visible to the other cores before the setting of lock->tail > > happens. > > I'm a wee bit confused on what exactly you mean. Both stores to @node: > > 1) osq_wait_next(): next = xchg(&node->next, NULL) > 2) osq_unlock(): next = xchg(&node->next, NULL) > > are xchg() calls which imply full ordering (sequential consistency). I think Andrew meant the atomic_xchg_acquire at the start of osq_lock, as opposed to "compare and swap". In which case, it does look like there's a bug here because there is nothing to order the initialisation of the node fields with publishing of the node, whether that's indirectly as a result of setting the tail to the current CPU or directly as a result of the WRITE_ONCE. Andrew, David: does making that atomic_xchg_acquire and atomic_xchg fix things for you? I don't fully grok what 81a43adae3b9 has to do with any of this, so maybe there's another bug too. Will --->8 diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c index d092a0c9c2d4..05a37857ab55 100644 --- a/kernel/locking/osq_lock.c +++ b/kernel/locking/osq_lock.c @@ -93,10 +93,12 @@ bool osq_lock(struct optimistic_spin_queue *lock) node->cpu = curr; /* - * ACQUIRE semantics, pairs with corresponding RELEASE - * in unlock() uncontended, or fastpath. + * We need both ACQUIRE (pairs with corresponding RELEASE in + * unlock() uncontended, or fastpath) and RELEASE (to publish + * the node fields we just initialised) semantics when updating + * the lock tail. */ - old = atomic_xchg_acquire(&lock->tail, curr); + old = atomic_xchg(&lock->tail, curr); if (old == OSQ_UNLOCKED_VAL) return true; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-12-11 13:20 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEuQ1-486-7@gated-at.bofh.it> |
| In reply to | #1289502 |
On Fri, Dec 11, 2015 at 12:04:19PM +0000, Will Deacon wrote: > I think Andrew meant the atomic_xchg_acquire at the start of osq_lock, > as opposed to "compare and swap". In which case, it does look like > there's a bug here because there is nothing to order the initialisation > of the node fields with publishing of the node, whether that's > indirectly as a result of setting the tail to the current CPU or > directly as a result of the WRITE_ONCE. Agreed, this does indeed look like a bug. If confirmed please write a shiny changelog and I'll queue asap. > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c > index d092a0c9c2d4..05a37857ab55 100644 > --- a/kernel/locking/osq_lock.c > +++ b/kernel/locking/osq_lock.c > @@ -93,10 +93,12 @@ bool osq_lock(struct optimistic_spin_queue *lock) > node->cpu = curr; > > /* > - * ACQUIRE semantics, pairs with corresponding RELEASE > - * in unlock() uncontended, or fastpath. > + * We need both ACQUIRE (pairs with corresponding RELEASE in > + * unlock() uncontended, or fastpath) and RELEASE (to publish > + * the node fields we just initialised) semantics when updating > + * the lock tail. > */ > - old = atomic_xchg_acquire(&lock->tail, curr); > + old = atomic_xchg(&lock->tail, curr); > if (old == OSQ_UNLOCKED_VAL) > return true; > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Will Deacon <will.deacon@arm.com> |
|---|---|
| Date | 2015-12-11 13:20 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEuQ2-486-13@gated-at.bofh.it> |
| In reply to | #1289509 |
On Fri, Dec 11, 2015 at 01:13:19PM +0100, Peter Zijlstra wrote: > On Fri, Dec 11, 2015 at 12:04:19PM +0000, Will Deacon wrote: > > I think Andrew meant the atomic_xchg_acquire at the start of osq_lock, > > as opposed to "compare and swap". In which case, it does look like > > there's a bug here because there is nothing to order the initialisation > > of the node fields with publishing of the node, whether that's > > indirectly as a result of setting the tail to the current CPU or > > directly as a result of the WRITE_ONCE. > > Agreed, this does indeed look like a bug. If confirmed please write a > shiny changelog and I'll queue asap. Yup. I've failed to reproduce the issue locally, so we'll need to wait for Andrew and/or David to get back to us first. Will > > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c > > index d092a0c9c2d4..05a37857ab55 100644 > > --- a/kernel/locking/osq_lock.c > > +++ b/kernel/locking/osq_lock.c > > @@ -93,10 +93,12 @@ bool osq_lock(struct optimistic_spin_queue *lock) > > node->cpu = curr; > > > > /* > > - * ACQUIRE semantics, pairs with corresponding RELEASE > > - * in unlock() uncontended, or fastpath. > > + * We need both ACQUIRE (pairs with corresponding RELEASE in > > + * unlock() uncontended, or fastpath) and RELEASE (to publish > > + * the node fields we just initialised) semantics when updating > > + * the lock tail. > > */ > > - old = atomic_xchg_acquire(&lock->tail, curr); > > + old = atomic_xchg(&lock->tail, curr); > > if (old == OSQ_UNLOCKED_VAL) > > return true; > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-12-11 13:30 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEuZI-4cP-1@gated-at.bofh.it> |
| In reply to | #1289511 |
On Fri, Dec 11, 2015 at 12:18:00PM +0000, Will Deacon wrote: > On Fri, Dec 11, 2015 at 01:13:19PM +0100, Peter Zijlstra wrote: > > On Fri, Dec 11, 2015 at 12:04:19PM +0000, Will Deacon wrote: > > > I think Andrew meant the atomic_xchg_acquire at the start of osq_lock, > > > as opposed to "compare and swap". In which case, it does look like > > > there's a bug here because there is nothing to order the initialisation > > > of the node fields with publishing of the node, whether that's > > > indirectly as a result of setting the tail to the current CPU or > > > directly as a result of the WRITE_ONCE. > > > > Agreed, this does indeed look like a bug. If confirmed please write a > > shiny changelog and I'll queue asap. > > Yup. I've failed to reproduce the issue locally, so we'll need to wait > for Andrew and/or David to get back to us first. While we're there, the acquire in osq_wait_next() seems somewhat ill documented too. I _think_ we need ACQUIRE semantics there because we want to strictly order the lock-unqueue A,B,C steps and we get that with: A: SC B: ACQ C: Relaxed Similarly for unlock we want the WRITE_ONCE to happen after osq_wait_next, but in that case we can even rely on the control dependency there. As noted in a previous email, the ACQUIRE for osq_wait_next() does not come from its use in lock since its on the fail path, and trylock failure doesn't imply any barriers. Not should it have RELEASE semantics for its use in unlock, since we already have that covered by the xchg() done prior. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Will Deacon <will.deacon@arm.com> |
|---|---|
| Date | 2015-12-11 14:40 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEw5t-52w-45@gated-at.bofh.it> |
| In reply to | #1289518 |
On Fri, Dec 11, 2015 at 01:26:47PM +0100, Peter Zijlstra wrote: > On Fri, Dec 11, 2015 at 12:18:00PM +0000, Will Deacon wrote: > > On Fri, Dec 11, 2015 at 01:13:19PM +0100, Peter Zijlstra wrote: > > > On Fri, Dec 11, 2015 at 12:04:19PM +0000, Will Deacon wrote: > > > > I think Andrew meant the atomic_xchg_acquire at the start of osq_lock, > > > > as opposed to "compare and swap". In which case, it does look like > > > > there's a bug here because there is nothing to order the initialisation > > > > of the node fields with publishing of the node, whether that's > > > > indirectly as a result of setting the tail to the current CPU or > > > > directly as a result of the WRITE_ONCE. > > > > > > Agreed, this does indeed look like a bug. If confirmed please write a > > > shiny changelog and I'll queue asap. > > > > Yup. I've failed to reproduce the issue locally, so we'll need to wait > > for Andrew and/or David to get back to us first. > > While we're there, the acquire in osq_wait_next() seems somewhat ill > documented too. > > I _think_ we need ACQUIRE semantics there because we want to strictly > order the lock-unqueue A,B,C steps and we get that with: > > A: SC > B: ACQ > C: Relaxed > > Similarly for unlock we want the WRITE_ONCE to happen after > osq_wait_next, but in that case we can even rely on the control > dependency there. Even for the lock-unqueue case, isn't B->C ordered by a control dependency because C consists only of stores? Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-12-11 14:50 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEwf8-57t-11@gated-at.bofh.it> |
| In reply to | #1289580 |
On Fri, Dec 11, 2015 at 01:33:14PM +0000, Will Deacon wrote: > On Fri, Dec 11, 2015 at 01:26:47PM +0100, Peter Zijlstra wrote: > > While we're there, the acquire in osq_wait_next() seems somewhat ill > > documented too. > > > > I _think_ we need ACQUIRE semantics there because we want to strictly > > order the lock-unqueue A,B,C steps and we get that with: > > > > A: SC > > B: ACQ > > C: Relaxed > > > > Similarly for unlock we want the WRITE_ONCE to happen after > > osq_wait_next, but in that case we can even rely on the control > > dependency there. > > Even for the lock-unqueue case, isn't B->C ordered by a control dependency > because C consists only of stores? Hmm, indeed. So we could go fully relaxed on it I suppose, since the same is true for the unlock site. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Will Deacon <will.deacon@arm.com> |
|---|---|
| Date | 2015-12-11 15:10 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEwyu-5wq-21@gated-at.bofh.it> |
| In reply to | #1289585 |
On Fri, Dec 11, 2015 at 02:48:03PM +0100, Peter Zijlstra wrote: > On Fri, Dec 11, 2015 at 01:33:14PM +0000, Will Deacon wrote: > > On Fri, Dec 11, 2015 at 01:26:47PM +0100, Peter Zijlstra wrote: > > > > While we're there, the acquire in osq_wait_next() seems somewhat ill > > > documented too. > > > > > > I _think_ we need ACQUIRE semantics there because we want to strictly > > > order the lock-unqueue A,B,C steps and we get that with: > > > > > > A: SC > > > B: ACQ > > > C: Relaxed > > > > > > Similarly for unlock we want the WRITE_ONCE to happen after > > > osq_wait_next, but in that case we can even rely on the control > > > dependency there. > > > > Even for the lock-unqueue case, isn't B->C ordered by a control dependency > > because C consists only of stores? > > Hmm, indeed. So we could go fully relaxed on it I suppose, since the > same is true for the unlock site. In which case, we should be able to relax the xchg in there (osq_wait_next) too, right? Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-12-11 18:20 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEzwm-7qK-15@gated-at.bofh.it> |
| In reply to | #1289599 |
On Fri, Dec 11, 2015 at 02:06:49PM +0000, Will Deacon wrote: > On Fri, Dec 11, 2015 at 02:48:03PM +0100, Peter Zijlstra wrote: > > On Fri, Dec 11, 2015 at 01:33:14PM +0000, Will Deacon wrote: > > > On Fri, Dec 11, 2015 at 01:26:47PM +0100, Peter Zijlstra wrote: > > > > > > While we're there, the acquire in osq_wait_next() seems somewhat ill > > > > documented too. > > > > > > > > I _think_ we need ACQUIRE semantics there because we want to strictly > > > > order the lock-unqueue A,B,C steps and we get that with: > > > > > > > > A: SC > > > > B: ACQ > > > > C: Relaxed > > > > > > > > Similarly for unlock we want the WRITE_ONCE to happen after > > > > osq_wait_next, but in that case we can even rely on the control > > > > dependency there. > > > > > > Even for the lock-unqueue case, isn't B->C ordered by a control dependency > > > because C consists only of stores? > > > > Hmm, indeed. So we could go fully relaxed on it I suppose, since the > > same is true for the unlock site. > > In which case, we should be able to relax the xchg in there (osq_wait_next) > too, right? Can I have second thoughts an confuse matters again? ;-) A RmW-acq is a load-acquire+store. That means the store is _after_ the load and thus not required for the completion of the control dependency. Therefore the store in question can reorder inside the conditional control block's stores. Hmm? Suggesting this acquire is in fact also wrong, since we need full order ops to guarantee full order both in lock and unlock. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Will Deacon <will.deacon@arm.com> |
|---|---|
| Date | 2015-12-11 18:30 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEzG1-7up-5@gated-at.bofh.it> |
| In reply to | #1289749 |
On Fri, Dec 11, 2015 at 06:11:28PM +0100, Peter Zijlstra wrote: > On Fri, Dec 11, 2015 at 02:06:49PM +0000, Will Deacon wrote: > > On Fri, Dec 11, 2015 at 02:48:03PM +0100, Peter Zijlstra wrote: > > > On Fri, Dec 11, 2015 at 01:33:14PM +0000, Will Deacon wrote: > > > > On Fri, Dec 11, 2015 at 01:26:47PM +0100, Peter Zijlstra wrote: > > > > > > > > While we're there, the acquire in osq_wait_next() seems somewhat ill > > > > > documented too. > > > > > > > > > > I _think_ we need ACQUIRE semantics there because we want to strictly > > > > > order the lock-unqueue A,B,C steps and we get that with: > > > > > > > > > > A: SC > > > > > B: ACQ > > > > > C: Relaxed > > > > > > > > > > Similarly for unlock we want the WRITE_ONCE to happen after > > > > > osq_wait_next, but in that case we can even rely on the control > > > > > dependency there. > > > > > > > > Even for the lock-unqueue case, isn't B->C ordered by a control dependency > > > > because C consists only of stores? > > > > > > Hmm, indeed. So we could go fully relaxed on it I suppose, since the > > > same is true for the unlock site. > > > > In which case, we should be able to relax the xchg in there (osq_wait_next) > > too, right? > > Can I have second thoughts an confuse matters again? ;-) > > A RmW-acq is a load-acquire+store. That means the store is _after_ the > load and thus not required for the completion of the control dependency. > > Therefore the store in question can reorder inside the conditional > control block's stores. > > Hmm? Ah yeah, it's the same thing we were discussing the other day! Whilst there is a form of control dependency from the SC part of the LL/SC sequence, it doesn't guarantee ordering in the same way that a load->store control dependency does. That is, it orders subsequent writes to be afterwards in the coherence order but it doesn't ensure multi-copy atomicity for readers. Now, in this case, &lock->tail is only ever accessed by other cmpxchg operations, so I think it does actually work using just the control dependency. Worst case, a concurrent osq_wait_next gets a stale value in the atomic_read, but that's not a correctness problem. Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2015-12-11 23:40 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEEw3-261-37@gated-at.bofh.it> |
| In reply to | #1289585 |
On Fri, Dec 11, 2015 at 02:48:03PM +0100, Peter Zijlstra wrote: > On Fri, Dec 11, 2015 at 01:33:14PM +0000, Will Deacon wrote: > > On Fri, Dec 11, 2015 at 01:26:47PM +0100, Peter Zijlstra wrote: > > > > While we're there, the acquire in osq_wait_next() seems somewhat ill > > > documented too. > > > > > > I _think_ we need ACQUIRE semantics there because we want to strictly > > > order the lock-unqueue A,B,C steps and we get that with: > > > > > > A: SC > > > B: ACQ > > > C: Relaxed > > > > > > Similarly for unlock we want the WRITE_ONCE to happen after > > > osq_wait_next, but in that case we can even rely on the control > > > dependency there. > > > > Even for the lock-unqueue case, isn't B->C ordered by a control dependency > > because C consists only of stores? > > Hmm, indeed. So we could go fully relaxed on it I suppose, since the > same is true for the unlock site. I am probably missing quite a bit on this thread, but don't x86 MMIO accesses to frame buffers need to interact with something more heavyweight than an x86 release store or acquire load in order to remain confined to the resulting critical section? Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> |
|---|---|
| Date | 2015-12-14 20:00 +0100 |
| Subject | Re: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qFGvL-1yD-13@gated-at.bofh.it> |
| In reply to | #1289973 |
On Fri, 11 Dec 2015 14:35:40 -0800 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote: > On Fri, Dec 11, 2015 at 02:48:03PM +0100, Peter Zijlstra wrote: > > On Fri, Dec 11, 2015 at 01:33:14PM +0000, Will Deacon wrote: > > > On Fri, Dec 11, 2015 at 01:26:47PM +0100, Peter Zijlstra wrote: > > > > > > While we're there, the acquire in osq_wait_next() seems somewhat ill > > > > documented too. > > > > > > > > I _think_ we need ACQUIRE semantics there because we want to strictly > > > > order the lock-unqueue A,B,C steps and we get that with: > > > > > > > > A: SC > > > > B: ACQ > > > > C: Relaxed > > > > > > > > Similarly for unlock we want the WRITE_ONCE to happen after > > > > osq_wait_next, but in that case we can even rely on the control > > > > dependency there. > > > > > > Even for the lock-unqueue case, isn't B->C ordered by a control dependency > > > because C consists only of stores? > > > > Hmm, indeed. So we could go fully relaxed on it I suppose, since the > > same is true for the unlock site. > > I am probably missing quite a bit on this thread, but don't x86 MMIO > accesses to frame buffers need to interact with something more heavyweight > than an x86 release store or acquire load in order to remain confined > to the resulting critical section? Depends upon the device and the mapping. There are also CPU errata related to write combining on older CPUs (notably Pentium Pro era) which result in ordering errors with write combining unless deliberately fenced. Any PCI access isn't constrained to the critical section unless a PCI read from the same device is done and completes before exiting. Even then on processors with a separate APIC bus (PPro, PII I think) interrupts are asynchronous on their own bus. The PCI posting rules also apply to DMA. Finally we run the IDT WinChip in out-of-order store mode not full x86 compatibility which while uniprocessor does mean the correct fences matter. Just to ensure total confusion some video cards have MMIO areas that are not in fact memory but a FIFO rigged to look like a block of RAM for speed of writing. In those cases the rules are a bit card dependant. But seriously are there any cases we actually care about this for osq ? Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-12-14 21:40 +0100 |
| Subject | Re: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qFI4x-2Ex-5@gated-at.bofh.it> |
| In reply to | #1291455 |
On Mon, Dec 14, 2015 at 06:49:31PM +0000, One Thousand Gnomes wrote: > > But seriously are there any cases we actually care about this for osq ? So I think what PaulMck is worried about is that one would expect things like: mutex_lock(); MMIO(++var); mutex_unlock(); (the same example Linus gave but with a mutex instead of a spinlock) to just work. Now, I haven't checked the code, but I'm not sure we ever rely on osq to provide the mutex ACQUIRE barrier, since we always need to acquire the mutex variable itself after we've acquired the osq 'lock'. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2015-12-15 07:20 +0100 |
| Subject | Re: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qFR7Q-fs-5@gated-at.bofh.it> |
| In reply to | #1291455 |
On Mon, Dec 14, 2015 at 06:49:31PM +0000, One Thousand Gnomes wrote: > On Fri, 11 Dec 2015 14:35:40 -0800 > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote: > > > On Fri, Dec 11, 2015 at 02:48:03PM +0100, Peter Zijlstra wrote: > > > On Fri, Dec 11, 2015 at 01:33:14PM +0000, Will Deacon wrote: > > > > On Fri, Dec 11, 2015 at 01:26:47PM +0100, Peter Zijlstra wrote: > > > > > > > > While we're there, the acquire in osq_wait_next() seems somewhat ill > > > > > documented too. > > > > > > > > > > I _think_ we need ACQUIRE semantics there because we want to strictly > > > > > order the lock-unqueue A,B,C steps and we get that with: > > > > > > > > > > A: SC > > > > > B: ACQ > > > > > C: Relaxed > > > > > > > > > > Similarly for unlock we want the WRITE_ONCE to happen after > > > > > osq_wait_next, but in that case we can even rely on the control > > > > > dependency there. > > > > > > > > Even for the lock-unqueue case, isn't B->C ordered by a control dependency > > > > because C consists only of stores? > > > > > > Hmm, indeed. So we could go fully relaxed on it I suppose, since the > > > same is true for the unlock site. > > > > I am probably missing quite a bit on this thread, but don't x86 MMIO > > accesses to frame buffers need to interact with something more heavyweight > > than an x86 release store or acquire load in order to remain confined > > to the resulting critical section? > > Depends upon the device and the mapping. There are also CPU errata > related to write combining on older CPUs (notably Pentium Pro era) which > result in ordering errors with write combining unless deliberately fenced. > > Any PCI access isn't constrained to the critical section unless a PCI > read from the same device is done and completes before exiting. Even then > on processors with a separate APIC bus (PPro, PII I think) interrupts are > asynchronous on their own bus. > > The PCI posting rules also apply to DMA. > > Finally we run the IDT WinChip in out-of-order store mode not full x86 > compatibility which while uniprocessor does mean the correct fences > matter. > > Just to ensure total confusion some video cards have MMIO areas that are > not in fact memory but a FIFO rigged to look like a block of RAM for > speed of writing. In those cases the rules are a bit card dependant. Sounds like the usual fun and excitement! ;-) > But seriously are there any cases we actually care about this for osq ? Apparently not, given Peter's email. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-12-14 21:30 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qFHUS-2Aa-1@gated-at.bofh.it> |
| In reply to | #1289973 |
On Fri, Dec 11, 2015 at 02:35:40PM -0800, Paul E. McKenney wrote: > On Fri, Dec 11, 2015 at 02:48:03PM +0100, Peter Zijlstra wrote: > > On Fri, Dec 11, 2015 at 01:33:14PM +0000, Will Deacon wrote: > > > On Fri, Dec 11, 2015 at 01:26:47PM +0100, Peter Zijlstra wrote: > > > > > > While we're there, the acquire in osq_wait_next() seems somewhat ill > > > > documented too. > > > > > > > > I _think_ we need ACQUIRE semantics there because we want to strictly > > > > order the lock-unqueue A,B,C steps and we get that with: > > > > > > > > A: SC > > > > B: ACQ > > > > C: Relaxed > > > > > > > > Similarly for unlock we want the WRITE_ONCE to happen after > > > > osq_wait_next, but in that case we can even rely on the control > > > > dependency there. > > > > > > Even for the lock-unqueue case, isn't B->C ordered by a control dependency > > > because C consists only of stores? > > > > Hmm, indeed. So we could go fully relaxed on it I suppose, since the > > same is true for the unlock site. > > I am probably missing quite a bit on this thread, but don't x86 MMIO > accesses to frame buffers need to interact with something more heavyweight > than an x86 release store or acquire load in order to remain confined > to the resulting critical section? So on x86 there really isn't a problem because every atomic op (and there's plenty here) will be a full barrier. That is, even if you were to replace everything with _relaxed() ops, it would still work as 'expected' on x86. ppc/arm64 will crash and burn, but that's another story. But the important point here was that osq_wait_next() is never relied upon to provide either the ACQUIRE semantics for osq_lock() not the RELEASE semantics for osq_unlock(). Those are provided by other ops. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> |
|---|---|
| Date | 2015-12-15 05:40 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qFPz3-7z1-3@gated-at.bofh.it> |
| In reply to | #1291530 |
On Mon, Dec 14, 2015 at 09:28:55PM +0100, Peter Zijlstra wrote: > On Fri, Dec 11, 2015 at 02:35:40PM -0800, Paul E. McKenney wrote: > > On Fri, Dec 11, 2015 at 02:48:03PM +0100, Peter Zijlstra wrote: > > > On Fri, Dec 11, 2015 at 01:33:14PM +0000, Will Deacon wrote: > > > > On Fri, Dec 11, 2015 at 01:26:47PM +0100, Peter Zijlstra wrote: > > > > > > > > While we're there, the acquire in osq_wait_next() seems somewhat ill > > > > > documented too. > > > > > > > > > > I _think_ we need ACQUIRE semantics there because we want to strictly > > > > > order the lock-unqueue A,B,C steps and we get that with: > > > > > > > > > > A: SC > > > > > B: ACQ > > > > > C: Relaxed > > > > > > > > > > Similarly for unlock we want the WRITE_ONCE to happen after > > > > > osq_wait_next, but in that case we can even rely on the control > > > > > dependency there. > > > > > > > > Even for the lock-unqueue case, isn't B->C ordered by a control dependency > > > > because C consists only of stores? > > > > > > Hmm, indeed. So we could go fully relaxed on it I suppose, since the > > > same is true for the unlock site. > > > > I am probably missing quite a bit on this thread, but don't x86 MMIO > > accesses to frame buffers need to interact with something more heavyweight > > than an x86 release store or acquire load in order to remain confined > > to the resulting critical section? > > So on x86 there really isn't a problem because every atomic op (and > there's plenty here) will be a full barrier. > > That is, even if you were to replace everything with _relaxed() ops, it > would still work as 'expected' on x86. > > ppc/arm64 will crash and burn, but that's another story. > > But the important point here was that osq_wait_next() is never relied > upon to provide either the ACQUIRE semantics for osq_lock() not the > RELEASE semantics for osq_unlock(). Those are provided by other ops. OK, good to know! Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Davidlohr Bueso <dave@stgolabs.net> |
|---|---|
| Date | 2015-12-11 15:20 +0100 |
| Subject | Re: FW: Commit 81a43adae3b9 (locking/mutex: Use acquire/release semantics) causing failures on arm64 (ThunderX) |
| Message-ID | <qEwI9-5Bq-5@gated-at.bofh.it> |
| In reply to | #1289502 |
On Fri, 11 Dec 2015, Will Deacon wrote: >I think Andrew meant the atomic_xchg_acquire at the start of osq_lock, >as opposed to "compare and swap". In which case, it does look like >there's a bug here because there is nothing to order the initialisation >of the node fields with publishing of the node, whether that's >indirectly as a result of setting the tail to the current CPU or >directly as a result of the WRITE_ONCE. Sorry I'm late to the party. Duh yes this is obviously bogus, and worse I recall triggering a similar tail initialization issue in osq_lock on some experimental work on x86, so this is very much a point of failure. Ack. > >Andrew, David: does making that atomic_xchg_acquire and atomic_xchg >fix things for you? > >I don't fully grok what 81a43adae3b9 has to do with any of this, so >maybe there's another bug too. I think this is mainly because mutex_optimistic_spin is where the stack shows the lockup, which really translates to c55a6ffa62. Thanks, Davidlohr -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web