| Message ID | ZfLdv5DZvBg0wajJ@libra05 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net] rds: introduce acquire/release ordering in acquire/release_in_xmit() | expand |
On Thu, Mar 14, 2024 at 08:21:35PM +0900, Yewon Choi wrote: > acquire/release_in_xmit() work as bit lock in rds_send_xmit(), so they > are expected to ensure acquire/release memory ordering semantics. > However, test_and_set_bit/clear_bit() don't imply such semantics, on > top of this, following smp_mb__after_atomic() does not guarantee release > ordering (memory barrier actually should be placed before clear_bit()). > > Instead, we use clear_bit_unlock/test_and_set_bit_lock() here. > > Signed-off-by: Yewon Choi <woni9911@gmail.com> Missing "Fixes" tag for the patch addressed to the "net" tree. Thanks, Michal
On Thu, 2024-03-14 at 12:51 +0100, Michal Kubiak wrote: > On Thu, Mar 14, 2024 at 08:21:35PM +0900, Yewon Choi wrote: > > acquire/release_in_xmit() work as bit lock in rds_send_xmit(), so > > they > > are expected to ensure acquire/release memory ordering semantics. > > However, test_and_set_bit/clear_bit() don't imply such semantics, > > on > > top of this, following smp_mb__after_atomic() does not guarantee > > release > > ordering (memory barrier actually should be placed before > > clear_bit()). > > > > Instead, we use clear_bit_unlock/test_and_set_bit_lock() here. > > > > Signed-off-by: Yewon Choi <woni9911@gmail.com> > > Missing "Fixes" tag for the patch addressed to the "net" tree. > > Thanks, > Michal Yes, I think it needs: Fixes: 1f9ecd7eacfd ("RDS: Pass rds_conn_path to rds_send_xmit()") Since that is the last patch to modify the affected code. Other than that I think the patch looks good. With the tag fixed, you can add my rvb: Reviewed-by: Allison Henderson <allison.henderson@oracle.com> >
On Thu, Mar 14, 2024 at 10:37:29PM +0000, Allison Henderson wrote: > On Thu, 2024-03-14 at 12:51 +0100, Michal Kubiak wrote: > > On Thu, Mar 14, 2024 at 08:21:35PM +0900, Yewon Choi wrote: > > > acquire/release_in_xmit() work as bit lock in rds_send_xmit(), so > > > they > > > are expected to ensure acquire/release memory ordering semantics. > > > However, test_and_set_bit/clear_bit() don't imply such semantics, > > > on > > > top of this, following smp_mb__after_atomic() does not guarantee > > > release > > > ordering (memory barrier actually should be placed before > > > clear_bit()). > > > > > > Instead, we use clear_bit_unlock/test_and_set_bit_lock() here. > > > > > > Signed-off-by: Yewon Choi <woni9911@gmail.com> > > > > Missing "Fixes" tag for the patch addressed to the "net" tree. > > Sorry for mistake, I'll correct this and send v2 patch. > > Thanks, > > Michal > > Yes, I think it needs: > > Fixes: 1f9ecd7eacfd ("RDS: Pass rds_conn_path to rds_send_xmit()") > > Since that is the last patch to modify the affected code. Other than > that I think the patch looks good. With the tag fixed, you can add my > rvb: > Also, test_and_set_bit/clear_bit() was first introduced in commit 0f4b1c7e89e6. I think this can be added, too: Fixes: 0f4b1c7e89e6 ("rds: fix rds_send_xmit() serialization") > Reviewed-by: Allison Henderson <allison.henderson@oracle.com> > > > Thank you for the reviewing. Sincerely, Yewon Choi
diff --git a/net/rds/send.c b/net/rds/send.c index 5e57a1581dc6..8f38009721b7 100644 --- a/net/rds/send.c +++ b/net/rds/send.c @@ -103,13 +103,12 @@ EXPORT_SYMBOL_GPL(rds_send_path_reset); static int acquire_in_xmit(struct rds_conn_path *cp) { - return test_and_set_bit(RDS_IN_XMIT, &cp->cp_flags) == 0; + return test_and_set_bit_lock(RDS_IN_XMIT, &cp->cp_flags) == 0; } static void release_in_xmit(struct rds_conn_path *cp) { - clear_bit(RDS_IN_XMIT, &cp->cp_flags); - smp_mb__after_atomic(); + clear_bit_unlock(RDS_IN_XMIT, &cp->cp_flags); /* * We don't use wait_on_bit()/wake_up_bit() because our waking is in a * hot path and finding waiters is very rare. We don't want to walk
acquire/release_in_xmit() work as bit lock in rds_send_xmit(), so they are expected to ensure acquire/release memory ordering semantics. However, test_and_set_bit/clear_bit() don't imply such semantics, on top of this, following smp_mb__after_atomic() does not guarantee release ordering (memory barrier actually should be placed before clear_bit()). Instead, we use clear_bit_unlock/test_and_set_bit_lock() here. Signed-off-by: Yewon Choi <woni9911@gmail.com> --- net/rds/send.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)