* [RFC][PATCH] wake_up_var() memory ordering
@ 2019-06-24 16:50 ` Peter Zijlstra
0 siblings, 0 replies; 14+ messages in thread
From: Peter Zijlstra @ 2019-06-24 16:50 UTC (permalink / raw)
To: Paul McKenney, Will Deacon
Cc: Martin Brandenburg, linux-cachefs-H+wXaHxf7aLQT0dZR+AlfA,
Mike Snitzer, linux-aio-Bw31MaZKKs3YtjvyW6yDsg, David Airlie,
samba-technical-w/Ol4Ecudpl8XjKLYN78aQ, Joonas Lahtinen,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, David Howells,
Chris Mason, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
keyrings-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
linux-afs-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Alasdair Kergon,
Mike Marshall, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
rds-devel-N0ozoZBvEnrZJqsBc5GL+g, Andreas Gruenbacher,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, James Morris,
cluster-devel-H+wXaHxf7aLQT0dZR+AlfA, Peter Zijlstra,
Antti Palosaari, Matthias Brugger,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
devel-urhm25FSIHzptaDgeN77Px2eb7JE58TQ, Serge
Hi all,
I tried using wake_up_var() today and accidentally noticed that it
didn't imply an smp_mb() and specifically requires it through
wake_up_bit() / waitqueue_active().
Now, wake_up_bit() doesn't imply the barrier because it is assumed to be
used with the atomic bitops API which either implies (test_and_clear) or
only needs smp_mb__after_atomic(), which is 'much' cheaper than an
unconditional smp_mb().
Still, while auditing all that, I found a whole bunch of things that
could be improved. There were missing barriers, superfluous barriers and
a whole bunch of sites that could use clear_and_wake_up_bit().
So this fixes all wake_up_bit() usage without actually changing
semantics of it (which are unfortunate but understandable). This does
however change the semantics of wake_up_var(); even though wake_up_var()
is most often used with atomics and then the additional smp_mb() is most
often superfluous :/
There isn't really a good option here, comments (other than I need to
split this up)?
---
drivers/bluetooth/btmtksdio.c | 5 +----
drivers/bluetooth/btmtkuart.c | 5 +----
drivers/bluetooth/hci_mrvl.c | 8 ++------
drivers/gpu/drm/i915/i915_reset.c | 6 ++----
drivers/md/dm-bufio.c | 10 ++--------
drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 15 ++++-----------
fs/afs/fs_probe.c | 1 +
fs/afs/server.c | 1 +
fs/afs/vl_probe.c | 1 +
fs/afs/volume.c | 1 +
fs/aio.c | 4 +---
fs/block_dev.c | 1 +
fs/btrfs/extent_io.c | 4 +---
fs/cachefiles/namei.c | 1 +
fs/cifs/connect.c | 3 +--
fs/cifs/misc.c | 15 +++++----------
fs/fscache/cookie.c | 2 ++
fs/fscache/object.c | 2 ++
fs/fscache/page.c | 3 +++
fs/gfs2/glock.c | 8 ++------
fs/gfs2/glops.c | 1 +
fs/gfs2/lock_dlm.c | 8 ++------
fs/gfs2/recovery.c | 4 +---
fs/gfs2/super.c | 1 +
fs/gfs2/sys.c | 4 +---
fs/nfs/nfs4state.c | 4 +---
fs/nfs/pnfs_nfs.c | 4 +---
fs/nfsd/nfs4recover.c | 4 +---
fs/orangefs/file.c | 2 +-
kernel/sched/wait_bit.c | 1 +
net/bluetooth/hci_event.c | 5 +----
net/rds/ib_recv.c | 1 +
security/keys/gc.c | 5 ++---
33 files changed, 50 insertions(+), 90 deletions(-)
diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index 813338288453..27523cfeac9a 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -356,11 +356,8 @@ static int btmtksdio_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
if (hdr->evt == HCI_EV_VENDOR) {
if (test_and_clear_bit(BTMTKSDIO_TX_WAIT_VND_EVT,
- &bdev->tx_state)) {
- /* Barrier to sync with other CPUs */
- smp_mb__after_atomic();
+ &bdev->tx_state))
wake_up_bit(&bdev->tx_state, BTMTKSDIO_TX_WAIT_VND_EVT);
- }
}
return 0;
diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
index f5dbeec8e274..7fe324df3799 100644
--- a/drivers/bluetooth/btmtkuart.c
+++ b/drivers/bluetooth/btmtkuart.c
@@ -340,11 +340,8 @@ static int btmtkuart_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
if (hdr->evt == HCI_EV_VENDOR) {
if (test_and_clear_bit(BTMTKUART_TX_WAIT_VND_EVT,
- &bdev->tx_state)) {
- /* Barrier to sync with other CPUs */
- smp_mb__after_atomic();
+ &bdev->tx_state))
wake_up_bit(&bdev->tx_state, BTMTKUART_TX_WAIT_VND_EVT);
- }
}
return 0;
diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
index 50212ac629e3..f03294d39d08 100644
--- a/drivers/bluetooth/hci_mrvl.c
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -157,9 +157,7 @@ static int mrvl_recv_fw_req(struct hci_dev *hdev, struct sk_buff *skb)
mrvl->tx_len = le16_to_cpu(pkt->lhs);
- clear_bit(STATE_FW_REQ_PENDING, &mrvl->flags);
- smp_mb__after_atomic();
- wake_up_bit(&mrvl->flags, STATE_FW_REQ_PENDING);
+ clear_and_wake_up_bit(STATE_FW_REQ_PENDING, &mrvl->flags);
done:
kfree_skb(skb);
@@ -192,9 +190,7 @@ static int mrvl_recv_chip_ver(struct hci_dev *hdev, struct sk_buff *skb)
bt_dev_info(hdev, "Controller id = %x, rev = %x", mrvl->id, mrvl->rev);
- clear_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
- smp_mb__after_atomic();
- wake_up_bit(&mrvl->flags, STATE_CHIP_VER_PENDING);
+ clear_and_wake_up_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
done:
kfree_skb(skb);
diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
index 677d59304e78..6809367dbfa9 100644
--- a/drivers/gpu/drm/i915/i915_reset.c
+++ b/drivers/gpu/drm/i915/i915_reset.c
@@ -1324,10 +1324,8 @@ void i915_handle_error(struct drm_i915_private *i915,
if (i915_reset_engine(engine, msg) == 0)
engine_mask &= ~engine->mask;
- clear_bit(I915_RESET_ENGINE + engine->id,
- &error->flags);
- wake_up_bit(&error->flags,
- I915_RESET_ENGINE + engine->id);
+ clear_and_wake_up_bit(I915_RESET_ENGINE + engine->id,
+ &error->flags);
}
}
diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 2a48ea3f1b30..3114836cc717 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -667,10 +667,7 @@ static void write_endio(struct dm_buffer *b, blk_status_t status)
BUG_ON(!test_bit(B_WRITING, &b->state));
smp_mb__before_atomic();
- clear_bit(B_WRITING, &b->state);
- smp_mb__after_atomic();
-
- wake_up_bit(&b->state, B_WRITING);
+ clear_and_wake_up_bit(B_WRITING, &b->state);
}
/*
@@ -1045,10 +1042,7 @@ static void read_endio(struct dm_buffer *b, blk_status_t status)
BUG_ON(!test_bit(B_READING, &b->state));
smp_mb__before_atomic();
- clear_bit(B_READING, &b->state);
- smp_mb__after_atomic();
-
- wake_up_bit(&b->state, B_READING);
+ clear_and_wake_up_bit(B_READING, &b->state);
}
/*
diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
index e5e056bf9dfa..fa46bc930704 100644
--- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
+++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
@@ -374,9 +374,7 @@ static int dvb_usb_stop_feed(struct dvb_demux_feed *dvbdmxfeed)
usb_urb_killv2(&adap->stream);
/* clear 'streaming' status bit */
- clear_bit(ADAP_STREAMING, &adap->state_bits);
- smp_mb__after_atomic();
- wake_up_bit(&adap->state_bits, ADAP_STREAMING);
+ clear_and_wake_up_bit(ADAP_STREAMING, &adap->state_bits);
skip_feed_stop:
if (ret)
@@ -578,11 +576,8 @@ static int dvb_usb_fe_init(struct dvb_frontend *fe)
goto err;
}
err:
- if (!adap->suspend_resume_active) {
- clear_bit(ADAP_INIT, &adap->state_bits);
- smp_mb__after_atomic();
- wake_up_bit(&adap->state_bits, ADAP_INIT);
- }
+ if (!adap->suspend_resume_active)
+ clear_and_wake_up_bit(ADAP_INIT, &adap->state_bits);
dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
return ret;
@@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
err:
if (!adap->suspend_resume_active) {
adap->active_fe = -1;
- clear_bit(ADAP_SLEEP, &adap->state_bits);
- smp_mb__after_atomic();
- wake_up_bit(&adap->state_bits, ADAP_SLEEP);
+ clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
}
dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
index cfe62b154f68..377ee07d5f76 100644
--- a/fs/afs/fs_probe.c
+++ b/fs/afs/fs_probe.c
@@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
wake_up_var(&server->probe_outstanding);
clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
+ smp_mb__after_atomic();
wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
return true;
}
diff --git a/fs/afs/server.c b/fs/afs/server.c
index e900cd74361b..c9b74a397cc1 100644
--- a/fs/afs/server.c
+++ b/fs/afs/server.c
@@ -569,6 +569,7 @@ bool afs_check_server_record(struct afs_fs_cursor *fc, struct afs_server *server
if (!test_and_set_bit_lock(AFS_SERVER_FL_UPDATING, &server->flags)) {
success = afs_update_server_record(fc, server);
clear_bit_unlock(AFS_SERVER_FL_UPDATING, &server->flags);
+ smp_mb__after_atomic();
wake_up_bit(&server->flags, AFS_SERVER_FL_UPDATING);
_leave(" = %d", success);
return success;
diff --git a/fs/afs/vl_probe.c b/fs/afs/vl_probe.c
index 858498cc1b05..1939624aa147 100644
--- a/fs/afs/vl_probe.c
+++ b/fs/afs/vl_probe.c
@@ -18,6 +18,7 @@ static bool afs_vl_probe_done(struct afs_vlserver *server)
wake_up_var(&server->probe_outstanding);
clear_bit_unlock(AFS_VLSERVER_FL_PROBING, &server->flags);
+ smp_mb__after_atomic();
wake_up_bit(&server->flags, AFS_VLSERVER_FL_PROBING);
return true;
}
diff --git a/fs/afs/volume.c b/fs/afs/volume.c
index 08fdb3951c49..b968e4e96f6b 100644
--- a/fs/afs/volume.c
+++ b/fs/afs/volume.c
@@ -303,6 +303,7 @@ int afs_check_volume_status(struct afs_volume *volume, struct key *key)
ret = afs_update_volume_status(volume, key);
clear_bit_unlock(AFS_VOLUME_WAIT, &volume->flags);
clear_bit_unlock(AFS_VOLUME_UPDATING, &volume->flags);
+ smp_mb__after_atomic();
wake_up_bit(&volume->flags, AFS_VOLUME_WAIT);
_leave(" = %d", ret);
return ret;
diff --git a/fs/aio.c b/fs/aio.c
index 3490d1fa0e16..a21acb7ee2a5 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1154,9 +1154,7 @@ static void aio_complete(struct aio_kiocb *iocb)
* like in wake_up_bit() where clearing a bit has to be
* ordered with the unlocked test.
*/
- smp_mb();
-
- if (waitqueue_active(&ctx->wait))
+ if (wq_has_sleeper(&ctx->wait))
wake_up(&ctx->wait);
}
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 749f5984425d..80e35cbfc7cc 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1687,6 +1687,7 @@ int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
/* tell others that we're done */
BUG_ON(whole->bd_claiming != holder);
whole->bd_claiming = NULL;
+ smp_mb();
wake_up_bit(&whole->bd_claiming, 0);
spin_unlock(&bdev_lock);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index db337e53aab3..ee1136ae46fd 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3683,9 +3683,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
static void end_extent_buffer_writeback(struct extent_buffer *eb)
{
- clear_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
- smp_mb__after_atomic();
- wake_up_bit(&eb->bflags, EXTENT_BUFFER_WRITEBACK);
+ clear_and_wake_up_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
}
static void set_btree_ioerr(struct page *page)
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index ecc8ecbbfa5a..ecebba72ec7d 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -267,6 +267,7 @@ void cachefiles_mark_object_inactive(struct cachefiles_cache *cache,
clear_bit(CACHEFILES_OBJECT_ACTIVE, &object->flags);
write_unlock(&cache->active_lock);
+ smp_mb__after_atomic();
wake_up_bit(&object->flags, CACHEFILES_OBJECT_ACTIVE);
/* This object can now be culled, so we need to let the daemon know
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 8c4121da624e..8c1115ed1c28 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -5235,8 +5235,7 @@ cifs_sb_tlink(struct cifs_sb_info *cifs_sb)
}
tlink->tl_tcon = cifs_construct_tcon(cifs_sb, fsuid);
- clear_bit(TCON_LINK_PENDING, &tlink->tl_flags);
- wake_up_bit(&tlink->tl_flags, TCON_LINK_PENDING);
+ clear_and_wake_up_bit(TCON_LINK_PENDING, &tlink->tl_flags);
if (IS_ERR(tlink->tl_tcon)) {
cifs_put_tlink(tlink);
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index b1a696a73f7c..961751d89113 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -584,10 +584,8 @@ int cifs_get_writer(struct cifsInodeInfo *cinode)
/* Check to see if we have started servicing an oplock break */
if (test_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags)) {
cinode->writers--;
- if (cinode->writers == 0) {
- clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
- wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
- }
+ if (cinode->writers == 0)
+ clear_and_wake_up_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
spin_unlock(&cinode->writers_lock);
goto start;
}
@@ -599,10 +597,8 @@ void cifs_put_writer(struct cifsInodeInfo *cinode)
{
spin_lock(&cinode->writers_lock);
cinode->writers--;
- if (cinode->writers == 0) {
- clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
- wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
- }
+ if (cinode->writers == 0)
+ clear_and_wake_up_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
spin_unlock(&cinode->writers_lock);
}
@@ -630,8 +626,7 @@ void cifs_queue_oplock_break(struct cifsFileInfo *cfile)
void cifs_done_oplock_break(struct cifsInodeInfo *cinode)
{
- clear_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags);
- wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_OPLOCK_BREAK);
+ clear_and_wake_up_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags);
}
bool
diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
index 0ce39658a620..40ba8957ebf0 100644
--- a/fs/fscache/cookie.c
+++ b/fs/fscache/cookie.c
@@ -381,6 +381,7 @@ void __fscache_enable_cookie(struct fscache_cookie *cookie,
out_unlock:
clear_bit_unlock(FSCACHE_COOKIE_ENABLEMENT_LOCK, &cookie->flags);
+ smp_mb__after_atomic();
wake_up_bit(&cookie->flags, FSCACHE_COOKIE_ENABLEMENT_LOCK);
}
EXPORT_SYMBOL(__fscache_enable_cookie);
@@ -778,6 +779,7 @@ void __fscache_disable_cookie(struct fscache_cookie *cookie,
out_unlock_enable:
clear_bit_unlock(FSCACHE_COOKIE_ENABLEMENT_LOCK, &cookie->flags);
+ smp_mb__after_atomic();
wake_up_bit(&cookie->flags, FSCACHE_COOKIE_ENABLEMENT_LOCK);
_leave("");
}
diff --git a/fs/fscache/object.c b/fs/fscache/object.c
index cfeba839a0f2..08b2edec6596 100644
--- a/fs/fscache/object.c
+++ b/fs/fscache/object.c
@@ -524,6 +524,7 @@ void fscache_object_lookup_negative(struct fscache_object *object)
_debug("wake up lookup %p", &cookie->flags);
clear_bit_unlock(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags);
+ smp_mb__after_atomic();
wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
}
_leave("");
@@ -559,6 +560,7 @@ void fscache_obtained_object(struct fscache_object *object)
* to begin shovelling data.
*/
clear_bit_unlock(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags);
+ smp_mb__after_atomic();
wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
} else {
fscache_stat(&fscache_n_object_created);
diff --git a/fs/fscache/page.c b/fs/fscache/page.c
index 26af6fdf1538..49ef238502a3 100644
--- a/fs/fscache/page.c
+++ b/fs/fscache/page.c
@@ -113,6 +113,7 @@ bool __fscache_maybe_release_page(struct fscache_cookie *cookie,
fscache_stat(&fscache_n_store_vmscan_gone);
}
+ smp_mb();
wake_up_bit(&cookie->flags, 0);
trace_fscache_wake_cookie(cookie);
if (xpage)
@@ -171,6 +172,7 @@ static void fscache_end_page_write(struct fscache_object *object,
trace_fscache_page(cookie, page, fscache_page_write_end_pend);
}
spin_unlock(&cookie->stores_lock);
+ smp_mb();
wake_up_bit(&cookie->flags, 0);
trace_fscache_wake_cookie(cookie);
} else {
@@ -922,6 +924,7 @@ void fscache_invalidate_writes(struct fscache_cookie *cookie)
put_page(results[i]);
}
+ smp_mb();
wake_up_bit(&cookie->flags, 0);
trace_fscache_wake_cookie(cookie);
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index f1ebcb42cbf5..8024b6bdb6d4 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -302,9 +302,7 @@ static inline int may_grant(const struct gfs2_glock *gl, const struct gfs2_holde
static void gfs2_holder_wake(struct gfs2_holder *gh)
{
- clear_bit(HIF_WAIT, &gh->gh_iflags);
- smp_mb__after_atomic();
- wake_up_bit(&gh->gh_iflags, HIF_WAIT);
+ clear_and_wake_up_bit(HIF_WAIT, &gh->gh_iflags);
}
/**
@@ -436,9 +434,7 @@ static void state_change(struct gfs2_glock *gl, unsigned int new_state)
static void gfs2_demote_wake(struct gfs2_glock *gl)
{
gl->gl_demote_state = LM_ST_EXCLUSIVE;
- clear_bit(GLF_DEMOTE, &gl->gl_flags);
- smp_mb__after_atomic();
- wake_up_bit(&gl->gl_flags, GLF_DEMOTE);
+ clear_and_wake_up_bit(GLF_DEMOTE, &gl->gl_flags);
}
/**
diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index cf4c767005b1..666629ea5da7 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
return;
clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
+ smp_mb__after_atomic();
wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);
}
diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
index 4361804646d8..0fa1865dd600 100644
--- a/fs/gfs2/lock_dlm.c
+++ b/fs/gfs2/lock_dlm.c
@@ -1139,9 +1139,7 @@ static void gdlm_recover_done(void *arg, struct dlm_slot *slots, int num_slots,
if (!test_bit(DFL_UNMOUNT, &ls->ls_recover_flags))
queue_delayed_work(gfs2_control_wq, &sdp->sd_control_work, 0);
- clear_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
- smp_mb__after_atomic();
- wake_up_bit(&ls->ls_recover_flags, DFL_DLM_RECOVERY);
+ clear_and_wake_up_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
spin_unlock(&ls->ls_recover_spin);
}
@@ -1276,9 +1274,7 @@ static int gdlm_mount(struct gfs2_sbd *sdp, const char *table)
}
ls->ls_first = !!test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags);
- clear_bit(SDF_NOJOURNALID, &sdp->sd_flags);
- smp_mb__after_atomic();
- wake_up_bit(&sdp->sd_flags, SDF_NOJOURNALID);
+ clear_and_wake_up_bit(SDF_NOJOURNALID, &sdp->sd_flags);
return 0;
fail_release:
diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
index 2299a3fa1911..ef9a8ef6689b 100644
--- a/fs/gfs2/recovery.c
+++ b/fs/gfs2/recovery.c
@@ -436,9 +436,7 @@ void gfs2_recover_func(struct work_struct *work)
jd->jd_recover_error = error;
gfs2_recovery_done(sdp, jd->jd_jid, LM_RD_GAVEUP);
done:
- clear_bit(JDF_RECOVERY, &jd->jd_flags);
- smp_mb__after_atomic();
- wake_up_bit(&jd->jd_flags, JDF_RECOVERY);
+ clear_and_wake_up_bit(JDF_RECOVERY, &jd->jd_flags);
}
int gfs2_recover_journal(struct gfs2_jdesc *jd, bool wait)
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index b70cea5c8c59..28ff2e10bdd9 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -985,6 +985,7 @@ void gfs2_freeze_func(struct work_struct *work)
}
deactivate_super(sb);
clear_bit_unlock(SDF_FS_FROZEN, &sdp->sd_flags);
+ smp_mb__after_atomic();
wake_up_bit(&sdp->sd_flags, SDF_FS_FROZEN);
return;
}
diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index 159aedf63c2a..b337798d8957 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -505,9 +505,7 @@ static ssize_t jid_store(struct gfs2_sbd *sdp, const char *buf, size_t len)
if (sdp->sd_args.ar_spectator && jid > 0)
rv = jid = -EINVAL;
sdp->sd_lockstruct.ls_jid = jid;
- clear_bit(SDF_NOJOURNALID, &sdp->sd_flags);
- smp_mb__after_atomic();
- wake_up_bit(&sdp->sd_flags, SDF_NOJOURNALID);
+ clear_and_wake_up_bit(SDF_NOJOURNALID, &sdp->sd_flags);
out:
spin_unlock(&sdp->sd_jindex_spin);
return rv ? rv : len;
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index e2e3c4f04d3e..a3ed413a38c5 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1196,9 +1196,7 @@ static int nfs4_run_state_manager(void *);
static void nfs4_clear_state_manager_bit(struct nfs_client *clp)
{
smp_mb__before_atomic();
- clear_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
- smp_mb__after_atomic();
- wake_up_bit(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING);
+ clear_and_wake_up_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
rpc_wake_up(&clp->cl_rpcwaitq);
}
diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
index c0046c348910..37c4fe5a595c 100644
--- a/fs/nfs/pnfs_nfs.c
+++ b/fs/nfs/pnfs_nfs.c
@@ -567,9 +567,7 @@ static void nfs4_wait_ds_connect(struct nfs4_pnfs_ds *ds)
static void nfs4_clear_ds_conn_bit(struct nfs4_pnfs_ds *ds)
{
smp_mb__before_atomic();
- clear_bit(NFS4DS_CONNECTING, &ds->ds_state);
- smp_mb__after_atomic();
- wake_up_bit(&ds->ds_state, NFS4DS_CONNECTING);
+ clear_and_wake_up_bit(NFS4DS_CONNECTING, &ds->ds_state);
}
static struct nfs_client *(*get_v3_ds_connect)(
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 87679557d0d6..0a30f9cdf66a 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -1638,9 +1638,7 @@ static void
nfsd4_cltrack_upcall_unlock(struct nfs4_client *clp)
{
smp_mb__before_atomic();
- clear_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
- smp_mb__after_atomic();
- wake_up_bit(&clp->cl_flags, NFSD4_CLIENT_UPCALL_LOCK);
+ clear_and_wake_up_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
}
static void
diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index a35c17017210..1958bb457143 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -302,7 +302,7 @@ int orangefs_revalidate_mapping(struct inode *inode)
orangefs_inode->mapping_time = jiffies +
orangefs_cache_timeout_msecs*HZ/1000;
- clear_bit(1, bitlock);
+ clear_bit_unlock(1, bitlock);
smp_mb__after_atomic();
wake_up_bit(bitlock, 1);
diff --git a/kernel/sched/wait_bit.c b/kernel/sched/wait_bit.c
index 45eba18a2898..2662975473e3 100644
--- a/kernel/sched/wait_bit.c
+++ b/kernel/sched/wait_bit.c
@@ -189,6 +189,7 @@ EXPORT_SYMBOL(init_wait_var_entry);
void wake_up_var(void *var)
{
+ smp_mb();
__wake_up_bit(__var_waitqueue(var), var, -1);
}
EXPORT_SYMBOL(wake_up_var);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 9e4fcf406d9c..4e57f1a1b7a2 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -50,9 +50,7 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
if (status)
return;
- clear_bit(HCI_INQUIRY, &hdev->flags);
- smp_mb__after_atomic(); /* wake_up_bit advises about this barrier */
- wake_up_bit(&hdev->flags, HCI_INQUIRY);
+ clear_and_wake_up_bit(HCI_INQUIRY, &hdev->flags);
hci_dev_lock(hdev);
/* Set discovery state to stopped if we're not doing LE active
@@ -2342,7 +2340,6 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
return;
- smp_mb__after_atomic(); /* wake_up_bit advises about this barrier */
wake_up_bit(&hdev->flags, HCI_INQUIRY);
if (!hci_dev_test_flag(hdev, HCI_MGMT))
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index 3cae88cbdaa0..2c2773172964 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -363,6 +363,7 @@ static int acquire_refill(struct rds_connection *conn)
static void release_refill(struct rds_connection *conn)
{
clear_bit(RDS_RECV_REFILL, &conn->c_flags);
+ smp_mb__after_atomic();
/* 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
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 44e58a3e5663..f452816bd7d0 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -308,9 +308,8 @@ static void key_garbage_collector(struct work_struct *work)
if (unlikely(gc_state & KEY_GC_REAPING_DEAD_3)) {
kdebug("dead wake");
- smp_mb();
- clear_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
- wake_up_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE);
+ smp_mb__before_atomic();
+ clear_and_wake_up_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
}
if (gc_state & KEY_GC_REAP_AGAIN)
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Cluster-devel] [RFC][PATCH] wake_up_var() memory ordering
@ 2019-06-24 16:50 ` Peter Zijlstra
0 siblings, 0 replies; 14+ messages in thread
From: Peter Zijlstra @ 2019-06-24 16:50 UTC (permalink / raw)
To: cluster-devel.redhat.com
Hi all,
I tried using wake_up_var() today and accidentally noticed that it
didn't imply an smp_mb() and specifically requires it through
wake_up_bit() / waitqueue_active().
Now, wake_up_bit() doesn't imply the barrier because it is assumed to be
used with the atomic bitops API which either implies (test_and_clear) or
only needs smp_mb__after_atomic(), which is 'much' cheaper than an
unconditional smp_mb().
Still, while auditing all that, I found a whole bunch of things that
could be improved. There were missing barriers, superfluous barriers and
a whole bunch of sites that could use clear_and_wake_up_bit().
So this fixes all wake_up_bit() usage without actually changing
semantics of it (which are unfortunate but understandable). This does
however change the semantics of wake_up_var(); even though wake_up_var()
is most often used with atomics and then the additional smp_mb() is most
often superfluous :/
There isn't really a good option here, comments (other than I need to
split this up)?
---
drivers/bluetooth/btmtksdio.c | 5 +----
drivers/bluetooth/btmtkuart.c | 5 +----
drivers/bluetooth/hci_mrvl.c | 8 ++------
drivers/gpu/drm/i915/i915_reset.c | 6 ++----
drivers/md/dm-bufio.c | 10 ++--------
drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 15 ++++-----------
fs/afs/fs_probe.c | 1 +
fs/afs/server.c | 1 +
fs/afs/vl_probe.c | 1 +
fs/afs/volume.c | 1 +
fs/aio.c | 4 +---
fs/block_dev.c | 1 +
fs/btrfs/extent_io.c | 4 +---
fs/cachefiles/namei.c | 1 +
fs/cifs/connect.c | 3 +--
fs/cifs/misc.c | 15 +++++----------
fs/fscache/cookie.c | 2 ++
fs/fscache/object.c | 2 ++
fs/fscache/page.c | 3 +++
fs/gfs2/glock.c | 8 ++------
fs/gfs2/glops.c | 1 +
fs/gfs2/lock_dlm.c | 8 ++------
fs/gfs2/recovery.c | 4 +---
fs/gfs2/super.c | 1 +
fs/gfs2/sys.c | 4 +---
fs/nfs/nfs4state.c | 4 +---
fs/nfs/pnfs_nfs.c | 4 +---
fs/nfsd/nfs4recover.c | 4 +---
fs/orangefs/file.c | 2 +-
kernel/sched/wait_bit.c | 1 +
net/bluetooth/hci_event.c | 5 +----
net/rds/ib_recv.c | 1 +
security/keys/gc.c | 5 ++---
33 files changed, 50 insertions(+), 90 deletions(-)
diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index 813338288453..27523cfeac9a 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -356,11 +356,8 @@ static int btmtksdio_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
if (hdr->evt == HCI_EV_VENDOR) {
if (test_and_clear_bit(BTMTKSDIO_TX_WAIT_VND_EVT,
- &bdev->tx_state)) {
- /* Barrier to sync with other CPUs */
- smp_mb__after_atomic();
+ &bdev->tx_state))
wake_up_bit(&bdev->tx_state, BTMTKSDIO_TX_WAIT_VND_EVT);
- }
}
return 0;
diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
index f5dbeec8e274..7fe324df3799 100644
--- a/drivers/bluetooth/btmtkuart.c
+++ b/drivers/bluetooth/btmtkuart.c
@@ -340,11 +340,8 @@ static int btmtkuart_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
if (hdr->evt == HCI_EV_VENDOR) {
if (test_and_clear_bit(BTMTKUART_TX_WAIT_VND_EVT,
- &bdev->tx_state)) {
- /* Barrier to sync with other CPUs */
- smp_mb__after_atomic();
+ &bdev->tx_state))
wake_up_bit(&bdev->tx_state, BTMTKUART_TX_WAIT_VND_EVT);
- }
}
return 0;
diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
index 50212ac629e3..f03294d39d08 100644
--- a/drivers/bluetooth/hci_mrvl.c
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -157,9 +157,7 @@ static int mrvl_recv_fw_req(struct hci_dev *hdev, struct sk_buff *skb)
mrvl->tx_len = le16_to_cpu(pkt->lhs);
- clear_bit(STATE_FW_REQ_PENDING, &mrvl->flags);
- smp_mb__after_atomic();
- wake_up_bit(&mrvl->flags, STATE_FW_REQ_PENDING);
+ clear_and_wake_up_bit(STATE_FW_REQ_PENDING, &mrvl->flags);
done:
kfree_skb(skb);
@@ -192,9 +190,7 @@ static int mrvl_recv_chip_ver(struct hci_dev *hdev, struct sk_buff *skb)
bt_dev_info(hdev, "Controller id = %x, rev = %x", mrvl->id, mrvl->rev);
- clear_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
- smp_mb__after_atomic();
- wake_up_bit(&mrvl->flags, STATE_CHIP_VER_PENDING);
+ clear_and_wake_up_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
done:
kfree_skb(skb);
diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
index 677d59304e78..6809367dbfa9 100644
--- a/drivers/gpu/drm/i915/i915_reset.c
+++ b/drivers/gpu/drm/i915/i915_reset.c
@@ -1324,10 +1324,8 @@ void i915_handle_error(struct drm_i915_private *i915,
if (i915_reset_engine(engine, msg) == 0)
engine_mask &= ~engine->mask;
- clear_bit(I915_RESET_ENGINE + engine->id,
- &error->flags);
- wake_up_bit(&error->flags,
- I915_RESET_ENGINE + engine->id);
+ clear_and_wake_up_bit(I915_RESET_ENGINE + engine->id,
+ &error->flags);
}
}
diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 2a48ea3f1b30..3114836cc717 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -667,10 +667,7 @@ static void write_endio(struct dm_buffer *b, blk_status_t status)
BUG_ON(!test_bit(B_WRITING, &b->state));
smp_mb__before_atomic();
- clear_bit(B_WRITING, &b->state);
- smp_mb__after_atomic();
-
- wake_up_bit(&b->state, B_WRITING);
+ clear_and_wake_up_bit(B_WRITING, &b->state);
}
/*
@@ -1045,10 +1042,7 @@ static void read_endio(struct dm_buffer *b, blk_status_t status)
BUG_ON(!test_bit(B_READING, &b->state));
smp_mb__before_atomic();
- clear_bit(B_READING, &b->state);
- smp_mb__after_atomic();
-
- wake_up_bit(&b->state, B_READING);
+ clear_and_wake_up_bit(B_READING, &b->state);
}
/*
diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
index e5e056bf9dfa..fa46bc930704 100644
--- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
+++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
@@ -374,9 +374,7 @@ static int dvb_usb_stop_feed(struct dvb_demux_feed *dvbdmxfeed)
usb_urb_killv2(&adap->stream);
/* clear 'streaming' status bit */
- clear_bit(ADAP_STREAMING, &adap->state_bits);
- smp_mb__after_atomic();
- wake_up_bit(&adap->state_bits, ADAP_STREAMING);
+ clear_and_wake_up_bit(ADAP_STREAMING, &adap->state_bits);
skip_feed_stop:
if (ret)
@@ -578,11 +576,8 @@ static int dvb_usb_fe_init(struct dvb_frontend *fe)
goto err;
}
err:
- if (!adap->suspend_resume_active) {
- clear_bit(ADAP_INIT, &adap->state_bits);
- smp_mb__after_atomic();
- wake_up_bit(&adap->state_bits, ADAP_INIT);
- }
+ if (!adap->suspend_resume_active)
+ clear_and_wake_up_bit(ADAP_INIT, &adap->state_bits);
dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
return ret;
@@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
err:
if (!adap->suspend_resume_active) {
adap->active_fe = -1;
- clear_bit(ADAP_SLEEP, &adap->state_bits);
- smp_mb__after_atomic();
- wake_up_bit(&adap->state_bits, ADAP_SLEEP);
+ clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
}
dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
index cfe62b154f68..377ee07d5f76 100644
--- a/fs/afs/fs_probe.c
+++ b/fs/afs/fs_probe.c
@@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
wake_up_var(&server->probe_outstanding);
clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
+ smp_mb__after_atomic();
wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
return true;
}
diff --git a/fs/afs/server.c b/fs/afs/server.c
index e900cd74361b..c9b74a397cc1 100644
--- a/fs/afs/server.c
+++ b/fs/afs/server.c
@@ -569,6 +569,7 @@ bool afs_check_server_record(struct afs_fs_cursor *fc, struct afs_server *server
if (!test_and_set_bit_lock(AFS_SERVER_FL_UPDATING, &server->flags)) {
success = afs_update_server_record(fc, server);
clear_bit_unlock(AFS_SERVER_FL_UPDATING, &server->flags);
+ smp_mb__after_atomic();
wake_up_bit(&server->flags, AFS_SERVER_FL_UPDATING);
_leave(" = %d", success);
return success;
diff --git a/fs/afs/vl_probe.c b/fs/afs/vl_probe.c
index 858498cc1b05..1939624aa147 100644
--- a/fs/afs/vl_probe.c
+++ b/fs/afs/vl_probe.c
@@ -18,6 +18,7 @@ static bool afs_vl_probe_done(struct afs_vlserver *server)
wake_up_var(&server->probe_outstanding);
clear_bit_unlock(AFS_VLSERVER_FL_PROBING, &server->flags);
+ smp_mb__after_atomic();
wake_up_bit(&server->flags, AFS_VLSERVER_FL_PROBING);
return true;
}
diff --git a/fs/afs/volume.c b/fs/afs/volume.c
index 08fdb3951c49..b968e4e96f6b 100644
--- a/fs/afs/volume.c
+++ b/fs/afs/volume.c
@@ -303,6 +303,7 @@ int afs_check_volume_status(struct afs_volume *volume, struct key *key)
ret = afs_update_volume_status(volume, key);
clear_bit_unlock(AFS_VOLUME_WAIT, &volume->flags);
clear_bit_unlock(AFS_VOLUME_UPDATING, &volume->flags);
+ smp_mb__after_atomic();
wake_up_bit(&volume->flags, AFS_VOLUME_WAIT);
_leave(" = %d", ret);
return ret;
diff --git a/fs/aio.c b/fs/aio.c
index 3490d1fa0e16..a21acb7ee2a5 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1154,9 +1154,7 @@ static void aio_complete(struct aio_kiocb *iocb)
* like in wake_up_bit() where clearing a bit has to be
* ordered with the unlocked test.
*/
- smp_mb();
-
- if (waitqueue_active(&ctx->wait))
+ if (wq_has_sleeper(&ctx->wait))
wake_up(&ctx->wait);
}
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 749f5984425d..80e35cbfc7cc 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1687,6 +1687,7 @@ int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
/* tell others that we're done */
BUG_ON(whole->bd_claiming != holder);
whole->bd_claiming = NULL;
+ smp_mb();
wake_up_bit(&whole->bd_claiming, 0);
spin_unlock(&bdev_lock);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index db337e53aab3..ee1136ae46fd 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3683,9 +3683,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
static void end_extent_buffer_writeback(struct extent_buffer *eb)
{
- clear_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
- smp_mb__after_atomic();
- wake_up_bit(&eb->bflags, EXTENT_BUFFER_WRITEBACK);
+ clear_and_wake_up_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
}
static void set_btree_ioerr(struct page *page)
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index ecc8ecbbfa5a..ecebba72ec7d 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -267,6 +267,7 @@ void cachefiles_mark_object_inactive(struct cachefiles_cache *cache,
clear_bit(CACHEFILES_OBJECT_ACTIVE, &object->flags);
write_unlock(&cache->active_lock);
+ smp_mb__after_atomic();
wake_up_bit(&object->flags, CACHEFILES_OBJECT_ACTIVE);
/* This object can now be culled, so we need to let the daemon know
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 8c4121da624e..8c1115ed1c28 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -5235,8 +5235,7 @@ cifs_sb_tlink(struct cifs_sb_info *cifs_sb)
}
tlink->tl_tcon = cifs_construct_tcon(cifs_sb, fsuid);
- clear_bit(TCON_LINK_PENDING, &tlink->tl_flags);
- wake_up_bit(&tlink->tl_flags, TCON_LINK_PENDING);
+ clear_and_wake_up_bit(TCON_LINK_PENDING, &tlink->tl_flags);
if (IS_ERR(tlink->tl_tcon)) {
cifs_put_tlink(tlink);
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index b1a696a73f7c..961751d89113 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -584,10 +584,8 @@ int cifs_get_writer(struct cifsInodeInfo *cinode)
/* Check to see if we have started servicing an oplock break */
if (test_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags)) {
cinode->writers--;
- if (cinode->writers == 0) {
- clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
- wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
- }
+ if (cinode->writers == 0)
+ clear_and_wake_up_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
spin_unlock(&cinode->writers_lock);
goto start;
}
@@ -599,10 +597,8 @@ void cifs_put_writer(struct cifsInodeInfo *cinode)
{
spin_lock(&cinode->writers_lock);
cinode->writers--;
- if (cinode->writers == 0) {
- clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
- wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
- }
+ if (cinode->writers == 0)
+ clear_and_wake_up_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
spin_unlock(&cinode->writers_lock);
}
@@ -630,8 +626,7 @@ void cifs_queue_oplock_break(struct cifsFileInfo *cfile)
void cifs_done_oplock_break(struct cifsInodeInfo *cinode)
{
- clear_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags);
- wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_OPLOCK_BREAK);
+ clear_and_wake_up_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags);
}
bool
diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
index 0ce39658a620..40ba8957ebf0 100644
--- a/fs/fscache/cookie.c
+++ b/fs/fscache/cookie.c
@@ -381,6 +381,7 @@ void __fscache_enable_cookie(struct fscache_cookie *cookie,
out_unlock:
clear_bit_unlock(FSCACHE_COOKIE_ENABLEMENT_LOCK, &cookie->flags);
+ smp_mb__after_atomic();
wake_up_bit(&cookie->flags, FSCACHE_COOKIE_ENABLEMENT_LOCK);
}
EXPORT_SYMBOL(__fscache_enable_cookie);
@@ -778,6 +779,7 @@ void __fscache_disable_cookie(struct fscache_cookie *cookie,
out_unlock_enable:
clear_bit_unlock(FSCACHE_COOKIE_ENABLEMENT_LOCK, &cookie->flags);
+ smp_mb__after_atomic();
wake_up_bit(&cookie->flags, FSCACHE_COOKIE_ENABLEMENT_LOCK);
_leave("");
}
diff --git a/fs/fscache/object.c b/fs/fscache/object.c
index cfeba839a0f2..08b2edec6596 100644
--- a/fs/fscache/object.c
+++ b/fs/fscache/object.c
@@ -524,6 +524,7 @@ void fscache_object_lookup_negative(struct fscache_object *object)
_debug("wake up lookup %p", &cookie->flags);
clear_bit_unlock(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags);
+ smp_mb__after_atomic();
wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
}
_leave("");
@@ -559,6 +560,7 @@ void fscache_obtained_object(struct fscache_object *object)
* to begin shovelling data.
*/
clear_bit_unlock(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags);
+ smp_mb__after_atomic();
wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
} else {
fscache_stat(&fscache_n_object_created);
diff --git a/fs/fscache/page.c b/fs/fscache/page.c
index 26af6fdf1538..49ef238502a3 100644
--- a/fs/fscache/page.c
+++ b/fs/fscache/page.c
@@ -113,6 +113,7 @@ bool __fscache_maybe_release_page(struct fscache_cookie *cookie,
fscache_stat(&fscache_n_store_vmscan_gone);
}
+ smp_mb();
wake_up_bit(&cookie->flags, 0);
trace_fscache_wake_cookie(cookie);
if (xpage)
@@ -171,6 +172,7 @@ static void fscache_end_page_write(struct fscache_object *object,
trace_fscache_page(cookie, page, fscache_page_write_end_pend);
}
spin_unlock(&cookie->stores_lock);
+ smp_mb();
wake_up_bit(&cookie->flags, 0);
trace_fscache_wake_cookie(cookie);
} else {
@@ -922,6 +924,7 @@ void fscache_invalidate_writes(struct fscache_cookie *cookie)
put_page(results[i]);
}
+ smp_mb();
wake_up_bit(&cookie->flags, 0);
trace_fscache_wake_cookie(cookie);
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index f1ebcb42cbf5..8024b6bdb6d4 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -302,9 +302,7 @@ static inline int may_grant(const struct gfs2_glock *gl, const struct gfs2_holde
static void gfs2_holder_wake(struct gfs2_holder *gh)
{
- clear_bit(HIF_WAIT, &gh->gh_iflags);
- smp_mb__after_atomic();
- wake_up_bit(&gh->gh_iflags, HIF_WAIT);
+ clear_and_wake_up_bit(HIF_WAIT, &gh->gh_iflags);
}
/**
@@ -436,9 +434,7 @@ static void state_change(struct gfs2_glock *gl, unsigned int new_state)
static void gfs2_demote_wake(struct gfs2_glock *gl)
{
gl->gl_demote_state = LM_ST_EXCLUSIVE;
- clear_bit(GLF_DEMOTE, &gl->gl_flags);
- smp_mb__after_atomic();
- wake_up_bit(&gl->gl_flags, GLF_DEMOTE);
+ clear_and_wake_up_bit(GLF_DEMOTE, &gl->gl_flags);
}
/**
diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index cf4c767005b1..666629ea5da7 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
return;
clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
+ smp_mb__after_atomic();
wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);
}
diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
index 4361804646d8..0fa1865dd600 100644
--- a/fs/gfs2/lock_dlm.c
+++ b/fs/gfs2/lock_dlm.c
@@ -1139,9 +1139,7 @@ static void gdlm_recover_done(void *arg, struct dlm_slot *slots, int num_slots,
if (!test_bit(DFL_UNMOUNT, &ls->ls_recover_flags))
queue_delayed_work(gfs2_control_wq, &sdp->sd_control_work, 0);
- clear_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
- smp_mb__after_atomic();
- wake_up_bit(&ls->ls_recover_flags, DFL_DLM_RECOVERY);
+ clear_and_wake_up_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
spin_unlock(&ls->ls_recover_spin);
}
@@ -1276,9 +1274,7 @@ static int gdlm_mount(struct gfs2_sbd *sdp, const char *table)
}
ls->ls_first = !!test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags);
- clear_bit(SDF_NOJOURNALID, &sdp->sd_flags);
- smp_mb__after_atomic();
- wake_up_bit(&sdp->sd_flags, SDF_NOJOURNALID);
+ clear_and_wake_up_bit(SDF_NOJOURNALID, &sdp->sd_flags);
return 0;
fail_release:
diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
index 2299a3fa1911..ef9a8ef6689b 100644
--- a/fs/gfs2/recovery.c
+++ b/fs/gfs2/recovery.c
@@ -436,9 +436,7 @@ void gfs2_recover_func(struct work_struct *work)
jd->jd_recover_error = error;
gfs2_recovery_done(sdp, jd->jd_jid, LM_RD_GAVEUP);
done:
- clear_bit(JDF_RECOVERY, &jd->jd_flags);
- smp_mb__after_atomic();
- wake_up_bit(&jd->jd_flags, JDF_RECOVERY);
+ clear_and_wake_up_bit(JDF_RECOVERY, &jd->jd_flags);
}
int gfs2_recover_journal(struct gfs2_jdesc *jd, bool wait)
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index b70cea5c8c59..28ff2e10bdd9 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -985,6 +985,7 @@ void gfs2_freeze_func(struct work_struct *work)
}
deactivate_super(sb);
clear_bit_unlock(SDF_FS_FROZEN, &sdp->sd_flags);
+ smp_mb__after_atomic();
wake_up_bit(&sdp->sd_flags, SDF_FS_FROZEN);
return;
}
diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index 159aedf63c2a..b337798d8957 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -505,9 +505,7 @@ static ssize_t jid_store(struct gfs2_sbd *sdp, const char *buf, size_t len)
if (sdp->sd_args.ar_spectator && jid > 0)
rv = jid = -EINVAL;
sdp->sd_lockstruct.ls_jid = jid;
- clear_bit(SDF_NOJOURNALID, &sdp->sd_flags);
- smp_mb__after_atomic();
- wake_up_bit(&sdp->sd_flags, SDF_NOJOURNALID);
+ clear_and_wake_up_bit(SDF_NOJOURNALID, &sdp->sd_flags);
out:
spin_unlock(&sdp->sd_jindex_spin);
return rv ? rv : len;
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index e2e3c4f04d3e..a3ed413a38c5 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1196,9 +1196,7 @@ static int nfs4_run_state_manager(void *);
static void nfs4_clear_state_manager_bit(struct nfs_client *clp)
{
smp_mb__before_atomic();
- clear_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
- smp_mb__after_atomic();
- wake_up_bit(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING);
+ clear_and_wake_up_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
rpc_wake_up(&clp->cl_rpcwaitq);
}
diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
index c0046c348910..37c4fe5a595c 100644
--- a/fs/nfs/pnfs_nfs.c
+++ b/fs/nfs/pnfs_nfs.c
@@ -567,9 +567,7 @@ static void nfs4_wait_ds_connect(struct nfs4_pnfs_ds *ds)
static void nfs4_clear_ds_conn_bit(struct nfs4_pnfs_ds *ds)
{
smp_mb__before_atomic();
- clear_bit(NFS4DS_CONNECTING, &ds->ds_state);
- smp_mb__after_atomic();
- wake_up_bit(&ds->ds_state, NFS4DS_CONNECTING);
+ clear_and_wake_up_bit(NFS4DS_CONNECTING, &ds->ds_state);
}
static struct nfs_client *(*get_v3_ds_connect)(
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 87679557d0d6..0a30f9cdf66a 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -1638,9 +1638,7 @@ static void
nfsd4_cltrack_upcall_unlock(struct nfs4_client *clp)
{
smp_mb__before_atomic();
- clear_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
- smp_mb__after_atomic();
- wake_up_bit(&clp->cl_flags, NFSD4_CLIENT_UPCALL_LOCK);
+ clear_and_wake_up_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
}
static void
diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index a35c17017210..1958bb457143 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -302,7 +302,7 @@ int orangefs_revalidate_mapping(struct inode *inode)
orangefs_inode->mapping_time = jiffies +
orangefs_cache_timeout_msecs*HZ/1000;
- clear_bit(1, bitlock);
+ clear_bit_unlock(1, bitlock);
smp_mb__after_atomic();
wake_up_bit(bitlock, 1);
diff --git a/kernel/sched/wait_bit.c b/kernel/sched/wait_bit.c
index 45eba18a2898..2662975473e3 100644
--- a/kernel/sched/wait_bit.c
+++ b/kernel/sched/wait_bit.c
@@ -189,6 +189,7 @@ EXPORT_SYMBOL(init_wait_var_entry);
void wake_up_var(void *var)
{
+ smp_mb();
__wake_up_bit(__var_waitqueue(var), var, -1);
}
EXPORT_SYMBOL(wake_up_var);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 9e4fcf406d9c..4e57f1a1b7a2 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -50,9 +50,7 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
if (status)
return;
- clear_bit(HCI_INQUIRY, &hdev->flags);
- smp_mb__after_atomic(); /* wake_up_bit advises about this barrier */
- wake_up_bit(&hdev->flags, HCI_INQUIRY);
+ clear_and_wake_up_bit(HCI_INQUIRY, &hdev->flags);
hci_dev_lock(hdev);
/* Set discovery state to stopped if we're not doing LE active
@@ -2342,7 +2340,6 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
return;
- smp_mb__after_atomic(); /* wake_up_bit advises about this barrier */
wake_up_bit(&hdev->flags, HCI_INQUIRY);
if (!hci_dev_test_flag(hdev, HCI_MGMT))
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index 3cae88cbdaa0..2c2773172964 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -363,6 +363,7 @@ static int acquire_refill(struct rds_connection *conn)
static void release_refill(struct rds_connection *conn)
{
clear_bit(RDS_RECV_REFILL, &conn->c_flags);
+ smp_mb__after_atomic();
/* 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
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 44e58a3e5663..f452816bd7d0 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -308,9 +308,8 @@ static void key_garbage_collector(struct work_struct *work)
if (unlikely(gc_state & KEY_GC_REAPING_DEAD_3)) {
kdebug("dead wake");
- smp_mb();
- clear_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
- wake_up_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE);
+ smp_mb__before_atomic();
+ clear_and_wake_up_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
}
if (gc_state & KEY_GC_REAP_AGAIN)
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC][PATCH] wake_up_var() memory ordering
2019-06-24 16:50 ` [Cluster-devel] " Peter Zijlstra
@ 2019-06-25 7:51 ` David Howells
-1 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2019-06-25 7:51 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Martin Brandenburg, linux-cachefs, Mike Snitzer, linux-aio,
David Airlie, samba-technical, Will Deacon, dri-devel, dhowells,
Chris Mason, dm-devel, keyrings, Ingo Molnar, linux-afs,
Alasdair Kergon, Mike Marshall, linux-cifs, rds-devel,
Andreas Gruenbacher, linux-rdma, James Morris, cluster-devel,
Matthias Brugger, Paul McKenney, intel-gfx, devel,
Serge E. Hallyn, Santosh Shilimkar
Peter Zijlstra <peterz@infradead.org> wrote:
> I tried using wake_up_var() today and accidentally noticed that it
> didn't imply an smp_mb() and specifically requires it through
> wake_up_bit() / waitqueue_active().
Thinking about it again, I'm not sure why you need to add the barrier when
wake_up() (which this is a wrapper around) is required to impose a barrier at
the front if there's anything to wake up (ie. the wait queue isn't empty).
If this is insufficient, does it make sense just to have wake_up*() functions
do an unconditional release or full barrier right at the front, rather than it
being conditional on something being woken up?
> @@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
> err:
> if (!adap->suspend_resume_active) {
> adap->active_fe = -1;
I'm wondering if there's a missing barrier here. Should the clear_bit() on
the next line be clear_bit_unlock() or clear_bit_release()?
> - clear_bit(ADAP_SLEEP, &adap->state_bits);
> - smp_mb__after_atomic();
> - wake_up_bit(&adap->state_bits, ADAP_SLEEP);
> + clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
> }
>
> dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
> index cfe62b154f68..377ee07d5f76 100644
> --- a/fs/afs/fs_probe.c
> +++ b/fs/afs/fs_probe.c
> @@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
>
> wake_up_var(&server->probe_outstanding);
> clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
> return true;
> }
Looking at this and the dvb one, does it make sense to stick the release
semantics of clear_bit_unlock() into clear_and_wake_up_bit()?
Also, should clear_bit_unlock() be renamed to clear_bit_release() (and
similarly test_and_set_bit_lock() -> test_and_set_bit_acquire()) if we seem to
be trying to standardise on that terminology.
David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [RFC][PATCH] wake_up_var() memory ordering
@ 2019-06-25 7:51 ` David Howells
0 siblings, 0 replies; 14+ messages in thread
From: David Howells @ 2019-06-25 7:51 UTC (permalink / raw)
To: cluster-devel.redhat.com
Peter Zijlstra <peterz@infradead.org> wrote:
> I tried using wake_up_var() today and accidentally noticed that it
> didn't imply an smp_mb() and specifically requires it through
> wake_up_bit() / waitqueue_active().
Thinking about it again, I'm not sure why you need to add the barrier when
wake_up() (which this is a wrapper around) is required to impose a barrier at
the front if there's anything to wake up (ie. the wait queue isn't empty).
If this is insufficient, does it make sense just to have wake_up*() functions
do an unconditional release or full barrier right at the front, rather than it
being conditional on something being woken up?
> @@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
> err:
> if (!adap->suspend_resume_active) {
> adap->active_fe = -1;
I'm wondering if there's a missing barrier here. Should the clear_bit() on
the next line be clear_bit_unlock() or clear_bit_release()?
> - clear_bit(ADAP_SLEEP, &adap->state_bits);
> - smp_mb__after_atomic();
> - wake_up_bit(&adap->state_bits, ADAP_SLEEP);
> + clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
> }
>
> dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
> index cfe62b154f68..377ee07d5f76 100644
> --- a/fs/afs/fs_probe.c
> +++ b/fs/afs/fs_probe.c
> @@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
>
> wake_up_var(&server->probe_outstanding);
> clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
> return true;
> }
Looking at this and the dvb one, does it make sense to stick the release
semantics of clear_bit_unlock() into clear_and_wake_up_bit()?
Also, should clear_bit_unlock() be renamed to clear_bit_release() (and
similarly test_and_set_bit_lock() -> test_and_set_bit_acquire()) if we seem to
be trying to standardise on that terminology.
David
^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <32379.1561449061-S6HVgzuS8uM4Awkfq6JHfwNdhmdF6hFW@public.gmane.org>]
* Re: [RFC][PATCH] wake_up_var() memory ordering
2019-06-25 7:51 ` [Cluster-devel] " David Howells
@ 2019-06-25 8:11 ` Peter Zijlstra
-1 siblings, 0 replies; 14+ messages in thread
From: Peter Zijlstra @ 2019-06-25 8:11 UTC (permalink / raw)
To: David Howells
Cc: Martin Brandenburg, Mike Snitzer,
linux-aio-Bw31MaZKKs3YtjvyW6yDsg, David Airlie,
samba-technical-w/Ol4Ecudpl8XjKLYN78aQ, Joonas Lahtinen,
Will Deacon, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
J. Bruce Fields, Chris Mason, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
keyrings-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
linux-afs-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Alasdair Kergon,
Mike Marshall, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
rds-devel-N0ozoZBvEnrZJqsBc5GL+g, Andreas Gruenbacher,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, James Morris,
cluster-devel-H+wXaHxf7aLQT0dZR+AlfA, Antti Palosaari,
Matthias Brugger, Paul McKenney,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, devel
(sorry for cross-posting to moderated lists btw, I've since
acquired a patch to get_maintainers.pl that wil exclude them
in the future)
On Tue, Jun 25, 2019 at 08:51:01AM +0100, David Howells wrote:
> Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
>
> > I tried using wake_up_var() today and accidentally noticed that it
> > didn't imply an smp_mb() and specifically requires it through
> > wake_up_bit() / waitqueue_active().
>
> Thinking about it again, I'm not sure why you need to add the barrier when
> wake_up() (which this is a wrapper around) is required to impose a barrier at
> the front if there's anything to wake up (ie. the wait queue isn't empty).
>
> If this is insufficient, does it make sense just to have wake_up*() functions
> do an unconditional release or full barrier right at the front, rather than it
> being conditional on something being woken up?
The curprit is __wake_up_bit()'s usage of waitqueue_active(); it is this
latter (see its comment) that requires the smp_mb().
wake_up_bit() and wake_up_var() are wrappers around __wake_up_bit().
Without this barrier it is possible for the waitqueue_active() load to
be hoisted over the cond=true store and the remote end can miss the
store and we can miss its enqueue and we'll all miss a wakeup and get
stuck.
Adding an smp_mb() (or use wq_has_sleeper()) in __wake_up_bit() would be
nice, but I fear some people will complain about overhead, esp. since
about half the sites don't need the barrier due to being behind
test_and_clear_bit() and the other half using smp_mb__after_atomic()
after some clear_bit*() variant.
There's a few sites that seem to open-code
wait_var_event()/wake_up_var() and those actually need the full
smp_mb(), but then maybe they should be converted to var instread of bit
anyway.
> > @@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
> > err:
> > if (!adap->suspend_resume_active) {
> > adap->active_fe = -1;
>
> I'm wondering if there's a missing barrier here. Should the clear_bit() on
> the next line be clear_bit_unlock() or clear_bit_release()?
That looks reasonable, but I'd like to hear from the DVB folks on that.
> > - clear_bit(ADAP_SLEEP, &adap->state_bits);
> > - smp_mb__after_atomic();
> > - wake_up_bit(&adap->state_bits, ADAP_SLEEP);
> > + clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
> > }
> >
> > dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> > diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
> > index cfe62b154f68..377ee07d5f76 100644
> > --- a/fs/afs/fs_probe.c
> > +++ b/fs/afs/fs_probe.c
> > @@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
> >
> > wake_up_var(&server->probe_outstanding);
> > clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
> > + smp_mb__after_atomic();
> > wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
> > return true;
> > }
>
> Looking at this and the dvb one, does it make sense to stick the release
> semantics of clear_bit_unlock() into clear_and_wake_up_bit()?
I was thinking of adding another helper, maybe unlock_and_wake_up_bit()
that included that extra barrier, but maybe making it unconditional
isn't the worst idea.
> Also, should clear_bit_unlock() be renamed to clear_bit_release() (and
> similarly test_and_set_bit_lock() -> test_and_set_bit_acquire()) if we seem to
> be trying to standardise on that terminology.
That definitely makes sense to me, there's only 157 clear_bit_unlock()
and 76 test_and_set_bit_lock() users (note the asymetry of that).
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [RFC][PATCH] wake_up_var() memory ordering
@ 2019-06-25 8:11 ` Peter Zijlstra
0 siblings, 0 replies; 14+ messages in thread
From: Peter Zijlstra @ 2019-06-25 8:11 UTC (permalink / raw)
To: cluster-devel.redhat.com
(sorry for cross-posting to moderated lists btw, I've since
acquired a patch to get_maintainers.pl that wil exclude them
in the future)
On Tue, Jun 25, 2019 at 08:51:01AM +0100, David Howells wrote:
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > I tried using wake_up_var() today and accidentally noticed that it
> > didn't imply an smp_mb() and specifically requires it through
> > wake_up_bit() / waitqueue_active().
>
> Thinking about it again, I'm not sure why you need to add the barrier when
> wake_up() (which this is a wrapper around) is required to impose a barrier at
> the front if there's anything to wake up (ie. the wait queue isn't empty).
>
> If this is insufficient, does it make sense just to have wake_up*() functions
> do an unconditional release or full barrier right at the front, rather than it
> being conditional on something being woken up?
The curprit is __wake_up_bit()'s usage of waitqueue_active(); it is this
latter (see its comment) that requires the smp_mb().
wake_up_bit() and wake_up_var() are wrappers around __wake_up_bit().
Without this barrier it is possible for the waitqueue_active() load to
be hoisted over the cond=true store and the remote end can miss the
store and we can miss its enqueue and we'll all miss a wakeup and get
stuck.
Adding an smp_mb() (or use wq_has_sleeper()) in __wake_up_bit() would be
nice, but I fear some people will complain about overhead, esp. since
about half the sites don't need the barrier due to being behind
test_and_clear_bit() and the other half using smp_mb__after_atomic()
after some clear_bit*() variant.
There's a few sites that seem to open-code
wait_var_event()/wake_up_var() and those actually need the full
smp_mb(), but then maybe they should be converted to var instread of bit
anyway.
> > @@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
> > err:
> > if (!adap->suspend_resume_active) {
> > adap->active_fe = -1;
>
> I'm wondering if there's a missing barrier here. Should the clear_bit() on
> the next line be clear_bit_unlock() or clear_bit_release()?
That looks reasonable, but I'd like to hear from the DVB folks on that.
> > - clear_bit(ADAP_SLEEP, &adap->state_bits);
> > - smp_mb__after_atomic();
> > - wake_up_bit(&adap->state_bits, ADAP_SLEEP);
> > + clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
> > }
> >
> > dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> > diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
> > index cfe62b154f68..377ee07d5f76 100644
> > --- a/fs/afs/fs_probe.c
> > +++ b/fs/afs/fs_probe.c
> > @@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
> >
> > wake_up_var(&server->probe_outstanding);
> > clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
> > + smp_mb__after_atomic();
> > wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
> > return true;
> > }
>
> Looking at this and the dvb one, does it make sense to stick the release
> semantics of clear_bit_unlock() into clear_and_wake_up_bit()?
I was thinking of adding another helper, maybe unlock_and_wake_up_bit()
that included that extra barrier, but maybe making it unconditional
isn't the worst idea.
> Also, should clear_bit_unlock() be renamed to clear_bit_release() (and
> similarly test_and_set_bit_lock() -> test_and_set_bit_acquire()) if we seem to
> be trying to standardise on that terminology.
That definitely makes sense to me, there's only 157 clear_bit_unlock()
and 76 test_and_set_bit_lock() users (note the asymetry of that).
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC][PATCH] wake_up_var() memory ordering
2019-06-24 16:50 ` [Cluster-devel] " Peter Zijlstra
@ 2019-06-25 9:19 ` Andreas Gruenbacher
-1 siblings, 0 replies; 14+ messages in thread
From: Andreas Gruenbacher @ 2019-06-25 9:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: David Howells, Paul McKenney, Will Deacon, Marcel Holtmann,
Johan Hedberg, Matthias Brugger, Sean Wang, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, David Airlie, Daniel Vetter,
Alasdair Kergon, Mike Snitzer, dm-devel, Antti Palosaari,
MauroCarvalho Chehab, Alexander Viro, Benjamin LaHaise,
Chris Mason, Josef Bacik, David Sterba, Steve French,
Bob Peterson
Peter,
thanks for the fixes.
On Mon, 24 Jun 2019 at 18:53, Peter Zijlstra <peterz@infradead.org> wrote:
> Hi all,
>
> I tried using wake_up_var() today and accidentally noticed that it
> didn't imply an smp_mb() and specifically requires it through
> wake_up_bit() / waitqueue_active().
>
> Now, wake_up_bit() doesn't imply the barrier because it is assumed to be
> used with the atomic bitops API which either implies (test_and_clear) or
> only needs smp_mb__after_atomic(), which is 'much' cheaper than an
> unconditional smp_mb().
>
> Still, while auditing all that, I found a whole bunch of things that
> could be improved. There were missing barriers, superfluous barriers and
> a whole bunch of sites that could use clear_and_wake_up_bit().
>
> So this fixes all wake_up_bit() usage without actually changing
> semantics of it (which are unfortunate but understandable).
thanks for the fixes.
> This does
> however change the semantics of wake_up_var(); even though wake_up_var()
> is most often used with atomics and then the additional smp_mb() is most
> often superfluous :/
>
> There isn't really a good option here, comments (other than I need to
> split this up)?
>
>
> ---
> drivers/bluetooth/btmtksdio.c | 5 +----
> drivers/bluetooth/btmtkuart.c | 5 +----
> drivers/bluetooth/hci_mrvl.c | 8 ++------
> drivers/gpu/drm/i915/i915_reset.c | 6 ++----
> drivers/md/dm-bufio.c | 10 ++--------
> drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 15 ++++-----------
> fs/afs/fs_probe.c | 1 +
> fs/afs/server.c | 1 +
> fs/afs/vl_probe.c | 1 +
> fs/afs/volume.c | 1 +
> fs/aio.c | 4 +---
> fs/block_dev.c | 1 +
> fs/btrfs/extent_io.c | 4 +---
> fs/cachefiles/namei.c | 1 +
> fs/cifs/connect.c | 3 +--
> fs/cifs/misc.c | 15 +++++----------
> fs/fscache/cookie.c | 2 ++
> fs/fscache/object.c | 2 ++
> fs/fscache/page.c | 3 +++
> fs/gfs2/glock.c | 8 ++------
> fs/gfs2/glops.c | 1 +
> fs/gfs2/lock_dlm.c | 8 ++------
> fs/gfs2/recovery.c | 4 +---
> fs/gfs2/super.c | 1 +
> fs/gfs2/sys.c | 4 +---
> fs/nfs/nfs4state.c | 4 +---
> fs/nfs/pnfs_nfs.c | 4 +---
> fs/nfsd/nfs4recover.c | 4 +---
> fs/orangefs/file.c | 2 +-
> kernel/sched/wait_bit.c | 1 +
> net/bluetooth/hci_event.c | 5 +----
> net/rds/ib_recv.c | 1 +
> security/keys/gc.c | 5 ++---
> 33 files changed, 50 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> index 813338288453..27523cfeac9a 100644
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -356,11 +356,8 @@ static int btmtksdio_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
>
> if (hdr->evt == HCI_EV_VENDOR) {
> if (test_and_clear_bit(BTMTKSDIO_TX_WAIT_VND_EVT,
> - &bdev->tx_state)) {
> - /* Barrier to sync with other CPUs */
> - smp_mb__after_atomic();
> + &bdev->tx_state))
> wake_up_bit(&bdev->tx_state, BTMTKSDIO_TX_WAIT_VND_EVT);
> - }
> }
>
> return 0;
> diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
> index f5dbeec8e274..7fe324df3799 100644
> --- a/drivers/bluetooth/btmtkuart.c
> +++ b/drivers/bluetooth/btmtkuart.c
> @@ -340,11 +340,8 @@ static int btmtkuart_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
>
> if (hdr->evt == HCI_EV_VENDOR) {
> if (test_and_clear_bit(BTMTKUART_TX_WAIT_VND_EVT,
> - &bdev->tx_state)) {
> - /* Barrier to sync with other CPUs */
> - smp_mb__after_atomic();
> + &bdev->tx_state))
> wake_up_bit(&bdev->tx_state, BTMTKUART_TX_WAIT_VND_EVT);
> - }
> }
>
> return 0;
> diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
> index 50212ac629e3..f03294d39d08 100644
> --- a/drivers/bluetooth/hci_mrvl.c
> +++ b/drivers/bluetooth/hci_mrvl.c
> @@ -157,9 +157,7 @@ static int mrvl_recv_fw_req(struct hci_dev *hdev, struct sk_buff *skb)
>
> mrvl->tx_len = le16_to_cpu(pkt->lhs);
>
> - clear_bit(STATE_FW_REQ_PENDING, &mrvl->flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&mrvl->flags, STATE_FW_REQ_PENDING);
> + clear_and_wake_up_bit(STATE_FW_REQ_PENDING, &mrvl->flags);
>
> done:
> kfree_skb(skb);
> @@ -192,9 +190,7 @@ static int mrvl_recv_chip_ver(struct hci_dev *hdev, struct sk_buff *skb)
>
> bt_dev_info(hdev, "Controller id = %x, rev = %x", mrvl->id, mrvl->rev);
>
> - clear_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&mrvl->flags, STATE_CHIP_VER_PENDING);
> + clear_and_wake_up_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
>
> done:
> kfree_skb(skb);
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 677d59304e78..6809367dbfa9 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -1324,10 +1324,8 @@ void i915_handle_error(struct drm_i915_private *i915,
> if (i915_reset_engine(engine, msg) == 0)
> engine_mask &= ~engine->mask;
>
> - clear_bit(I915_RESET_ENGINE + engine->id,
> - &error->flags);
> - wake_up_bit(&error->flags,
> - I915_RESET_ENGINE + engine->id);
> + clear_and_wake_up_bit(I915_RESET_ENGINE + engine->id,
> + &error->flags);
> }
> }
>
> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> index 2a48ea3f1b30..3114836cc717 100644
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -667,10 +667,7 @@ static void write_endio(struct dm_buffer *b, blk_status_t status)
> BUG_ON(!test_bit(B_WRITING, &b->state));
>
> smp_mb__before_atomic();
> - clear_bit(B_WRITING, &b->state);
> - smp_mb__after_atomic();
> -
> - wake_up_bit(&b->state, B_WRITING);
> + clear_and_wake_up_bit(B_WRITING, &b->state);
> }
>
> /*
> @@ -1045,10 +1042,7 @@ static void read_endio(struct dm_buffer *b, blk_status_t status)
> BUG_ON(!test_bit(B_READING, &b->state));
>
> smp_mb__before_atomic();
> - clear_bit(B_READING, &b->state);
> - smp_mb__after_atomic();
> -
> - wake_up_bit(&b->state, B_READING);
> + clear_and_wake_up_bit(B_READING, &b->state);
> }
>
> /*
> diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> index e5e056bf9dfa..fa46bc930704 100644
> --- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> +++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> @@ -374,9 +374,7 @@ static int dvb_usb_stop_feed(struct dvb_demux_feed *dvbdmxfeed)
> usb_urb_killv2(&adap->stream);
>
> /* clear 'streaming' status bit */
> - clear_bit(ADAP_STREAMING, &adap->state_bits);
> - smp_mb__after_atomic();
> - wake_up_bit(&adap->state_bits, ADAP_STREAMING);
> + clear_and_wake_up_bit(ADAP_STREAMING, &adap->state_bits);
> skip_feed_stop:
>
> if (ret)
> @@ -578,11 +576,8 @@ static int dvb_usb_fe_init(struct dvb_frontend *fe)
> goto err;
> }
> err:
> - if (!adap->suspend_resume_active) {
> - clear_bit(ADAP_INIT, &adap->state_bits);
> - smp_mb__after_atomic();
> - wake_up_bit(&adap->state_bits, ADAP_INIT);
> - }
> + if (!adap->suspend_resume_active)
> + clear_and_wake_up_bit(ADAP_INIT, &adap->state_bits);
>
> dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> return ret;
> @@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
> err:
> if (!adap->suspend_resume_active) {
> adap->active_fe = -1;
> - clear_bit(ADAP_SLEEP, &adap->state_bits);
> - smp_mb__after_atomic();
> - wake_up_bit(&adap->state_bits, ADAP_SLEEP);
> + clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
> }
>
> dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
> index cfe62b154f68..377ee07d5f76 100644
> --- a/fs/afs/fs_probe.c
> +++ b/fs/afs/fs_probe.c
> @@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
>
> wake_up_var(&server->probe_outstanding);
> clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
> return true;
> }
> diff --git a/fs/afs/server.c b/fs/afs/server.c
> index e900cd74361b..c9b74a397cc1 100644
> --- a/fs/afs/server.c
> +++ b/fs/afs/server.c
> @@ -569,6 +569,7 @@ bool afs_check_server_record(struct afs_fs_cursor *fc, struct afs_server *server
> if (!test_and_set_bit_lock(AFS_SERVER_FL_UPDATING, &server->flags)) {
> success = afs_update_server_record(fc, server);
> clear_bit_unlock(AFS_SERVER_FL_UPDATING, &server->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&server->flags, AFS_SERVER_FL_UPDATING);
> _leave(" = %d", success);
> return success;
> diff --git a/fs/afs/vl_probe.c b/fs/afs/vl_probe.c
> index 858498cc1b05..1939624aa147 100644
> --- a/fs/afs/vl_probe.c
> +++ b/fs/afs/vl_probe.c
> @@ -18,6 +18,7 @@ static bool afs_vl_probe_done(struct afs_vlserver *server)
>
> wake_up_var(&server->probe_outstanding);
> clear_bit_unlock(AFS_VLSERVER_FL_PROBING, &server->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&server->flags, AFS_VLSERVER_FL_PROBING);
> return true;
> }
> diff --git a/fs/afs/volume.c b/fs/afs/volume.c
> index 08fdb3951c49..b968e4e96f6b 100644
> --- a/fs/afs/volume.c
> +++ b/fs/afs/volume.c
> @@ -303,6 +303,7 @@ int afs_check_volume_status(struct afs_volume *volume, struct key *key)
> ret = afs_update_volume_status(volume, key);
> clear_bit_unlock(AFS_VOLUME_WAIT, &volume->flags);
> clear_bit_unlock(AFS_VOLUME_UPDATING, &volume->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&volume->flags, AFS_VOLUME_WAIT);
> _leave(" = %d", ret);
> return ret;
> diff --git a/fs/aio.c b/fs/aio.c
> index 3490d1fa0e16..a21acb7ee2a5 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1154,9 +1154,7 @@ static void aio_complete(struct aio_kiocb *iocb)
> * like in wake_up_bit() where clearing a bit has to be
> * ordered with the unlocked test.
> */
> - smp_mb();
> -
> - if (waitqueue_active(&ctx->wait))
> + if (wq_has_sleeper(&ctx->wait))
> wake_up(&ctx->wait);
> }
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 749f5984425d..80e35cbfc7cc 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1687,6 +1687,7 @@ int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
> /* tell others that we're done */
> BUG_ON(whole->bd_claiming != holder);
> whole->bd_claiming = NULL;
> + smp_mb();
> wake_up_bit(&whole->bd_claiming, 0);
>
> spin_unlock(&bdev_lock);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index db337e53aab3..ee1136ae46fd 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3683,9 +3683,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
>
> static void end_extent_buffer_writeback(struct extent_buffer *eb)
> {
> - clear_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
> - smp_mb__after_atomic();
> - wake_up_bit(&eb->bflags, EXTENT_BUFFER_WRITEBACK);
> + clear_and_wake_up_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
> }
>
> static void set_btree_ioerr(struct page *page)
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index ecc8ecbbfa5a..ecebba72ec7d 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -267,6 +267,7 @@ void cachefiles_mark_object_inactive(struct cachefiles_cache *cache,
> clear_bit(CACHEFILES_OBJECT_ACTIVE, &object->flags);
> write_unlock(&cache->active_lock);
>
> + smp_mb__after_atomic();
> wake_up_bit(&object->flags, CACHEFILES_OBJECT_ACTIVE);
>
> /* This object can now be culled, so we need to let the daemon know
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 8c4121da624e..8c1115ed1c28 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -5235,8 +5235,7 @@ cifs_sb_tlink(struct cifs_sb_info *cifs_sb)
> }
>
> tlink->tl_tcon = cifs_construct_tcon(cifs_sb, fsuid);
> - clear_bit(TCON_LINK_PENDING, &tlink->tl_flags);
> - wake_up_bit(&tlink->tl_flags, TCON_LINK_PENDING);
> + clear_and_wake_up_bit(TCON_LINK_PENDING, &tlink->tl_flags);
>
> if (IS_ERR(tlink->tl_tcon)) {
> cifs_put_tlink(tlink);
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index b1a696a73f7c..961751d89113 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -584,10 +584,8 @@ int cifs_get_writer(struct cifsInodeInfo *cinode)
> /* Check to see if we have started servicing an oplock break */
> if (test_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags)) {
> cinode->writers--;
> - if (cinode->writers == 0) {
> - clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> - wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
> - }
> + if (cinode->writers == 0)
> + clear_and_wake_up_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> spin_unlock(&cinode->writers_lock);
> goto start;
> }
> @@ -599,10 +597,8 @@ void cifs_put_writer(struct cifsInodeInfo *cinode)
> {
> spin_lock(&cinode->writers_lock);
> cinode->writers--;
> - if (cinode->writers == 0) {
> - clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> - wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
> - }
> + if (cinode->writers == 0)
> + clear_and_wake_up_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> spin_unlock(&cinode->writers_lock);
> }
>
> @@ -630,8 +626,7 @@ void cifs_queue_oplock_break(struct cifsFileInfo *cfile)
>
> void cifs_done_oplock_break(struct cifsInodeInfo *cinode)
> {
> - clear_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags);
> - wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_OPLOCK_BREAK);
> + clear_and_wake_up_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags);
> }
>
> bool
> diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
> index 0ce39658a620..40ba8957ebf0 100644
> --- a/fs/fscache/cookie.c
> +++ b/fs/fscache/cookie.c
> @@ -381,6 +381,7 @@ void __fscache_enable_cookie(struct fscache_cookie *cookie,
>
> out_unlock:
> clear_bit_unlock(FSCACHE_COOKIE_ENABLEMENT_LOCK, &cookie->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&cookie->flags, FSCACHE_COOKIE_ENABLEMENT_LOCK);
> }
> EXPORT_SYMBOL(__fscache_enable_cookie);
> @@ -778,6 +779,7 @@ void __fscache_disable_cookie(struct fscache_cookie *cookie,
>
> out_unlock_enable:
> clear_bit_unlock(FSCACHE_COOKIE_ENABLEMENT_LOCK, &cookie->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&cookie->flags, FSCACHE_COOKIE_ENABLEMENT_LOCK);
> _leave("");
> }
> diff --git a/fs/fscache/object.c b/fs/fscache/object.c
> index cfeba839a0f2..08b2edec6596 100644
> --- a/fs/fscache/object.c
> +++ b/fs/fscache/object.c
> @@ -524,6 +524,7 @@ void fscache_object_lookup_negative(struct fscache_object *object)
>
> _debug("wake up lookup %p", &cookie->flags);
> clear_bit_unlock(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
> }
> _leave("");
> @@ -559,6 +560,7 @@ void fscache_obtained_object(struct fscache_object *object)
> * to begin shovelling data.
> */
> clear_bit_unlock(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
> } else {
> fscache_stat(&fscache_n_object_created);
> diff --git a/fs/fscache/page.c b/fs/fscache/page.c
> index 26af6fdf1538..49ef238502a3 100644
> --- a/fs/fscache/page.c
> +++ b/fs/fscache/page.c
> @@ -113,6 +113,7 @@ bool __fscache_maybe_release_page(struct fscache_cookie *cookie,
> fscache_stat(&fscache_n_store_vmscan_gone);
> }
>
> + smp_mb();
> wake_up_bit(&cookie->flags, 0);
> trace_fscache_wake_cookie(cookie);
> if (xpage)
> @@ -171,6 +172,7 @@ static void fscache_end_page_write(struct fscache_object *object,
> trace_fscache_page(cookie, page, fscache_page_write_end_pend);
> }
> spin_unlock(&cookie->stores_lock);
> + smp_mb();
> wake_up_bit(&cookie->flags, 0);
> trace_fscache_wake_cookie(cookie);
> } else {
> @@ -922,6 +924,7 @@ void fscache_invalidate_writes(struct fscache_cookie *cookie)
> put_page(results[i]);
> }
>
> + smp_mb();
> wake_up_bit(&cookie->flags, 0);
> trace_fscache_wake_cookie(cookie);
>
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index f1ebcb42cbf5..8024b6bdb6d4 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -302,9 +302,7 @@ static inline int may_grant(const struct gfs2_glock *gl, const struct gfs2_holde
>
> static void gfs2_holder_wake(struct gfs2_holder *gh)
> {
> - clear_bit(HIF_WAIT, &gh->gh_iflags);
> - smp_mb__after_atomic();
> - wake_up_bit(&gh->gh_iflags, HIF_WAIT);
> + clear_and_wake_up_bit(HIF_WAIT, &gh->gh_iflags);
> }
>
> /**
> @@ -436,9 +434,7 @@ static void state_change(struct gfs2_glock *gl, unsigned int new_state)
> static void gfs2_demote_wake(struct gfs2_glock *gl)
> {
> gl->gl_demote_state = LM_ST_EXCLUSIVE;
> - clear_bit(GLF_DEMOTE, &gl->gl_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&gl->gl_flags, GLF_DEMOTE);
> + clear_and_wake_up_bit(GLF_DEMOTE, &gl->gl_flags);
> }
>
> /**
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index cf4c767005b1..666629ea5da7 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
> return;
>
> clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
> + smp_mb__after_atomic();
> wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);
This should become clear_and_wake_up_bit as well, right? There are
several more instances of the same pattern.
> }
>
> diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
> index 4361804646d8..0fa1865dd600 100644
> --- a/fs/gfs2/lock_dlm.c
> +++ b/fs/gfs2/lock_dlm.c
> @@ -1139,9 +1139,7 @@ static void gdlm_recover_done(void *arg, struct dlm_slot *slots, int num_slots,
> if (!test_bit(DFL_UNMOUNT, &ls->ls_recover_flags))
> queue_delayed_work(gfs2_control_wq, &sdp->sd_control_work, 0);
>
> - clear_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&ls->ls_recover_flags, DFL_DLM_RECOVERY);
> + clear_and_wake_up_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
> spin_unlock(&ls->ls_recover_spin);
> }
>
> @@ -1276,9 +1274,7 @@ static int gdlm_mount(struct gfs2_sbd *sdp, const char *table)
> }
>
> ls->ls_first = !!test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags);
> - clear_bit(SDF_NOJOURNALID, &sdp->sd_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&sdp->sd_flags, SDF_NOJOURNALID);
> + clear_and_wake_up_bit(SDF_NOJOURNALID, &sdp->sd_flags);
> return 0;
>
> fail_release:
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 2299a3fa1911..ef9a8ef6689b 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -436,9 +436,7 @@ void gfs2_recover_func(struct work_struct *work)
> jd->jd_recover_error = error;
> gfs2_recovery_done(sdp, jd->jd_jid, LM_RD_GAVEUP);
> done:
> - clear_bit(JDF_RECOVERY, &jd->jd_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&jd->jd_flags, JDF_RECOVERY);
> + clear_and_wake_up_bit(JDF_RECOVERY, &jd->jd_flags);
> }
>
> int gfs2_recover_journal(struct gfs2_jdesc *jd, bool wait)
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index b70cea5c8c59..28ff2e10bdd9 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -985,6 +985,7 @@ void gfs2_freeze_func(struct work_struct *work)
> }
> deactivate_super(sb);
> clear_bit_unlock(SDF_FS_FROZEN, &sdp->sd_flags);
> + smp_mb__after_atomic();
> wake_up_bit(&sdp->sd_flags, SDF_FS_FROZEN);
> return;
> }
> diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> index 159aedf63c2a..b337798d8957 100644
> --- a/fs/gfs2/sys.c
> +++ b/fs/gfs2/sys.c
> @@ -505,9 +505,7 @@ static ssize_t jid_store(struct gfs2_sbd *sdp, const char *buf, size_t len)
> if (sdp->sd_args.ar_spectator && jid > 0)
> rv = jid = -EINVAL;
> sdp->sd_lockstruct.ls_jid = jid;
> - clear_bit(SDF_NOJOURNALID, &sdp->sd_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&sdp->sd_flags, SDF_NOJOURNALID);
> + clear_and_wake_up_bit(SDF_NOJOURNALID, &sdp->sd_flags);
> out:
> spin_unlock(&sdp->sd_jindex_spin);
> return rv ? rv : len;
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index e2e3c4f04d3e..a3ed413a38c5 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1196,9 +1196,7 @@ static int nfs4_run_state_manager(void *);
> static void nfs4_clear_state_manager_bit(struct nfs_client *clp)
> {
> smp_mb__before_atomic();
> - clear_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
> - smp_mb__after_atomic();
> - wake_up_bit(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING);
> + clear_and_wake_up_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
> rpc_wake_up(&clp->cl_rpcwaitq);
> }
>
> diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
> index c0046c348910..37c4fe5a595c 100644
> --- a/fs/nfs/pnfs_nfs.c
> +++ b/fs/nfs/pnfs_nfs.c
> @@ -567,9 +567,7 @@ static void nfs4_wait_ds_connect(struct nfs4_pnfs_ds *ds)
> static void nfs4_clear_ds_conn_bit(struct nfs4_pnfs_ds *ds)
> {
> smp_mb__before_atomic();
> - clear_bit(NFS4DS_CONNECTING, &ds->ds_state);
> - smp_mb__after_atomic();
> - wake_up_bit(&ds->ds_state, NFS4DS_CONNECTING);
> + clear_and_wake_up_bit(NFS4DS_CONNECTING, &ds->ds_state);
> }
>
> static struct nfs_client *(*get_v3_ds_connect)(
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 87679557d0d6..0a30f9cdf66a 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -1638,9 +1638,7 @@ static void
> nfsd4_cltrack_upcall_unlock(struct nfs4_client *clp)
> {
> smp_mb__before_atomic();
> - clear_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&clp->cl_flags, NFSD4_CLIENT_UPCALL_LOCK);
> + clear_and_wake_up_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
> }
>
> static void
> diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
> index a35c17017210..1958bb457143 100644
> --- a/fs/orangefs/file.c
> +++ b/fs/orangefs/file.c
> @@ -302,7 +302,7 @@ int orangefs_revalidate_mapping(struct inode *inode)
> orangefs_inode->mapping_time = jiffies +
> orangefs_cache_timeout_msecs*HZ/1000;
>
> - clear_bit(1, bitlock);
> + clear_bit_unlock(1, bitlock);
> smp_mb__after_atomic();
> wake_up_bit(bitlock, 1);
>
> diff --git a/kernel/sched/wait_bit.c b/kernel/sched/wait_bit.c
> index 45eba18a2898..2662975473e3 100644
> --- a/kernel/sched/wait_bit.c
> +++ b/kernel/sched/wait_bit.c
> @@ -189,6 +189,7 @@ EXPORT_SYMBOL(init_wait_var_entry);
>
> void wake_up_var(void *var)
> {
> + smp_mb();
> __wake_up_bit(__var_waitqueue(var), var, -1);
> }
> EXPORT_SYMBOL(wake_up_var);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 9e4fcf406d9c..4e57f1a1b7a2 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -50,9 +50,7 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
> if (status)
> return;
>
> - clear_bit(HCI_INQUIRY, &hdev->flags);
> - smp_mb__after_atomic(); /* wake_up_bit advises about this barrier */
> - wake_up_bit(&hdev->flags, HCI_INQUIRY);
> + clear_and_wake_up_bit(HCI_INQUIRY, &hdev->flags);
>
> hci_dev_lock(hdev);
> /* Set discovery state to stopped if we're not doing LE active
> @@ -2342,7 +2340,6 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
> return;
>
> - smp_mb__after_atomic(); /* wake_up_bit advises about this barrier */
> wake_up_bit(&hdev->flags, HCI_INQUIRY);
>
> if (!hci_dev_test_flag(hdev, HCI_MGMT))
> diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
> index 3cae88cbdaa0..2c2773172964 100644
> --- a/net/rds/ib_recv.c
> +++ b/net/rds/ib_recv.c
> @@ -363,6 +363,7 @@ static int acquire_refill(struct rds_connection *conn)
> static void release_refill(struct rds_connection *conn)
> {
> clear_bit(RDS_RECV_REFILL, &conn->c_flags);
> + smp_mb__after_atomic();
>
> /* 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
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 44e58a3e5663..f452816bd7d0 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -308,9 +308,8 @@ static void key_garbage_collector(struct work_struct *work)
>
> if (unlikely(gc_state & KEY_GC_REAPING_DEAD_3)) {
> kdebug("dead wake");
> - smp_mb();
> - clear_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
> - wake_up_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE);
> + smp_mb__before_atomic();
> + clear_and_wake_up_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
> }
>
> if (gc_state & KEY_GC_REAP_AGAIN)
Thanks,
Andreas
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [RFC][PATCH] wake_up_var() memory ordering
@ 2019-06-25 9:19 ` Andreas Gruenbacher
0 siblings, 0 replies; 14+ messages in thread
From: Andreas Gruenbacher @ 2019-06-25 9:19 UTC (permalink / raw)
To: cluster-devel.redhat.com
Peter,
thanks for the fixes.
On Mon, 24 Jun 2019 at 18:53, Peter Zijlstra <peterz@infradead.org> wrote:
> Hi all,
>
> I tried using wake_up_var() today and accidentally noticed that it
> didn't imply an smp_mb() and specifically requires it through
> wake_up_bit() / waitqueue_active().
>
> Now, wake_up_bit() doesn't imply the barrier because it is assumed to be
> used with the atomic bitops API which either implies (test_and_clear) or
> only needs smp_mb__after_atomic(), which is 'much' cheaper than an
> unconditional smp_mb().
>
> Still, while auditing all that, I found a whole bunch of things that
> could be improved. There were missing barriers, superfluous barriers and
> a whole bunch of sites that could use clear_and_wake_up_bit().
>
> So this fixes all wake_up_bit() usage without actually changing
> semantics of it (which are unfortunate but understandable).
thanks for the fixes.
> This does
> however change the semantics of wake_up_var(); even though wake_up_var()
> is most often used with atomics and then the additional smp_mb() is most
> often superfluous :/
>
> There isn't really a good option here, comments (other than I need to
> split this up)?
>
>
> ---
> drivers/bluetooth/btmtksdio.c | 5 +----
> drivers/bluetooth/btmtkuart.c | 5 +----
> drivers/bluetooth/hci_mrvl.c | 8 ++------
> drivers/gpu/drm/i915/i915_reset.c | 6 ++----
> drivers/md/dm-bufio.c | 10 ++--------
> drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 15 ++++-----------
> fs/afs/fs_probe.c | 1 +
> fs/afs/server.c | 1 +
> fs/afs/vl_probe.c | 1 +
> fs/afs/volume.c | 1 +
> fs/aio.c | 4 +---
> fs/block_dev.c | 1 +
> fs/btrfs/extent_io.c | 4 +---
> fs/cachefiles/namei.c | 1 +
> fs/cifs/connect.c | 3 +--
> fs/cifs/misc.c | 15 +++++----------
> fs/fscache/cookie.c | 2 ++
> fs/fscache/object.c | 2 ++
> fs/fscache/page.c | 3 +++
> fs/gfs2/glock.c | 8 ++------
> fs/gfs2/glops.c | 1 +
> fs/gfs2/lock_dlm.c | 8 ++------
> fs/gfs2/recovery.c | 4 +---
> fs/gfs2/super.c | 1 +
> fs/gfs2/sys.c | 4 +---
> fs/nfs/nfs4state.c | 4 +---
> fs/nfs/pnfs_nfs.c | 4 +---
> fs/nfsd/nfs4recover.c | 4 +---
> fs/orangefs/file.c | 2 +-
> kernel/sched/wait_bit.c | 1 +
> net/bluetooth/hci_event.c | 5 +----
> net/rds/ib_recv.c | 1 +
> security/keys/gc.c | 5 ++---
> 33 files changed, 50 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> index 813338288453..27523cfeac9a 100644
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -356,11 +356,8 @@ static int btmtksdio_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
>
> if (hdr->evt == HCI_EV_VENDOR) {
> if (test_and_clear_bit(BTMTKSDIO_TX_WAIT_VND_EVT,
> - &bdev->tx_state)) {
> - /* Barrier to sync with other CPUs */
> - smp_mb__after_atomic();
> + &bdev->tx_state))
> wake_up_bit(&bdev->tx_state, BTMTKSDIO_TX_WAIT_VND_EVT);
> - }
> }
>
> return 0;
> diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
> index f5dbeec8e274..7fe324df3799 100644
> --- a/drivers/bluetooth/btmtkuart.c
> +++ b/drivers/bluetooth/btmtkuart.c
> @@ -340,11 +340,8 @@ static int btmtkuart_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
>
> if (hdr->evt == HCI_EV_VENDOR) {
> if (test_and_clear_bit(BTMTKUART_TX_WAIT_VND_EVT,
> - &bdev->tx_state)) {
> - /* Barrier to sync with other CPUs */
> - smp_mb__after_atomic();
> + &bdev->tx_state))
> wake_up_bit(&bdev->tx_state, BTMTKUART_TX_WAIT_VND_EVT);
> - }
> }
>
> return 0;
> diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
> index 50212ac629e3..f03294d39d08 100644
> --- a/drivers/bluetooth/hci_mrvl.c
> +++ b/drivers/bluetooth/hci_mrvl.c
> @@ -157,9 +157,7 @@ static int mrvl_recv_fw_req(struct hci_dev *hdev, struct sk_buff *skb)
>
> mrvl->tx_len = le16_to_cpu(pkt->lhs);
>
> - clear_bit(STATE_FW_REQ_PENDING, &mrvl->flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&mrvl->flags, STATE_FW_REQ_PENDING);
> + clear_and_wake_up_bit(STATE_FW_REQ_PENDING, &mrvl->flags);
>
> done:
> kfree_skb(skb);
> @@ -192,9 +190,7 @@ static int mrvl_recv_chip_ver(struct hci_dev *hdev, struct sk_buff *skb)
>
> bt_dev_info(hdev, "Controller id = %x, rev = %x", mrvl->id, mrvl->rev);
>
> - clear_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&mrvl->flags, STATE_CHIP_VER_PENDING);
> + clear_and_wake_up_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
>
> done:
> kfree_skb(skb);
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 677d59304e78..6809367dbfa9 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -1324,10 +1324,8 @@ void i915_handle_error(struct drm_i915_private *i915,
> if (i915_reset_engine(engine, msg) == 0)
> engine_mask &= ~engine->mask;
>
> - clear_bit(I915_RESET_ENGINE + engine->id,
> - &error->flags);
> - wake_up_bit(&error->flags,
> - I915_RESET_ENGINE + engine->id);
> + clear_and_wake_up_bit(I915_RESET_ENGINE + engine->id,
> + &error->flags);
> }
> }
>
> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> index 2a48ea3f1b30..3114836cc717 100644
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -667,10 +667,7 @@ static void write_endio(struct dm_buffer *b, blk_status_t status)
> BUG_ON(!test_bit(B_WRITING, &b->state));
>
> smp_mb__before_atomic();
> - clear_bit(B_WRITING, &b->state);
> - smp_mb__after_atomic();
> -
> - wake_up_bit(&b->state, B_WRITING);
> + clear_and_wake_up_bit(B_WRITING, &b->state);
> }
>
> /*
> @@ -1045,10 +1042,7 @@ static void read_endio(struct dm_buffer *b, blk_status_t status)
> BUG_ON(!test_bit(B_READING, &b->state));
>
> smp_mb__before_atomic();
> - clear_bit(B_READING, &b->state);
> - smp_mb__after_atomic();
> -
> - wake_up_bit(&b->state, B_READING);
> + clear_and_wake_up_bit(B_READING, &b->state);
> }
>
> /*
> diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> index e5e056bf9dfa..fa46bc930704 100644
> --- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> +++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> @@ -374,9 +374,7 @@ static int dvb_usb_stop_feed(struct dvb_demux_feed *dvbdmxfeed)
> usb_urb_killv2(&adap->stream);
>
> /* clear 'streaming' status bit */
> - clear_bit(ADAP_STREAMING, &adap->state_bits);
> - smp_mb__after_atomic();
> - wake_up_bit(&adap->state_bits, ADAP_STREAMING);
> + clear_and_wake_up_bit(ADAP_STREAMING, &adap->state_bits);
> skip_feed_stop:
>
> if (ret)
> @@ -578,11 +576,8 @@ static int dvb_usb_fe_init(struct dvb_frontend *fe)
> goto err;
> }
> err:
> - if (!adap->suspend_resume_active) {
> - clear_bit(ADAP_INIT, &adap->state_bits);
> - smp_mb__after_atomic();
> - wake_up_bit(&adap->state_bits, ADAP_INIT);
> - }
> + if (!adap->suspend_resume_active)
> + clear_and_wake_up_bit(ADAP_INIT, &adap->state_bits);
>
> dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> return ret;
> @@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
> err:
> if (!adap->suspend_resume_active) {
> adap->active_fe = -1;
> - clear_bit(ADAP_SLEEP, &adap->state_bits);
> - smp_mb__after_atomic();
> - wake_up_bit(&adap->state_bits, ADAP_SLEEP);
> + clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
> }
>
> dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
> index cfe62b154f68..377ee07d5f76 100644
> --- a/fs/afs/fs_probe.c
> +++ b/fs/afs/fs_probe.c
> @@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
>
> wake_up_var(&server->probe_outstanding);
> clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
> return true;
> }
> diff --git a/fs/afs/server.c b/fs/afs/server.c
> index e900cd74361b..c9b74a397cc1 100644
> --- a/fs/afs/server.c
> +++ b/fs/afs/server.c
> @@ -569,6 +569,7 @@ bool afs_check_server_record(struct afs_fs_cursor *fc, struct afs_server *server
> if (!test_and_set_bit_lock(AFS_SERVER_FL_UPDATING, &server->flags)) {
> success = afs_update_server_record(fc, server);
> clear_bit_unlock(AFS_SERVER_FL_UPDATING, &server->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&server->flags, AFS_SERVER_FL_UPDATING);
> _leave(" = %d", success);
> return success;
> diff --git a/fs/afs/vl_probe.c b/fs/afs/vl_probe.c
> index 858498cc1b05..1939624aa147 100644
> --- a/fs/afs/vl_probe.c
> +++ b/fs/afs/vl_probe.c
> @@ -18,6 +18,7 @@ static bool afs_vl_probe_done(struct afs_vlserver *server)
>
> wake_up_var(&server->probe_outstanding);
> clear_bit_unlock(AFS_VLSERVER_FL_PROBING, &server->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&server->flags, AFS_VLSERVER_FL_PROBING);
> return true;
> }
> diff --git a/fs/afs/volume.c b/fs/afs/volume.c
> index 08fdb3951c49..b968e4e96f6b 100644
> --- a/fs/afs/volume.c
> +++ b/fs/afs/volume.c
> @@ -303,6 +303,7 @@ int afs_check_volume_status(struct afs_volume *volume, struct key *key)
> ret = afs_update_volume_status(volume, key);
> clear_bit_unlock(AFS_VOLUME_WAIT, &volume->flags);
> clear_bit_unlock(AFS_VOLUME_UPDATING, &volume->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&volume->flags, AFS_VOLUME_WAIT);
> _leave(" = %d", ret);
> return ret;
> diff --git a/fs/aio.c b/fs/aio.c
> index 3490d1fa0e16..a21acb7ee2a5 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1154,9 +1154,7 @@ static void aio_complete(struct aio_kiocb *iocb)
> * like in wake_up_bit() where clearing a bit has to be
> * ordered with the unlocked test.
> */
> - smp_mb();
> -
> - if (waitqueue_active(&ctx->wait))
> + if (wq_has_sleeper(&ctx->wait))
> wake_up(&ctx->wait);
> }
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 749f5984425d..80e35cbfc7cc 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1687,6 +1687,7 @@ int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
> /* tell others that we're done */
> BUG_ON(whole->bd_claiming != holder);
> whole->bd_claiming = NULL;
> + smp_mb();
> wake_up_bit(&whole->bd_claiming, 0);
>
> spin_unlock(&bdev_lock);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index db337e53aab3..ee1136ae46fd 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3683,9 +3683,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
>
> static void end_extent_buffer_writeback(struct extent_buffer *eb)
> {
> - clear_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
> - smp_mb__after_atomic();
> - wake_up_bit(&eb->bflags, EXTENT_BUFFER_WRITEBACK);
> + clear_and_wake_up_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
> }
>
> static void set_btree_ioerr(struct page *page)
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index ecc8ecbbfa5a..ecebba72ec7d 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -267,6 +267,7 @@ void cachefiles_mark_object_inactive(struct cachefiles_cache *cache,
> clear_bit(CACHEFILES_OBJECT_ACTIVE, &object->flags);
> write_unlock(&cache->active_lock);
>
> + smp_mb__after_atomic();
> wake_up_bit(&object->flags, CACHEFILES_OBJECT_ACTIVE);
>
> /* This object can now be culled, so we need to let the daemon know
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 8c4121da624e..8c1115ed1c28 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -5235,8 +5235,7 @@ cifs_sb_tlink(struct cifs_sb_info *cifs_sb)
> }
>
> tlink->tl_tcon = cifs_construct_tcon(cifs_sb, fsuid);
> - clear_bit(TCON_LINK_PENDING, &tlink->tl_flags);
> - wake_up_bit(&tlink->tl_flags, TCON_LINK_PENDING);
> + clear_and_wake_up_bit(TCON_LINK_PENDING, &tlink->tl_flags);
>
> if (IS_ERR(tlink->tl_tcon)) {
> cifs_put_tlink(tlink);
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index b1a696a73f7c..961751d89113 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -584,10 +584,8 @@ int cifs_get_writer(struct cifsInodeInfo *cinode)
> /* Check to see if we have started servicing an oplock break */
> if (test_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags)) {
> cinode->writers--;
> - if (cinode->writers == 0) {
> - clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> - wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
> - }
> + if (cinode->writers == 0)
> + clear_and_wake_up_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> spin_unlock(&cinode->writers_lock);
> goto start;
> }
> @@ -599,10 +597,8 @@ void cifs_put_writer(struct cifsInodeInfo *cinode)
> {
> spin_lock(&cinode->writers_lock);
> cinode->writers--;
> - if (cinode->writers == 0) {
> - clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> - wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
> - }
> + if (cinode->writers == 0)
> + clear_and_wake_up_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> spin_unlock(&cinode->writers_lock);
> }
>
> @@ -630,8 +626,7 @@ void cifs_queue_oplock_break(struct cifsFileInfo *cfile)
>
> void cifs_done_oplock_break(struct cifsInodeInfo *cinode)
> {
> - clear_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags);
> - wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_OPLOCK_BREAK);
> + clear_and_wake_up_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags);
> }
>
> bool
> diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
> index 0ce39658a620..40ba8957ebf0 100644
> --- a/fs/fscache/cookie.c
> +++ b/fs/fscache/cookie.c
> @@ -381,6 +381,7 @@ void __fscache_enable_cookie(struct fscache_cookie *cookie,
>
> out_unlock:
> clear_bit_unlock(FSCACHE_COOKIE_ENABLEMENT_LOCK, &cookie->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&cookie->flags, FSCACHE_COOKIE_ENABLEMENT_LOCK);
> }
> EXPORT_SYMBOL(__fscache_enable_cookie);
> @@ -778,6 +779,7 @@ void __fscache_disable_cookie(struct fscache_cookie *cookie,
>
> out_unlock_enable:
> clear_bit_unlock(FSCACHE_COOKIE_ENABLEMENT_LOCK, &cookie->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&cookie->flags, FSCACHE_COOKIE_ENABLEMENT_LOCK);
> _leave("");
> }
> diff --git a/fs/fscache/object.c b/fs/fscache/object.c
> index cfeba839a0f2..08b2edec6596 100644
> --- a/fs/fscache/object.c
> +++ b/fs/fscache/object.c
> @@ -524,6 +524,7 @@ void fscache_object_lookup_negative(struct fscache_object *object)
>
> _debug("wake up lookup %p", &cookie->flags);
> clear_bit_unlock(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
> }
> _leave("");
> @@ -559,6 +560,7 @@ void fscache_obtained_object(struct fscache_object *object)
> * to begin shovelling data.
> */
> clear_bit_unlock(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags);
> + smp_mb__after_atomic();
> wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
> } else {
> fscache_stat(&fscache_n_object_created);
> diff --git a/fs/fscache/page.c b/fs/fscache/page.c
> index 26af6fdf1538..49ef238502a3 100644
> --- a/fs/fscache/page.c
> +++ b/fs/fscache/page.c
> @@ -113,6 +113,7 @@ bool __fscache_maybe_release_page(struct fscache_cookie *cookie,
> fscache_stat(&fscache_n_store_vmscan_gone);
> }
>
> + smp_mb();
> wake_up_bit(&cookie->flags, 0);
> trace_fscache_wake_cookie(cookie);
> if (xpage)
> @@ -171,6 +172,7 @@ static void fscache_end_page_write(struct fscache_object *object,
> trace_fscache_page(cookie, page, fscache_page_write_end_pend);
> }
> spin_unlock(&cookie->stores_lock);
> + smp_mb();
> wake_up_bit(&cookie->flags, 0);
> trace_fscache_wake_cookie(cookie);
> } else {
> @@ -922,6 +924,7 @@ void fscache_invalidate_writes(struct fscache_cookie *cookie)
> put_page(results[i]);
> }
>
> + smp_mb();
> wake_up_bit(&cookie->flags, 0);
> trace_fscache_wake_cookie(cookie);
>
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index f1ebcb42cbf5..8024b6bdb6d4 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -302,9 +302,7 @@ static inline int may_grant(const struct gfs2_glock *gl, const struct gfs2_holde
>
> static void gfs2_holder_wake(struct gfs2_holder *gh)
> {
> - clear_bit(HIF_WAIT, &gh->gh_iflags);
> - smp_mb__after_atomic();
> - wake_up_bit(&gh->gh_iflags, HIF_WAIT);
> + clear_and_wake_up_bit(HIF_WAIT, &gh->gh_iflags);
> }
>
> /**
> @@ -436,9 +434,7 @@ static void state_change(struct gfs2_glock *gl, unsigned int new_state)
> static void gfs2_demote_wake(struct gfs2_glock *gl)
> {
> gl->gl_demote_state = LM_ST_EXCLUSIVE;
> - clear_bit(GLF_DEMOTE, &gl->gl_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&gl->gl_flags, GLF_DEMOTE);
> + clear_and_wake_up_bit(GLF_DEMOTE, &gl->gl_flags);
> }
>
> /**
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index cf4c767005b1..666629ea5da7 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
> return;
>
> clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
> + smp_mb__after_atomic();
> wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);
This should become clear_and_wake_up_bit as well, right? There are
several more instances of the same pattern.
> }
>
> diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
> index 4361804646d8..0fa1865dd600 100644
> --- a/fs/gfs2/lock_dlm.c
> +++ b/fs/gfs2/lock_dlm.c
> @@ -1139,9 +1139,7 @@ static void gdlm_recover_done(void *arg, struct dlm_slot *slots, int num_slots,
> if (!test_bit(DFL_UNMOUNT, &ls->ls_recover_flags))
> queue_delayed_work(gfs2_control_wq, &sdp->sd_control_work, 0);
>
> - clear_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&ls->ls_recover_flags, DFL_DLM_RECOVERY);
> + clear_and_wake_up_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
> spin_unlock(&ls->ls_recover_spin);
> }
>
> @@ -1276,9 +1274,7 @@ static int gdlm_mount(struct gfs2_sbd *sdp, const char *table)
> }
>
> ls->ls_first = !!test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags);
> - clear_bit(SDF_NOJOURNALID, &sdp->sd_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&sdp->sd_flags, SDF_NOJOURNALID);
> + clear_and_wake_up_bit(SDF_NOJOURNALID, &sdp->sd_flags);
> return 0;
>
> fail_release:
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 2299a3fa1911..ef9a8ef6689b 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -436,9 +436,7 @@ void gfs2_recover_func(struct work_struct *work)
> jd->jd_recover_error = error;
> gfs2_recovery_done(sdp, jd->jd_jid, LM_RD_GAVEUP);
> done:
> - clear_bit(JDF_RECOVERY, &jd->jd_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&jd->jd_flags, JDF_RECOVERY);
> + clear_and_wake_up_bit(JDF_RECOVERY, &jd->jd_flags);
> }
>
> int gfs2_recover_journal(struct gfs2_jdesc *jd, bool wait)
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index b70cea5c8c59..28ff2e10bdd9 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -985,6 +985,7 @@ void gfs2_freeze_func(struct work_struct *work)
> }
> deactivate_super(sb);
> clear_bit_unlock(SDF_FS_FROZEN, &sdp->sd_flags);
> + smp_mb__after_atomic();
> wake_up_bit(&sdp->sd_flags, SDF_FS_FROZEN);
> return;
> }
> diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> index 159aedf63c2a..b337798d8957 100644
> --- a/fs/gfs2/sys.c
> +++ b/fs/gfs2/sys.c
> @@ -505,9 +505,7 @@ static ssize_t jid_store(struct gfs2_sbd *sdp, const char *buf, size_t len)
> if (sdp->sd_args.ar_spectator && jid > 0)
> rv = jid = -EINVAL;
> sdp->sd_lockstruct.ls_jid = jid;
> - clear_bit(SDF_NOJOURNALID, &sdp->sd_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&sdp->sd_flags, SDF_NOJOURNALID);
> + clear_and_wake_up_bit(SDF_NOJOURNALID, &sdp->sd_flags);
> out:
> spin_unlock(&sdp->sd_jindex_spin);
> return rv ? rv : len;
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index e2e3c4f04d3e..a3ed413a38c5 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1196,9 +1196,7 @@ static int nfs4_run_state_manager(void *);
> static void nfs4_clear_state_manager_bit(struct nfs_client *clp)
> {
> smp_mb__before_atomic();
> - clear_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
> - smp_mb__after_atomic();
> - wake_up_bit(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING);
> + clear_and_wake_up_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
> rpc_wake_up(&clp->cl_rpcwaitq);
> }
>
> diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
> index c0046c348910..37c4fe5a595c 100644
> --- a/fs/nfs/pnfs_nfs.c
> +++ b/fs/nfs/pnfs_nfs.c
> @@ -567,9 +567,7 @@ static void nfs4_wait_ds_connect(struct nfs4_pnfs_ds *ds)
> static void nfs4_clear_ds_conn_bit(struct nfs4_pnfs_ds *ds)
> {
> smp_mb__before_atomic();
> - clear_bit(NFS4DS_CONNECTING, &ds->ds_state);
> - smp_mb__after_atomic();
> - wake_up_bit(&ds->ds_state, NFS4DS_CONNECTING);
> + clear_and_wake_up_bit(NFS4DS_CONNECTING, &ds->ds_state);
> }
>
> static struct nfs_client *(*get_v3_ds_connect)(
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 87679557d0d6..0a30f9cdf66a 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -1638,9 +1638,7 @@ static void
> nfsd4_cltrack_upcall_unlock(struct nfs4_client *clp)
> {
> smp_mb__before_atomic();
> - clear_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
> - smp_mb__after_atomic();
> - wake_up_bit(&clp->cl_flags, NFSD4_CLIENT_UPCALL_LOCK);
> + clear_and_wake_up_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
> }
>
> static void
> diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
> index a35c17017210..1958bb457143 100644
> --- a/fs/orangefs/file.c
> +++ b/fs/orangefs/file.c
> @@ -302,7 +302,7 @@ int orangefs_revalidate_mapping(struct inode *inode)
> orangefs_inode->mapping_time = jiffies +
> orangefs_cache_timeout_msecs*HZ/1000;
>
> - clear_bit(1, bitlock);
> + clear_bit_unlock(1, bitlock);
> smp_mb__after_atomic();
> wake_up_bit(bitlock, 1);
>
> diff --git a/kernel/sched/wait_bit.c b/kernel/sched/wait_bit.c
> index 45eba18a2898..2662975473e3 100644
> --- a/kernel/sched/wait_bit.c
> +++ b/kernel/sched/wait_bit.c
> @@ -189,6 +189,7 @@ EXPORT_SYMBOL(init_wait_var_entry);
>
> void wake_up_var(void *var)
> {
> + smp_mb();
> __wake_up_bit(__var_waitqueue(var), var, -1);
> }
> EXPORT_SYMBOL(wake_up_var);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 9e4fcf406d9c..4e57f1a1b7a2 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -50,9 +50,7 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
> if (status)
> return;
>
> - clear_bit(HCI_INQUIRY, &hdev->flags);
> - smp_mb__after_atomic(); /* wake_up_bit advises about this barrier */
> - wake_up_bit(&hdev->flags, HCI_INQUIRY);
> + clear_and_wake_up_bit(HCI_INQUIRY, &hdev->flags);
>
> hci_dev_lock(hdev);
> /* Set discovery state to stopped if we're not doing LE active
> @@ -2342,7 +2340,6 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
> return;
>
> - smp_mb__after_atomic(); /* wake_up_bit advises about this barrier */
> wake_up_bit(&hdev->flags, HCI_INQUIRY);
>
> if (!hci_dev_test_flag(hdev, HCI_MGMT))
> diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
> index 3cae88cbdaa0..2c2773172964 100644
> --- a/net/rds/ib_recv.c
> +++ b/net/rds/ib_recv.c
> @@ -363,6 +363,7 @@ static int acquire_refill(struct rds_connection *conn)
> static void release_refill(struct rds_connection *conn)
> {
> clear_bit(RDS_RECV_REFILL, &conn->c_flags);
> + smp_mb__after_atomic();
>
> /* 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
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 44e58a3e5663..f452816bd7d0 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -308,9 +308,8 @@ static void key_garbage_collector(struct work_struct *work)
>
> if (unlikely(gc_state & KEY_GC_REAPING_DEAD_3)) {
> kdebug("dead wake");
> - smp_mb();
> - clear_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
> - wake_up_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE);
> + smp_mb__before_atomic();
> + clear_and_wake_up_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
> }
>
> if (gc_state & KEY_GC_REAP_AGAIN)
Thanks,
Andreas
^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CAHc6FU7j5iW7WQoxN_OSfvK4zxv_MxTWJpiNsqFW8TEDMX1rjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC][PATCH] wake_up_var() memory ordering
2019-06-25 9:19 ` [Cluster-devel] " Andreas Gruenbacher
@ 2019-06-25 10:34 ` Peter Zijlstra
-1 siblings, 0 replies; 14+ messages in thread
From: Peter Zijlstra @ 2019-06-25 10:34 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: Martin Brandenburg, linux-cachefs-H+wXaHxf7aLQT0dZR+AlfA,
Mike Snitzer, linux-aio-Bw31MaZKKs3YtjvyW6yDsg, David Airlie,
samba-technical, Joonas Lahtinen, Will Deacon,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, David Howells,
Chris Mason, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
keyrings-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
linux-afs-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Alasdair Kergon,
Mike Marshall, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, James Morris, cluster-devel,
Antti Palosaari, Matthias Brugger, Paul McKenney,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On Tue, Jun 25, 2019 at 11:19:35AM +0200, Andreas Gruenbacher wrote:
> > diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> > index cf4c767005b1..666629ea5da7 100644
> > --- a/fs/gfs2/glops.c
> > +++ b/fs/gfs2/glops.c
> > @@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
> > return;
> >
> > clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
> > + smp_mb__after_atomic();
> > wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);
>
> This should become clear_and_wake_up_bit as well, right? There are
> several more instances of the same pattern.
Only if we do as David suggested and make clean_and_wake_up_bit()
provide the RELEASE barrier.
That is, currently clear_and_wake_up_bit() is
clear_bit()
smp_mb__after_atomic();
wake_up_bit();
But the above is:
clear_bit_unlock();
smp_mb__after_atomic();
wake_up_bit()
the difference is that _unlock() uses RELEASE semantics, where
clear_bit() does not.
The difference is illustrated with something like:
cond = true;
clear_bit()
smp_mb__after_atomic();
wake_up_bit();
In this case, a remote CPU can first observe the clear_bit() and then
the 'cond = true' store. When we use clear_bit_unlock() this is not
possible, because the RELEASE barrier ensures that everything before,
stays before.
> > }
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [RFC][PATCH] wake_up_var() memory ordering
@ 2019-06-25 10:34 ` Peter Zijlstra
0 siblings, 0 replies; 14+ messages in thread
From: Peter Zijlstra @ 2019-06-25 10:34 UTC (permalink / raw)
To: cluster-devel.redhat.com
On Tue, Jun 25, 2019 at 11:19:35AM +0200, Andreas Gruenbacher wrote:
> > diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> > index cf4c767005b1..666629ea5da7 100644
> > --- a/fs/gfs2/glops.c
> > +++ b/fs/gfs2/glops.c
> > @@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
> > return;
> >
> > clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
> > + smp_mb__after_atomic();
> > wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);
>
> This should become clear_and_wake_up_bit as well, right? There are
> several more instances of the same pattern.
Only if we do as David suggested and make clean_and_wake_up_bit()
provide the RELEASE barrier.
That is, currently clear_and_wake_up_bit() is
clear_bit()
smp_mb__after_atomic();
wake_up_bit();
But the above is:
clear_bit_unlock();
smp_mb__after_atomic();
wake_up_bit()
the difference is that _unlock() uses RELEASE semantics, where
clear_bit() does not.
The difference is illustrated with something like:
cond = true;
clear_bit()
smp_mb__after_atomic();
wake_up_bit();
In this case, a remote CPU can first observe the clear_bit() and then
the 'cond = true' store. When we use clear_bit_unlock() this is not
possible, because the RELEASE barrier ensures that everything before,
stays before.
> > }
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC][PATCH] wake_up_var() memory ordering
2019-06-25 10:34 ` [Cluster-devel] " Peter Zijlstra
@ 2019-06-25 12:12 ` Andreas Gruenbacher
-1 siblings, 0 replies; 14+ messages in thread
From: Andreas Gruenbacher @ 2019-06-25 12:12 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Martin Brandenburg, linux-cachefs, Mike Snitzer, linux-aio,
David Airlie, samba-technical, Will Deacon, dri-devel,
David Howells, Chris Mason, dm-devel, keyrings, Ingo Molnar,
linux-afs, Alasdair Kergon, Mike Marshall, linux-cifs, rds-devel,
linux-rdma, James Morris, cluster-devel, Antti Palosaari,
Matthias Brugger, Paul McKenney, intel-gfx, devel,
Serge E. Hallyn
On Tue, 25 Jun 2019 at 12:36, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jun 25, 2019 at 11:19:35AM +0200, Andreas Gruenbacher wrote:
> > > diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> > > index cf4c767005b1..666629ea5da7 100644
> > > --- a/fs/gfs2/glops.c
> > > +++ b/fs/gfs2/glops.c
> > > @@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
> > > return;
> > >
> > > clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
> > > + smp_mb__after_atomic();
> > > wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);
> >
> > This should become clear_and_wake_up_bit as well, right? There are
> > several more instances of the same pattern.
>
> Only if we do as David suggested and make clean_and_wake_up_bit()
> provide the RELEASE barrier.
(It's clear_and_wake_up_bit, not clean_and_wake_up_bit.)
> That is, currently clear_and_wake_up_bit() is
>
> clear_bit()
> smp_mb__after_atomic();
> wake_up_bit();
>
> But the above is:
>
> clear_bit_unlock();
> smp_mb__after_atomic();
> wake_up_bit()
>
> the difference is that _unlock() uses RELEASE semantics, where
> clear_bit() does not.
>
> The difference is illustrated with something like:
>
> cond = true;
> clear_bit()
> smp_mb__after_atomic();
> wake_up_bit();
>
> In this case, a remote CPU can first observe the clear_bit() and then
> the 'cond = true' store. When we use clear_bit_unlock() this is not
> possible, because the RELEASE barrier ensures that everything before,
> stays before.
Now I'm confused because clear_and_wake_up_bit() in mainline does use
clear_bit_unlock(), so it's the exact opposite of what you just said.
Thanks,
Andreas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [RFC][PATCH] wake_up_var() memory ordering
@ 2019-06-25 12:12 ` Andreas Gruenbacher
0 siblings, 0 replies; 14+ messages in thread
From: Andreas Gruenbacher @ 2019-06-25 12:12 UTC (permalink / raw)
To: cluster-devel.redhat.com
On Tue, 25 Jun 2019 at 12:36, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jun 25, 2019 at 11:19:35AM +0200, Andreas Gruenbacher wrote:
> > > diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> > > index cf4c767005b1..666629ea5da7 100644
> > > --- a/fs/gfs2/glops.c
> > > +++ b/fs/gfs2/glops.c
> > > @@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
> > > return;
> > >
> > > clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
> > > + smp_mb__after_atomic();
> > > wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);
> >
> > This should become clear_and_wake_up_bit as well, right? There are
> > several more instances of the same pattern.
>
> Only if we do as David suggested and make clean_and_wake_up_bit()
> provide the RELEASE barrier.
(It's clear_and_wake_up_bit, not clean_and_wake_up_bit.)
> That is, currently clear_and_wake_up_bit() is
>
> clear_bit()
> smp_mb__after_atomic();
> wake_up_bit();
>
> But the above is:
>
> clear_bit_unlock();
> smp_mb__after_atomic();
> wake_up_bit()
>
> the difference is that _unlock() uses RELEASE semantics, where
> clear_bit() does not.
>
> The difference is illustrated with something like:
>
> cond = true;
> clear_bit()
> smp_mb__after_atomic();
> wake_up_bit();
>
> In this case, a remote CPU can first observe the clear_bit() and then
> the 'cond = true' store. When we use clear_bit_unlock() this is not
> possible, because the RELEASE barrier ensures that everything before,
> stays before.
Now I'm confused because clear_and_wake_up_bit() in mainline does use
clear_bit_unlock(), so it's the exact opposite of what you just said.
Thanks,
Andreas
^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CAHc6FU6zUCdQZ1AfN2KYcPYVKc5bwvc0bD7=-KZpFXws+F9QZQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC][PATCH] wake_up_var() memory ordering
2019-06-25 12:12 ` [Cluster-devel] " Andreas Gruenbacher
@ 2019-06-25 13:27 ` Peter Zijlstra
-1 siblings, 0 replies; 14+ messages in thread
From: Peter Zijlstra @ 2019-06-25 13:27 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: Martin Brandenburg, linux-cachefs-H+wXaHxf7aLQT0dZR+AlfA,
Mike Snitzer, linux-aio-Bw31MaZKKs3YtjvyW6yDsg, David Airlie,
samba-technical, Joonas Lahtinen, Will Deacon,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, David Howells,
Chris Mason, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
keyrings-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
linux-afs-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Alasdair Kergon,
Mike Marshall, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, James Morris, cluster-devel,
Antti Palosaari, Matthias Brugger, Paul McKenney,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On Tue, Jun 25, 2019 at 02:12:22PM +0200, Andreas Gruenbacher wrote:
> > Only if we do as David suggested and make clean_and_wake_up_bit()
> > provide the RELEASE barrier.
>
> (It's clear_and_wake_up_bit, not clean_and_wake_up_bit.)
Yes, typing hard.
> > That is, currently clear_and_wake_up_bit() is
> >
> > clear_bit()
> > smp_mb__after_atomic();
> > wake_up_bit();
> >
> Now I'm confused because clear_and_wake_up_bit() in mainline does use
> clear_bit_unlock(), so it's the exact opposite of what you just said.
Argh; clearly I couldn't read. And then yes, you're right.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Cluster-devel] [RFC][PATCH] wake_up_var() memory ordering
@ 2019-06-25 13:27 ` Peter Zijlstra
0 siblings, 0 replies; 14+ messages in thread
From: Peter Zijlstra @ 2019-06-25 13:27 UTC (permalink / raw)
To: cluster-devel.redhat.com
On Tue, Jun 25, 2019 at 02:12:22PM +0200, Andreas Gruenbacher wrote:
> > Only if we do as David suggested and make clean_and_wake_up_bit()
> > provide the RELEASE barrier.
>
> (It's clear_and_wake_up_bit, not clean_and_wake_up_bit.)
Yes, typing hard.
> > That is, currently clear_and_wake_up_bit() is
> >
> > clear_bit()
> > smp_mb__after_atomic();
> > wake_up_bit();
> >
> Now I'm confused because clear_and_wake_up_bit() in mainline does use
> clear_bit_unlock(), so it's the exact opposite of what you just said.
Argh; clearly I couldn't read. And then yes, you're right.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-06-25 13:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-24 16:50 [RFC][PATCH] wake_up_var() memory ordering Peter Zijlstra
2019-06-24 16:50 ` [Cluster-devel] " Peter Zijlstra
2019-06-25 7:51 ` David Howells
2019-06-25 7:51 ` [Cluster-devel] " David Howells
[not found] ` <32379.1561449061-S6HVgzuS8uM4Awkfq6JHfwNdhmdF6hFW@public.gmane.org>
2019-06-25 8:11 ` Peter Zijlstra
2019-06-25 8:11 ` [Cluster-devel] " Peter Zijlstra
2019-06-25 9:19 ` Andreas Gruenbacher
2019-06-25 9:19 ` [Cluster-devel] " Andreas Gruenbacher
[not found] ` <CAHc6FU7j5iW7WQoxN_OSfvK4zxv_MxTWJpiNsqFW8TEDMX1rjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-06-25 10:34 ` Peter Zijlstra
2019-06-25 10:34 ` [Cluster-devel] " Peter Zijlstra
2019-06-25 12:12 ` Andreas Gruenbacher
2019-06-25 12:12 ` [Cluster-devel] " Andreas Gruenbacher
[not found] ` <CAHc6FU6zUCdQZ1AfN2KYcPYVKc5bwvc0bD7=-KZpFXws+F9QZQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-06-25 13:27 ` Peter Zijlstra
2019-06-25 13:27 ` [Cluster-devel] " Peter Zijlstra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.