From: Jiri Olsa <hidden> Date: 2009-06-25 12:26:00
Adding memory barrier to the __pollwait function paired with
receive callbacks. The smp_mb__after_lock define is added,
since {read|write|spin}_lock() on x86 are full memory barriers.
The race fires, when following code paths meet, and the tp->rcv_nxt and
__add_wait_queue updates stay in CPU caches.
CPU1 CPU2
sys_select receive packet
... ...
__add_wait_queue update tp->rcv_nxt
... ...
tp->rcv_nxt check sock_def_readable
... {
schedule ...
if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
wake_up_interruptible(sk->sk_sleep)
...
}
If there was no cache the code would work ok, since the wait_queue and
rcv_nxt are opposit to each other.
Meaning that once tp->rcv_nxt is updated by CPU2, the CPU1 either already
passed the tp->rcv_nxt check and sleeps, or will get the new value for
tp->rcv_nxt and will return with new data mask.
In both cases the process (CPU1) is being added to the wait queue, so the
waitqueue_active (CPU2) call cannot miss and will wake up CPU1.
The bad case is when the __add_wait_queue changes done by CPU1 stay in its
cache, and so does the tp->rcv_nxt update on CPU2 side. The CPU1 will then
endup calling schedule and sleep forever if there are no more data on the
socket.
wbr,
jirka
Signed-off-by: Jiri Olsa <redacted>
---
arch/x86/include/asm/spinlock.h | 3 +++
fs/select.c | 4 ++++
include/linux/spinlock.h | 5 +++++
include/net/sock.h | 18 ++++++++++++++++++
net/atm/common.c | 4 ++--
net/core/sock.c | 8 ++++----
net/dccp/output.c | 2 +-
net/iucv/af_iucv.c | 2 +-
net/rxrpc/af_rxrpc.c | 2 +-
net/unix/af_unix.c | 2 +-
10 files changed, 40 insertions(+), 10 deletions(-)
@@ -302,4 +302,7 @@ static inline void __raw_write_unlock(raw_rwlock_t *rw)#define _raw_read_relax(lock) cpu_relax()#define _raw_write_relax(lock) cpu_relax()+/* The {read|write|spin}_lock() on x86 are full memory barriers. */+#define smp_mb__after_lock() do { } while (0)+#endif /* _ASM_X86_SPINLOCK_H */
@@ -219,6 +219,10 @@ static void __pollwait(struct file *filp, wait_queue_head_t *wait_address,init_waitqueue_func_entry(&entry->wait,pollwake);entry->wait.private=pwq;add_wait_queue(wait_address,&entry->wait);++/* This memory barrier is paired with the smp_mb__after_lock+*inthesk_has_sleeper.*/+smp_mb();}intpoll_schedule_timeout(structpoll_wqueues*pwq,intstate,
@@ -132,6 +132,11 @@ do { \#endif /*__raw_spin_is_contended*/#endif+/* The lock does not imply full memory barrier. */+#ifndef smp_mb__after_lock+#define smp_mb__after_lock() smp_mb()+#endif+/***spin_unlock_wait-waituntilthespinlockgetsunlocked*@lock:thespinlockinquestion.
Can't really comment this patch, except this all looks reasonable to me.
Add more CCs.
On 06/25, Jiri Olsa wrote:
quoted hunk
Adding memory barrier to the __pollwait function paired with
receive callbacks. The smp_mb__after_lock define is added,
since {read|write|spin}_lock() on x86 are full memory barriers.
The race fires, when following code paths meet, and the tp->rcv_nxt and
__add_wait_queue updates stay in CPU caches.
CPU1 CPU2
sys_select receive packet
... ...
__add_wait_queue update tp->rcv_nxt
... ...
tp->rcv_nxt check sock_def_readable
... {
schedule ...
if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
wake_up_interruptible(sk->sk_sleep)
...
}
If there was no cache the code would work ok, since the wait_queue and
rcv_nxt are opposit to each other.
Meaning that once tp->rcv_nxt is updated by CPU2, the CPU1 either already
passed the tp->rcv_nxt check and sleeps, or will get the new value for
tp->rcv_nxt and will return with new data mask.
In both cases the process (CPU1) is being added to the wait queue, so the
waitqueue_active (CPU2) call cannot miss and will wake up CPU1.
The bad case is when the __add_wait_queue changes done by CPU1 stay in its
cache, and so does the tp->rcv_nxt update on CPU2 side. The CPU1 will then
endup calling schedule and sleep forever if there are no more data on the
socket.
wbr,
jirka
Signed-off-by: Jiri Olsa <redacted>
---
arch/x86/include/asm/spinlock.h | 3 +++
fs/select.c | 4 ++++
include/linux/spinlock.h | 5 +++++
include/net/sock.h | 18 ++++++++++++++++++
net/atm/common.c | 4 ++--
net/core/sock.c | 8 ++++----
net/dccp/output.c | 2 +-
net/iucv/af_iucv.c | 2 +-
net/rxrpc/af_rxrpc.c | 2 +-
net/unix/af_unix.c | 2 +-
10 files changed, 40 insertions(+), 10 deletions(-)
@@ -302,4 +302,7 @@ static inline void __raw_write_unlock(raw_rwlock_t *rw)#define _raw_read_relax(lock) cpu_relax()#define _raw_write_relax(lock) cpu_relax()+/* The {read|write|spin}_lock() on x86 are full memory barriers. */+#define smp_mb__after_lock() do { } while (0)+#endif /* _ASM_X86_SPINLOCK_H */
@@ -219,6 +219,10 @@ static void __pollwait(struct file *filp, wait_queue_head_t *wait_address,init_waitqueue_func_entry(&entry->wait,pollwake);entry->wait.private=pwq;add_wait_queue(wait_address,&entry->wait);++/* This memory barrier is paired with the smp_mb__after_lock+*inthesk_has_sleeper.*/+smp_mb();}intpoll_schedule_timeout(structpoll_wqueues*pwq,intstate,
@@ -132,6 +132,11 @@ do { \#endif /*__raw_spin_is_contended*/#endif+/* The lock does not imply full memory barrier. */+#ifndef smp_mb__after_lock+#define smp_mb__after_lock() smp_mb()+#endif+/***spin_unlock_wait-waituntilthespinlockgetsunlocked*@lock:thespinlockinquestion.
From: Eric Dumazet <hidden> Date: 2009-06-25 23:19:32
Jiri Olsa a écrit :
Adding memory barrier to the __pollwait function paired with
receive callbacks. The smp_mb__after_lock define is added,
since {read|write|spin}_lock() on x86 are full memory barriers.
The race fires, when following code paths meet, and the tp->rcv_nxt and
__add_wait_queue updates stay in CPU caches.
CPU1 CPU2
sys_select receive packet
... ...
__add_wait_queue update tp->rcv_nxt
... ...
tp->rcv_nxt check sock_def_readable
... {
schedule ...
if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
wake_up_interruptible(sk->sk_sleep)
...
}
If there was no cache the code would work ok, since the wait_queue and
rcv_nxt are opposit to each other.
Meaning that once tp->rcv_nxt is updated by CPU2, the CPU1 either already
passed the tp->rcv_nxt check and sleeps, or will get the new value for
tp->rcv_nxt and will return with new data mask.
In both cases the process (CPU1) is being added to the wait queue, so the
waitqueue_active (CPU2) call cannot miss and will wake up CPU1.
The bad case is when the __add_wait_queue changes done by CPU1 stay in its
cache, and so does the tp->rcv_nxt update on CPU2 side. The CPU1 will then
endup calling schedule and sleep forever if there are no more data on the
socket.
wbr,
jirka
Signed-off-by: Jiri Olsa <redacted>
Patch seems fine for me, thanks a lot Jiri !
Signed-off-by: Eric Dumazet <redacted>
@@ -302,4 +302,7 @@ static inline void __raw_write_unlock(raw_rwlock_t *rw)#define _raw_read_relax(lock) cpu_relax()#define _raw_write_relax(lock) cpu_relax()+/* The {read|write|spin}_lock() on x86 are full memory barriers. */+#define smp_mb__after_lock() do { } while (0)+#endif /* _ASM_X86_SPINLOCK_H */
@@ -219,6 +219,10 @@ static void __pollwait(struct file *filp, wait_queue_head_t *wait_address,init_waitqueue_func_entry(&entry->wait,pollwake);entry->wait.private=pwq;add_wait_queue(wait_address,&entry->wait);++/* This memory barrier is paired with the smp_mb__after_lock+*inthesk_has_sleeper.*/+smp_mb();}intpoll_schedule_timeout(structpoll_wqueues*pwq,intstate,
@@ -132,6 +132,11 @@ do { \#endif /*__raw_spin_is_contended*/#endif+/* The lock does not imply full memory barrier. */+#ifndef smp_mb__after_lock+#define smp_mb__after_lock() smp_mb()+#endif+/***spin_unlock_wait-waituntilthespinlockgetsunlocked*@lock:thespinlockinquestion.
Can't really comment this patch, except this all looks reasonable to me.
Add more CCs.
While this can work, IMO it'd be cleaner to have the smp_mb() moved from
fs/select.c to the ->poll() function.
Having a barrier that matches another one in another susbsystem, because
of the special locking logic of such subsystem, is not too shiny IMHO.
On 06/25, Jiri Olsa wrote:
quoted
Adding memory barrier to the __pollwait function paired with
receive callbacks. The smp_mb__after_lock define is added,
since {read|write|spin}_lock() on x86 are full memory barriers.
The race fires, when following code paths meet, and the tp->rcv_nxt and
__add_wait_queue updates stay in CPU caches.
CPU1 CPU2
sys_select receive packet
... ...
__add_wait_queue update tp->rcv_nxt
... ...
tp->rcv_nxt check sock_def_readable
... {
schedule ...
if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
wake_up_interruptible(sk->sk_sleep)
...
}
If there was no cache the code would work ok, since the wait_queue and
rcv_nxt are opposit to each other.
Meaning that once tp->rcv_nxt is updated by CPU2, the CPU1 either already
passed the tp->rcv_nxt check and sleeps, or will get the new value for
tp->rcv_nxt and will return with new data mask.
In both cases the process (CPU1) is being added to the wait queue, so the
waitqueue_active (CPU2) call cannot miss and will wake up CPU1.
The bad case is when the __add_wait_queue changes done by CPU1 stay in its
cache, and so does the tp->rcv_nxt update on CPU2 side. The CPU1 will then
endup calling schedule and sleep forever if there are no more data on the
socket.
wbr,
jirka
Signed-off-by: Jiri Olsa <redacted>
---
arch/x86/include/asm/spinlock.h | 3 +++
fs/select.c | 4 ++++
include/linux/spinlock.h | 5 +++++
include/net/sock.h | 18 ++++++++++++++++++
net/atm/common.c | 4 ++--
net/core/sock.c | 8 ++++----
net/dccp/output.c | 2 +-
net/iucv/af_iucv.c | 2 +-
net/rxrpc/af_rxrpc.c | 2 +-
net/unix/af_unix.c | 2 +-
10 files changed, 40 insertions(+), 10 deletions(-)
@@ -302,4 +302,7 @@ static inline void __raw_write_unlock(raw_rwlock_t *rw)#define _raw_read_relax(lock) cpu_relax()#define _raw_write_relax(lock) cpu_relax()+/* The {read|write|spin}_lock() on x86 are full memory barriers. */+#define smp_mb__after_lock() do { } while (0)+#endif /* _ASM_X86_SPINLOCK_H */
@@ -219,6 +219,10 @@ static void __pollwait(struct file *filp, wait_queue_head_t *wait_address,init_waitqueue_func_entry(&entry->wait,pollwake);entry->wait.private=pwq;add_wait_queue(wait_address,&entry->wait);++/* This memory barrier is paired with the smp_mb__after_lock+*inthesk_has_sleeper.*/+smp_mb();}intpoll_schedule_timeout(structpoll_wqueues*pwq,intstate,
@@ -132,6 +132,11 @@ do { \#endif /*__raw_spin_is_contended*/#endif+/* The lock does not imply full memory barrier. */+#ifndef smp_mb__after_lock+#define smp_mb__after_lock() smp_mb()+#endif+/***spin_unlock_wait-waituntilthespinlockgetsunlocked*@lock:thespinlockinquestion.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Adding memory barrier to the __pollwait function paired with
receive callbacks. The smp_mb__after_lock define is added,
since {read|write|spin}_lock() on x86 are full memory barriers.
The race fires, when following code paths meet, and the tp->rcv_nxt and
__add_wait_queue updates stay in CPU caches.
CPU1 CPU2
sys_select receive packet
... ...
__add_wait_queue update tp->rcv_nxt
... ...
tp->rcv_nxt check sock_def_readable
... {
schedule ...
if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
wake_up_interruptible(sk->sk_sleep)
...
}
If there was no cache the code would work ok, since the wait_queue and
rcv_nxt are opposit to each other.
Meaning that once tp->rcv_nxt is updated by CPU2, the CPU1 either already
passed the tp->rcv_nxt check and sleeps, or will get the new value for
tp->rcv_nxt and will return with new data mask.
In both cases the process (CPU1) is being added to the wait queue, so the
waitqueue_active (CPU2) call cannot miss and will wake up CPU1.
The bad case is when the __add_wait_queue changes done by CPU1 stay in its
cache, and so does the tp->rcv_nxt update on CPU2 side. The CPU1 will then
endup calling schedule and sleep forever if there are no more data on the
socket.
So, the problem is the half barrier semantics of spin_lock on CPU1's
side and lack of any barrier (read for tp->rcv_nxt check can creep
above the queuelist update) in waitqueue_active() check on CPU2's side
(read for waitqueue list can creep above tcv_nxt update). Am I
understanding it right?
This is a little bit scary. The interface kind of suggests that they
have strong enough barrier semantics (well, I would assume that). I
wonder whether there are more more places where this kind of race
condition exists.
quoted hunk
@@ -219,6 +219,10 @@ static void __pollwait(struct file *filp, wait_queue_head_t *wait_address, init_waitqueue_func_entry(&entry->wait, pollwake); entry->wait.private = pwq; add_wait_queue(wait_address, &entry->wait);++ /* This memory barrier is paired with the smp_mb__after_lock+ * in the sk_has_sleeper. */+ smp_mb();
I'm not entirely sure this is the correct place to do it while the mb
for the other side lives in network code. Wouldn't it be better to
move this memory barrier to network select code? It's strange for an
API to have only single side of a barrier pair and leave the other to
the API user.
Also, maybe we need smp_mb__after_unlock() too? Maybe
add_wait_queue_mb() possibly paired with wait_queue_active_mb() is
better? On x86, it wouldn't make any difference tho.
One more thing, can you please use fully winged style for multiline
comments?
Thanks.
--
tejun
From: Eric Dumazet <hidden> Date: 2009-06-26 01:59:43
Davide Libenzi a écrit :
On Thu, 25 Jun 2009, Oleg Nesterov wrote:
quoted
Can't really comment this patch, except this all looks reasonable to me.
Add more CCs.
While this can work, IMO it'd be cleaner to have the smp_mb() moved from
fs/select.c to the ->poll() function.
Having a barrier that matches another one in another susbsystem, because
of the special locking logic of such subsystem, is not too shiny IMHO.
Yes but barrier is necessary only if add_wait_queue() was actually called, and __pollwait()
does this call.
Adding a plain smp_mb() in tcp_poll() for example would slowdown select()/poll() with NULL
timeout.
Adding a cond test before smp_mb() in tcp_poll() (and other ->poll() functions)
would be litle bit overkill too...
I believe this race was not existent in the past because spin_unlock() had a memory barrier,
and we changed this to a plain memory write at some point...
Most add_wait_queue() calls are followed by a call to set_current_state()
so a proper smp_mb() is explicitly included.
quoted
On 06/25, Jiri Olsa wrote:
quoted
Adding memory barrier to the __pollwait function paired with
receive callbacks. The smp_mb__after_lock define is added,
since {read|write|spin}_lock() on x86 are full memory barriers.
The race fires, when following code paths meet, and the tp->rcv_nxt and
__add_wait_queue updates stay in CPU caches.
CPU1 CPU2
sys_select receive packet
... ...
__add_wait_queue update tp->rcv_nxt
... ...
tp->rcv_nxt check sock_def_readable
... {
schedule ...
if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
wake_up_interruptible(sk->sk_sleep)
...
}
If there was no cache the code would work ok, since the wait_queue and
rcv_nxt are opposit to each other.
Meaning that once tp->rcv_nxt is updated by CPU2, the CPU1 either already
passed the tp->rcv_nxt check and sleeps, or will get the new value for
tp->rcv_nxt and will return with new data mask.
In both cases the process (CPU1) is being added to the wait queue, so the
waitqueue_active (CPU2) call cannot miss and will wake up CPU1.
The bad case is when the __add_wait_queue changes done by CPU1 stay in its
cache, and so does the tp->rcv_nxt update on CPU2 side. The CPU1 will then
endup calling schedule and sleep forever if there are no more data on the
socket.
wbr,
jirka
Signed-off-by: Jiri Olsa <redacted>
---
arch/x86/include/asm/spinlock.h | 3 +++
fs/select.c | 4 ++++
include/linux/spinlock.h | 5 +++++
include/net/sock.h | 18 ++++++++++++++++++
net/atm/common.c | 4 ++--
net/core/sock.c | 8 ++++----
net/dccp/output.c | 2 +-
net/iucv/af_iucv.c | 2 +-
net/rxrpc/af_rxrpc.c | 2 +-
net/unix/af_unix.c | 2 +-
10 files changed, 40 insertions(+), 10 deletions(-)
@@ -302,4 +302,7 @@ static inline void __raw_write_unlock(raw_rwlock_t *rw)#define _raw_read_relax(lock) cpu_relax()#define _raw_write_relax(lock) cpu_relax()+/* The {read|write|spin}_lock() on x86 are full memory barriers. */+#define smp_mb__after_lock() do { } while (0)+#endif /* _ASM_X86_SPINLOCK_H */
@@ -219,6 +219,10 @@ static void __pollwait(struct file *filp, wait_queue_head_t *wait_address,init_waitqueue_func_entry(&entry->wait,pollwake);entry->wait.private=pwq;add_wait_queue(wait_address,&entry->wait);++/* This memory barrier is paired with the smp_mb__after_lock+*inthesk_has_sleeper.*/+smp_mb();}intpoll_schedule_timeout(structpoll_wqueues*pwq,intstate,
@@ -132,6 +132,11 @@ do { \#endif /*__raw_spin_is_contended*/#endif+/* The lock does not imply full memory barrier. */+#ifndef smp_mb__after_lock+#define smp_mb__after_lock() smp_mb()+#endif+/***spin_unlock_wait-waituntilthespinlockgetsunlocked*@lock:thespinlockinquestion.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Can't really comment this patch, except this all looks reasonable to me.
Add more CCs.
While this can work, IMO it'd be cleaner to have the smp_mb() moved from
fs/select.c to the ->poll() function.
Having a barrier that matches another one in another susbsystem, because
of the special locking logic of such subsystem, is not too shiny IMHO.
Yes but barrier is necessary only if add_wait_queue() was actually called, and __pollwait()
does this call.
Adding a plain smp_mb() in tcp_poll() for example would slowdown select()/poll() with NULL
timeout.
Do you think of it as good design adding an MB on a subsystem, because of
the special locking logic of another one?
The (eventual) slowdown, IMO can be argued sideways, by saying that
non-socket users will pay the price for their polls.
- Davide
Adding a plain smp_mb() in tcp_poll() for example would slowdown select()/poll() with NULL
timeout.
Do you think of it as good design adding an MB on a subsystem, because of
the special locking logic of another one?
The (eventual) slowdown, IMO can be argued sideways, by saying that
non-socket users will pay the price for their polls.
Perhaps every performance argument is moot since spin_unlock() used to
have the barrier :-)
From: Eric Dumazet <hidden> Date: 2009-06-26 02:20:15
Davide Libenzi a écrit :
On Fri, 26 Jun 2009, Eric Dumazet wrote:
quoted
Davide Libenzi a écrit :
quoted
On Thu, 25 Jun 2009, Oleg Nesterov wrote:
quoted
Can't really comment this patch, except this all looks reasonable to me.
Add more CCs.
While this can work, IMO it'd be cleaner to have the smp_mb() moved from
fs/select.c to the ->poll() function.
Having a barrier that matches another one in another susbsystem, because
of the special locking logic of such subsystem, is not too shiny IMHO.
Yes but barrier is necessary only if add_wait_queue() was actually called, and __pollwait()
does this call.
Adding a plain smp_mb() in tcp_poll() for example would slowdown select()/poll() with NULL
timeout.
Do you think of it as good design adding an MB on a subsystem, because of
the special locking logic of another one?
The (eventual) slowdown, IMO can be argued sideways, by saying that
non-socket users will pay the price for their polls.
I wont argue with you David, just try to correct bugs.
fs/ext4/ioctl.c line 182
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&EXT4_SB(sb)->ro_wait_queue, &wait);
if (timer_pending(&EXT4_SB(sb)->turn_ro_timer)) {
schedule();
Another example of missing barrier after add_wait_queue()
Because add_wait_queue() misses a barrier, we have to add one after each call.
Maybe it would be safer to add barrier in add_wait_queue() itself, not in _pollwait().
I wont argue with you David, just try to correct bugs.
fs/ext4/ioctl.c line 182
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&EXT4_SB(sb)->ro_wait_queue, &wait);
if (timer_pending(&EXT4_SB(sb)->turn_ro_timer)) {
schedule();
Another example of missing barrier after add_wait_queue()
Because add_wait_queue() misses a barrier, we have to add one after each call.
Maybe it would be safer to add barrier in add_wait_queue() itself, not in _pollwait().
Not all the code that uses add_wait_queue() does need to have the MB,
like code that does the most common pattern:
xxx_poll(...) {
poll_wait(...);
lock();
flags = calc_flags(->status);
unlock();
return flags;
}
xxx_update(...) {
lock();
->status = ...;
unlock();
if (waitqueue_active())
wake_up();
}
It's the code that does the lockless flags calculation in ->poll that
might need it.
I dunno what the amount of changes are, but cross-matching MB across
subsystems does not look nice.
IMHO that's a detail of the subsystem locking, and should be confined
inside the subsystem itself.
No?
- Davide
I wont argue with you David, just try to correct bugs.
fs/ext4/ioctl.c line 182
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&EXT4_SB(sb)->ro_wait_queue, &wait);
if (timer_pending(&EXT4_SB(sb)->turn_ro_timer)) {
schedule();
Another example of missing barrier after add_wait_queue()
Because add_wait_queue() misses a barrier, we have to add one after each call.
Maybe it would be safer to add barrier in add_wait_queue() itself, not in _pollwait().
Not all the code that uses add_wait_queue() does need to have the MB,
like code that does the most common pattern:
xxx_poll(...) {
poll_wait(...);
lock();
flags = calc_flags(->status);
unlock();
return flags;
}
xxx_update(...) {
lock();
->status = ...;
unlock();
if (waitqueue_active())
wake_up();
}
It's the code that does the lockless flags calculation in ->poll that
might need it.
I dunno what the amount of changes are, but cross-matching MB across
subsystems does not look nice.
IMHO that's a detail of the subsystem locking, and should be confined
inside the subsystem itself.
No?
How about poll_wait_mb() and waitqueue_active_mb() (with mb and
additional check for NULL of wait_queue_head)?
Jarek P.
On Fri, Jun 26, 2009 at 05:42:30AM +0000, Jarek Poplawski wrote:
On 26-06-2009 05:14, Davide Libenzi wrote:
quoted
On Fri, 26 Jun 2009, Eric Dumazet wrote:
quoted
I wont argue with you David, just try to correct bugs.
fs/ext4/ioctl.c line 182
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&EXT4_SB(sb)->ro_wait_queue, &wait);
if (timer_pending(&EXT4_SB(sb)->turn_ro_timer)) {
schedule();
Another example of missing barrier after add_wait_queue()
Because add_wait_queue() misses a barrier, we have to add one after each call.
Maybe it would be safer to add barrier in add_wait_queue() itself, not in _pollwait().
Not all the code that uses add_wait_queue() does need to have the MB,
like code that does the most common pattern:
xxx_poll(...) {
poll_wait(...);
lock();
flags = calc_flags(->status);
unlock();
return flags;
}
xxx_update(...) {
lock();
->status = ...;
unlock();
if (waitqueue_active())
wake_up();
}
It's the code that does the lockless flags calculation in ->poll that
might need it.
I dunno what the amount of changes are, but cross-matching MB across
subsystems does not look nice.
IMHO that's a detail of the subsystem locking, and should be confined
inside the subsystem itself.
No?
How about poll_wait_mb() and waitqueue_active_mb() (with mb and
additional check for NULL of wait_queue_head)?
Hmm... But considering Eric's arguments I see it would be hard
/impossible to do it with the current api, so let's forget.
Jarek P.
Do you think of it as good design adding an MB on a subsystem, because of
the special locking logic of another one?
The (eventual) slowdown, IMO can be argued sideways, by saying that
non-socket users will pay the price for their polls.
I wont argue with you David, just try to correct bugs.
I must admit, I agree with David.
fs/ext4/ioctl.c line 182
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&EXT4_SB(sb)->ro_wait_queue, &wait);
if (timer_pending(&EXT4_SB(sb)->turn_ro_timer)) {
schedule();
Another example of missing barrier after add_wait_queue()
Assuming that ->turn_ro_timer does wake_up(->ro_wait_queue) everything
is OK, we do not need a barrier.
Oleg.
Not all the code that uses add_wait_queue() does need to have the MB,
like code that does the most common pattern:
xxx_poll(...) {
poll_wait(...);
lock();
flags = calc_flags(->status);
unlock();
return flags;
}
xxx_update(...) {
lock();
->status = ...;
unlock();
if (waitqueue_active())
wake_up();
}
It's the code that does the lockless flags calculation in ->poll that
might need it.
And if we remove waitqueue_active() in xxx_update(), then lock/unlock is
not needed too.
If xxx_poll() takes q->lock first, it can safely miss the changes in ->status
and schedule(): xxx_update() will take q->lock, notice the sleeper and wake
it up (ok, it will set ->triggered but this doesn't matter).
If xxx_update() takes q->lock first, xxx_poll() must see the changes in
status after poll_wait()->unlock(&q->lock) (in fact, after lock, not unlock).
Oleg.
And if we remove waitqueue_active() in xxx_update(), then lock/unlock is
not needed too.
If xxx_poll() takes q->lock first, it can safely miss the changes in ->status
and schedule(): xxx_update() will take q->lock, notice the sleeper and wake
it up (ok, it will set ->triggered but this doesn't matter).
If xxx_update() takes q->lock first, xxx_poll() must see the changes in
status after poll_wait()->unlock(&q->lock) (in fact, after lock, not unlock).
Sure. The snippet above was just to show what typically the code does, not
a suggestion on how to solve the socket case.
But yeah, the problem in this case is the waitqueue_active() call. Without
that, the wait queue lock/unlock in poll_wait() and the one in wake_up()
guarantees the necessary barriers.
Some might argue the costs of the lock/unlock of q->lock, and wonder if
MBs are a more efficient solution. This is something I'm not going into.
To me, it just looked not right having cross-matching MB in different
subsystems.
- Davide
And if we remove waitqueue_active() in xxx_update(), then lock/unlock is
not needed too.
If xxx_poll() takes q->lock first, it can safely miss the changes in ->status
and schedule(): xxx_update() will take q->lock, notice the sleeper and wake
it up (ok, it will set ->triggered but this doesn't matter).
If xxx_update() takes q->lock first, xxx_poll() must see the changes in
status after poll_wait()->unlock(&q->lock) (in fact, after lock, not unlock).
Sure. The snippet above was just to show what typically the code does, not
a suggestion on how to solve the socket case.
Yes, yes. I just meant you are right imho, we shouldn't add mb() into
add_wait_queue().
But yeah, the problem in this case is the waitqueue_active() call. Without
that, the wait queue lock/unlock in poll_wait() and the one in wake_up()
guarantees the necessary barriers.
Some might argue the costs of the lock/unlock of q->lock, and wonder if
MBs are a more efficient solution. This is something I'm not going into.
To me, it just looked not right having cross-matching MB in different
subsystems.
This is subjective and thus up to maintainers, but personally I think you
are very, very right.
Perhaps we can add
void sock_poll_wait(struct file *file, struct sock *sk, poll_table *pt)
{
if (pt) {
poll_wait(file, sk->sk_sleep, pt);
/*
* fat comment
*/
smp_mb(); // or smp_mb__after_unlock();
}
}
Oleg.
And if we remove waitqueue_active() in xxx_update(), then lock/unlock is
not needed too.
If xxx_poll() takes q->lock first, it can safely miss the changes in ->status
and schedule(): xxx_update() will take q->lock, notice the sleeper and wake
it up (ok, it will set ->triggered but this doesn't matter).
If xxx_update() takes q->lock first, xxx_poll() must see the changes in
status after poll_wait()->unlock(&q->lock) (in fact, after lock, not unlock).
Sure. The snippet above was just to show what typically the code does, not
a suggestion on how to solve the socket case.
Yes, yes. I just meant you are right imho, we shouldn't add mb() into
add_wait_queue().
quoted
But yeah, the problem in this case is the waitqueue_active() call. Without
that, the wait queue lock/unlock in poll_wait() and the one in wake_up()
guarantees the necessary barriers.
Some might argue the costs of the lock/unlock of q->lock, and wonder if
MBs are a more efficient solution. This is something I'm not going into.
To me, it just looked not right having cross-matching MB in different
subsystems.
This is subjective and thus up to maintainers, but personally I think you
are very, very right.
Perhaps we can add
void sock_poll_wait(struct file *file, struct sock *sk, poll_table *pt)
{
if (pt) {
poll_wait(file, sk->sk_sleep, pt);
/*
* fat comment
*/
smp_mb(); // or smp_mb__after_unlock();
}
}
Oleg.
And if we remove waitqueue_active() in xxx_update(), then lock/unlock is
not needed too.
If xxx_poll() takes q->lock first, it can safely miss the changes in ->status
and schedule(): xxx_update() will take q->lock, notice the sleeper and wake
it up (ok, it will set ->triggered but this doesn't matter).
If xxx_update() takes q->lock first, xxx_poll() must see the changes in
status after poll_wait()->unlock(&q->lock) (in fact, after lock, not unlock).
Sure. The snippet above was just to show what typically the code does, not
a suggestion on how to solve the socket case.
Yes, yes. I just meant you are right imho, we shouldn't add mb() into
add_wait_queue().
quoted
But yeah, the problem in this case is the waitqueue_active() call. Without
that, the wait queue lock/unlock in poll_wait() and the one in wake_up()
guarantees the necessary barriers.
Some might argue the costs of the lock/unlock of q->lock, and wonder if
MBs are a more efficient solution. This is something I'm not going into.
To me, it just looked not right having cross-matching MB in different
subsystems.
This is subjective and thus up to maintainers, but personally I think you
are very, very right.
Perhaps we can add
void sock_poll_wait(struct file *file, struct sock *sk, poll_table *pt)
{
if (pt) {
poll_wait(file, sk->sk_sleep, pt);
/*
* fat comment
*/
smp_mb(); // or smp_mb__after_unlock();
}
}
Oleg.
Perhaps it makes sense to check ->sk_sleep != NULL in sock_poll_wait(), but
I don't think we need __poll_wait(). poll_wait() is inline, I think gcc
will optimize out "if (p && wait_address)" check if poll_wait() is called
from sock_poll_wait().
This all is up to Jiri of course. But speaking about cosmetic changes, I
think it is better to make 2 patches. The first one fixes the problem using
smp_mb(), another introduces smp_mb__xxx_lock() to optimize the code.
Oleg.
Perhaps it makes sense to check ->sk_sleep != NULL in sock_poll_wait(), but
I don't think we need __poll_wait(). poll_wait() is inline, I think gcc
will optimize out "if (p && wait_address)" check if poll_wait() is called
from sock_poll_wait().
Sure, to me it looks a bit more readable, but let Jiri choose.;-)
Cheers,
Jarek P.
Perhaps it makes sense to check ->sk_sleep != NULL in sock_poll_wait(), but
I don't think we need __poll_wait(). poll_wait() is inline, I think gcc
will optimize out "if (p && wait_address)" check if poll_wait() is called
from sock_poll_wait().
Sure, to me it looks a bit more readable, but let Jiri choose.;-)
Cheers,
Jarek P.
yes :) I like more Jarek's way.. and I'll send separate patch for the
smp_mb_after_lock change.
thanks,
jirka
Perhaps we can add
void sock_poll_wait(struct file *file, struct sock *sk, poll_table *pt)
{
if (pt) {
poll_wait(file, sk->sk_sleep, pt);
/*
* fat comment
*/
smp_mb(); // or smp_mb__after_unlock();
}
}
That'd be fine IMHO. Are DaveM and Eric OK?
No objections from me.
Very good :)
Jiri, please respin a patch with this idea from Oleg
(We'll have to check all calls to poll_wait() in net tree)
Thanks everybody
thanks a lot! :) I'll send out new patch shortly..
I'll include all the poll_wait calls change I'm kind of sure of,
and list of others I'm not, so we can discuss them.
jirka