tech-net archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[PATCH] Switch some things to atomic_load/store_*
The attached patch switches some systems in sys/net from using
membar_* to using atomic_load/store_*. I found a few ordering bugs
while doing this -- both in missing membar_datadep_consumer (now
atomic_load_consume) and in wrongly ordered membar_producer/consumer
or nonsensical membar_sync. Compile-tested only. OK to commit?
From 7193c1f1e9e4a5e6fb52d00b965417c613e7b126 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Wed, 29 Jan 2020 15:30:24 +0000
Subject: [PATCH 1/6] Fix wrong memory order and switch bpf to
atomic_load/store_*.
---
sys/net/bpf.c | 23 ++++++++---------------
sys/net/bpfjit.c | 5 ++---
2 files changed, 10 insertions(+), 18 deletions(-)
diff --git a/sys/net/bpf.c b/sys/net/bpf.c
index b1b646f6c5cc..b6c2a10cd752 100644
--- a/sys/net/bpf.c
+++ b/sys/net/bpf.c
@@ -301,10 +301,13 @@ const struct cdevsw bpf_cdevsw = {
bpfjit_func_t
bpf_jit_generate(bpf_ctx_t *bc, void *code, size_t size)
{
+ struct bpfjit_ops *ops = &bpfjit_module_ops;
+ bpfjit_func_t (*generate_code)(const bpf_ctx_t *,
+ const struct bpf_insn *, size_t);
- membar_consumer();
- if (bpfjit_module_ops.bj_generate_code != NULL) {
- return bpfjit_module_ops.bj_generate_code(bc, code, size);
+ generate_code = atomic_load_acquire(&ops->bj_generate_code);
+ if (generate_code != NULL) {
+ return generate_code(bc, code, size);
}
return NULL;
}
@@ -1289,7 +1292,6 @@ bpf_setf(struct bpf_d *d, struct bpf_program *fp)
kmem_free(fcode, size);
return EINVAL;
}
- membar_consumer();
if (bpf_jit)
jcode = bpf_jit_generate(NULL, fcode, flen);
} else {
@@ -1306,8 +1308,7 @@ bpf_setf(struct bpf_d *d, struct bpf_program *fp)
mutex_enter(&bpf_mtx);
mutex_enter(d->bd_mtx);
oldf = d->bd_filter;
- d->bd_filter = newf;
- membar_producer();
+ atomic_store_release(&d->bd_filter, newf);
reset_d(d);
pserialize_perform(bpf_psz);
mutex_exit(d->bd_mtx);
@@ -1607,8 +1608,7 @@ bpf_deliver(struct bpf_if *bp, void *(*cpfn)(void *, const void *, size_t),
atomic_inc_ulong(&d->bd_rcount);
BPF_STATINC(recv);
- filter = d->bd_filter;
- membar_datadep_consumer();
+ filter = atomic_load_consume(&d->bd_filter);
if (filter != NULL) {
if (filter->bf_jitcode != NULL)
slen = filter->bf_jitcode(NULL, &args);
@@ -2308,13 +2308,6 @@ sysctl_net_bpf_jit(SYSCTLFN_ARGS)
return error;
bpf_jit = newval;
-
- /*
- * Do a full sync to publish new bpf_jit value and
- * update bpfjit_module_ops.bj_generate_code variable.
- */
- membar_sync();
-
if (newval && bpfjit_module_ops.bj_generate_code == NULL) {
printf("JIT compilation is postponed "
"until after bpfjit module is loaded\n");
diff --git a/sys/net/bpfjit.c b/sys/net/bpfjit.c
index f250cccfd822..ee88f9113156 100644
--- a/sys/net/bpfjit.c
+++ b/sys/net/bpfjit.c
@@ -231,9 +231,8 @@ bpfjit_modcmd(modcmd_t cmd, void *arg)
switch (cmd) {
case MODULE_CMD_INIT:
bpfjit_module_ops.bj_free_code = &bpfjit_free_code;
- membar_producer();
- bpfjit_module_ops.bj_generate_code = &bpfjit_generate_code;
- membar_producer();
+ atomic_store_release(&bpfjit_module_ops.bj_generate_code,
+ &bpfjit_generate_code);
return 0;
case MODULE_CMD_FINI:
From f71fa6de65a95a81229de8db0dfaec774a47f637 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Wed, 29 Jan 2020 15:36:53 +0000
Subject: [PATCH 2/6] Fix wrong memory order and switch pfil to
atomic_load/store_*.
---
sys/net/pfil.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/sys/net/pfil.c b/sys/net/pfil.c
index 677f8b586251..0e71f9127050 100644
--- a/sys/net/pfil.c
+++ b/sys/net/pfil.c
@@ -232,8 +232,7 @@ pfil_list_add(pfil_listset_t *phlistset, pfil_polyfunc_t func, void *arg,
pfh->pfil_arg = arg;
/* switch from oldlist to newlist */
- membar_producer();
- phlistset->active = newlist;
+ atomic_store_release(&phlistset->active, newlist);
#ifdef NET_MPSAFE
pserialize_perform(pfil_psz);
#endif
@@ -336,8 +335,7 @@ pfil_list_remove(pfil_listset_t *phlistset, pfil_polyfunc_t func, void *arg)
newlist->nhooks--;
/* switch from oldlist to newlist */
- phlistset->active = newlist;
- membar_producer();
+ atomic_store_release(&phlistset->active, newlist);
#ifdef NET_MPSAFE
pserialize_perform(pfil_psz);
#endif
@@ -406,8 +404,7 @@ pfil_run_hooks(pfil_head_t *ph, struct mbuf **mp, ifnet_t *ifp, int dir)
bound = curlwp_bind();
s = pserialize_read_enter();
- phlist = phlistset->active;
- membar_datadep_consumer();
+ phlist = atomic_load_consume(&phlistset->active);
psref_acquire(&psref, &phlist->psref, pfil_psref_class);
pserialize_read_exit(s);
for (u_int i = 0; i < phlist->nhooks; i++) {
@@ -436,8 +433,7 @@ pfil_run_arg(pfil_listset_t *phlistset, u_long cmd, void *arg)
bound = curlwp_bind();
s = pserialize_read_enter();
- phlist = phlistset->active;
- membar_datadep_consumer();
+ phlist = atomic_load_consume(&phlistset->active);
psref_acquire(&psref, &phlist->psref, pfil_psref_class);
pserialize_read_exit(s);
for (u_int i = 0; i < phlist->nhooks; i++) {
From 53e37c4e68dabbeb5a71e85aaeeeaeb71e80cdec Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Wed, 29 Jan 2020 15:44:42 +0000
Subject: [PATCH 3/6] Switch if_gif to atomic_load/store_*.
---
sys/net/if_gif.c | 5 ++---
sys/net/if_gif.h | 3 +--
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/sys/net/if_gif.c b/sys/net/if_gif.c
index 06f34c04bd73..ed09a73543ef 100644
--- a/sys/net/if_gif.c
+++ b/sys/net/if_gif.c
@@ -40,6 +40,7 @@ __KERNEL_RCSID(0, "$NetBSD: if_gif.c,v 1.150 2019/10/30 03:45:59 knakahara Exp $
#include <sys/param.h>
#include <sys/systm.h>
+#include <sys/atomic.h>
#include <sys/kernel.h>
#include <sys/mbuf.h>
#include <sys/socket.h>
@@ -1130,7 +1131,6 @@ gif_set_tunnel(struct ifnet *ifp, struct sockaddr *src, struct sockaddr *dst)
if (error)
goto out;
psref_target_init(&nvar->gv_psref, gv_psref_class);
- membar_producer();
gif_update_variant(sc, nvar);
mutex_exit(&sc->gif_lock);
@@ -1207,7 +1207,6 @@ gif_delete_tunnel(struct ifnet *ifp)
nvar->gv_encap_cookie6 = NULL;
nvar->gv_output = NULL;
psref_target_init(&nvar->gv_psref, gv_psref_class);
- membar_producer();
gif_update_variant(sc, nvar);
mutex_exit(&sc->gif_lock);
@@ -1240,7 +1239,7 @@ gif_update_variant(struct gif_softc *sc, struct gif_variant *nvar)
KASSERT(mutex_owned(&sc->gif_lock));
- sc->gif_var = nvar;
+ atomic_store_release(&sc->gif_var, nvar);
pserialize_perform(sc->gif_psz);
psref_target_destroy(&ovar->gv_psref, gv_psref_class);
diff --git a/sys/net/if_gif.h b/sys/net/if_gif.h
index ae46cb7e2a15..7f522a37527c 100644
--- a/sys/net/if_gif.h
+++ b/sys/net/if_gif.h
@@ -101,9 +101,8 @@ gif_getref_variant(struct gif_softc *sc, struct psref *psref)
int s;
s = pserialize_read_enter();
- var = sc->gif_var;
+ var = atomic_load_consume(&sc->gif_var);
KASSERT(var != NULL);
- membar_datadep_consumer();
psref_acquire(psref, &var->gv_psref, gv_psref_class);
pserialize_read_exit(s);
From b600b6b7212784cf5c738541534db0aaadedb8ee Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Wed, 29 Jan 2020 15:53:00 +0000
Subject: [PATCH 4/6] Fix order in rollback case; switch if_ipsec to
atomic_load/store_*.
---
sys/net/if_ipsec.c | 10 ++++------
sys/net/if_ipsec.h | 3 +--
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/sys/net/if_ipsec.c b/sys/net/if_ipsec.c
index 481ae7adbc99..6da2f50f30b5 100644
--- a/sys/net/if_ipsec.c
+++ b/sys/net/if_ipsec.c
@@ -34,6 +34,7 @@ __KERNEL_RCSID(0, "$NetBSD: if_ipsec.c,v 1.25 2019/11/01 04:28:14 knakahara Exp
#endif
#include <sys/param.h>
+#include <sys/atomic.h>
#include <sys/systm.h>
#include <sys/kernel.h>
#include <sys/mbuf.h>
@@ -1132,7 +1133,6 @@ if_ipsec_set_tunnel(struct ifnet *ifp,
if_ipsec_copy_variant(nullvar, ovar);
if_ipsec_clear_config(nullvar);
psref_target_init(&nullvar->iv_psref, iv_psref_class);
- membar_producer();
/*
* (2-3) Swap variant include its SPs.
*/
@@ -1238,7 +1238,6 @@ if_ipsec_delete_tunnel(struct ifnet *ifp)
if_ipsec_copy_variant(nullvar, ovar);
if_ipsec_clear_config(nullvar);
psref_target_init(&nullvar->iv_psref, iv_psref_class);
- membar_producer();
/*
* (2-3) Swap variant include its SPs.
*/
@@ -1325,7 +1324,6 @@ if_ipsec_ensure_flags(struct ifnet *ifp, u_short oflags)
if_ipsec_copy_variant(nullvar, ovar);
if_ipsec_clear_config(nullvar);
psref_target_init(&nullvar->iv_psref, iv_psref_class);
- membar_producer();
/*
* (2-3) Swap variant include its SPs.
*/
@@ -1896,16 +1894,16 @@ if_ipsec_update_variant(struct ipsec_softc *sc, struct ipsec_variant *nvar,
* we stop packet processing while replacing SPs, that is, we set
* "null" config variant to sc->ipsec_var.
*/
- sc->ipsec_var = nullvar;
+ atomic_store_release(&sc->ipsec_var, nullvar);
pserialize_perform(sc->ipsec_psz);
psref_target_destroy(&ovar->iv_psref, iv_psref_class);
error = if_ipsec_replace_sp(sc, ovar, nvar);
if (!error)
- sc->ipsec_var = nvar;
+ atomic_store_release(&sc->ipsec_var, nvar);
else {
- sc->ipsec_var = ovar; /* rollback */
psref_target_init(&ovar->iv_psref, iv_psref_class);
+ atomic_store_release(&sc->ipsec_var, ovar); /* rollback */
}
pserialize_perform(sc->ipsec_psz);
diff --git a/sys/net/if_ipsec.h b/sys/net/if_ipsec.h
index 25529ccf95eb..0db5010af991 100644
--- a/sys/net/if_ipsec.h
+++ b/sys/net/if_ipsec.h
@@ -165,9 +165,8 @@ if_ipsec_getref_variant(struct ipsec_softc *sc, struct psref *psref)
int s;
s = pserialize_read_enter();
- var = sc->ipsec_var;
+ var = atomic_load_consume(&sc->ipsec_var);
KASSERT(var != NULL);
- membar_datadep_consumer();
psref_acquire(psref, &var->iv_psref, iv_psref_class);
pserialize_read_exit(s);
From 6c7d7b0ea4d7171afcfa215c3ab8e37b6860acd3 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Wed, 29 Jan 2020 15:58:35 +0000
Subject: [PATCH 5/6] Switch if_l2tp to atomic_load/store_*.
Fix missing membar_datadep_consumer -- now atomic_load_consume -- in
l2tp_lookup_session_ref.
---
sys/net/if_l2tp.c | 20 ++++----------------
sys/net/if_l2tp.h | 3 +--
2 files changed, 5 insertions(+), 18 deletions(-)
diff --git a/sys/net/if_l2tp.c b/sys/net/if_l2tp.c
index f4bf2d9648a4..e71d78fd406a 100644
--- a/sys/net/if_l2tp.c
+++ b/sys/net/if_l2tp.c
@@ -1062,7 +1062,6 @@ l2tp_set_tunnel(struct ifnet *ifp, struct sockaddr *src, struct sockaddr *dst)
encap_lock_exit();
goto error;
}
- membar_producer();
l2tp_variant_update(sc, nvar);
mutex_exit(&sc->l2tp_lock);
@@ -1111,7 +1110,6 @@ l2tp_delete_tunnel(struct ifnet *ifp)
psref_target_init(&nvar->lv_psref, lv_psref_class);
nvar->lv_psrc = NULL;
nvar->lv_pdst = NULL;
- membar_producer();
l2tp_variant_update(sc, nvar);
mutex_exit(&sc->l2tp_lock);
@@ -1186,7 +1184,6 @@ l2tp_set_session(struct l2tp_softc *sc, uint32_t my_sess_id,
psref_target_init(&nvar->lv_psref, lv_psref_class);
nvar->lv_my_sess_id = my_sess_id;
nvar->lv_peer_sess_id = peer_sess_id;
- membar_producer();
mutex_enter(&l2tp_hash.lock);
if (ovar->lv_my_sess_id > 0 && ovar->lv_peer_sess_id > 0) {
@@ -1227,7 +1224,6 @@ l2tp_clear_session(struct l2tp_softc *sc)
psref_target_init(&nvar->lv_psref, lv_psref_class);
nvar->lv_my_sess_id = 0;
nvar->lv_peer_sess_id = 0;
- membar_producer();
mutex_enter(&l2tp_hash.lock);
if (ovar->lv_my_sess_id > 0 && ovar->lv_peer_sess_id > 0) {
@@ -1254,7 +1250,7 @@ l2tp_lookup_session_ref(uint32_t id, struct psref *psref)
s = pserialize_read_enter();
PSLIST_READER_FOREACH(sc, &l2tp_hash.lists[idx], struct l2tp_softc,
l2tp_hash) {
- struct l2tp_variant *var = sc->l2tp_var;
+ struct l2tp_variant *var = atomic_load_consume(&sc->l2tp_var);
if (var == NULL)
continue;
if (var->lv_my_sess_id != id)
@@ -1283,17 +1279,12 @@ l2tp_variant_update(struct l2tp_softc *sc, struct l2tp_variant *nvar)
KASSERT(mutex_owned(&sc->l2tp_lock));
- sc->l2tp_var = nvar;
+ atomic_store_release(&sc->l2tp_var, nvar);
pserialize_perform(sc->l2tp_psz);
psref_target_destroy(&ovar->lv_psref, lv_psref_class);
- /*
- * In the manual of atomic_swap_ptr(3), there is no mention if 2nd
- * argument is rewrite or not. So, use sc->l2tp_var instead of nvar.
- */
- if (sc->l2tp_var != NULL) {
- if (sc->l2tp_var->lv_psrc != NULL
- && sc->l2tp_var->lv_pdst != NULL)
+ if (nvar != NULL) {
+ if (nvar->lv_psrc != NULL && nvar->lv_pdst != NULL)
ifp->if_flags |= IFF_RUNNING;
else
ifp->if_flags &= ~IFF_RUNNING;
@@ -1324,7 +1315,6 @@ l2tp_set_cookie(struct l2tp_softc *sc, uint64_t my_cookie, u_int my_cookie_len,
nvar->lv_peer_cookie = peer_cookie;
nvar->lv_peer_cookie_len = peer_cookie_len;
nvar->lv_use_cookie = L2TP_COOKIE_ON;
- membar_producer();
l2tp_variant_update(sc, nvar);
mutex_exit(&sc->l2tp_lock);
@@ -1358,7 +1348,6 @@ l2tp_clear_cookie(struct l2tp_softc *sc)
nvar->lv_peer_cookie = 0;
nvar->lv_peer_cookie_len = 0;
nvar->lv_use_cookie = L2TP_COOKIE_OFF;
- membar_producer();
l2tp_variant_update(sc, nvar);
mutex_exit(&sc->l2tp_lock);
@@ -1377,7 +1366,6 @@ l2tp_set_state(struct l2tp_softc *sc, int state)
*nvar = *sc->l2tp_var;
psref_target_init(&nvar->lv_psref, lv_psref_class);
nvar->lv_state = state;
- membar_producer();
l2tp_variant_update(sc, nvar);
if (nvar->lv_state == L2TP_STATE_UP) {
diff --git a/sys/net/if_l2tp.h b/sys/net/if_l2tp.h
index cbadbbcd93cc..bd9afadc654c 100644
--- a/sys/net/if_l2tp.h
+++ b/sys/net/if_l2tp.h
@@ -131,12 +131,11 @@ l2tp_getref_variant(struct l2tp_softc *sc, struct psref *psref)
int s;
s = pserialize_read_enter();
- var = sc->l2tp_var;
+ var = atomic_load_consume(&sc->l2tp_var);
if (var == NULL) {
pserialize_read_exit(s);
return NULL;
}
- membar_datadep_consumer();
psref_acquire(psref, &var->lv_psref, lv_psref_class);
pserialize_read_exit(s);
From 01bc27c096a1054d6993a5e5de755f5f43f8a7e9 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Wed, 29 Jan 2020 16:01:55 +0000
Subject: [PATCH 6/6] Switch if_vlan to atomic_load/store_*.
Fix missing membar_datadep_consumer -- now atomic_load_consume -- in
vlan_lookup_tag_psref.
---
sys/net/if_vlan.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c
index 9bae536569a1..2769c4da12a8 100644
--- a/sys/net/if_vlan.c
+++ b/sys/net/if_vlan.c
@@ -780,12 +780,11 @@ vlan_getref_linkmib(struct ifvlan *sc, struct psref *psref)
int s;
s = pserialize_read_enter();
- mib = sc->ifv_mib;
+ mib = atomic_load_consume(&sc->ifv_mib);
if (mib == NULL) {
pserialize_read_exit(s);
return NULL;
}
- membar_datadep_consumer();
psref_acquire(psref, &mib->ifvm_psref, ifvm_psref_class);
pserialize_read_exit(s);
@@ -812,7 +811,7 @@ vlan_lookup_tag_psref(struct ifnet *ifp, uint16_t tag, struct psref *psref)
s = pserialize_read_enter();
PSLIST_READER_FOREACH(sc, &ifv_hash.lists[idx], struct ifvlan,
ifv_hash) {
- struct ifvlan_linkmib *mib = sc->ifv_mib;
+ struct ifvlan_linkmib *mib = atomic_load_consume(&sc->ifv_mib);
if (mib == NULL)
continue;
if (mib->ifvm_tag != tag)
@@ -835,8 +834,7 @@ vlan_linkmib_update(struct ifvlan *ifv, struct ifvlan_linkmib *nmib)
KASSERT(mutex_owned(&ifv->ifv_lock));
- membar_producer();
- ifv->ifv_mib = nmib;
+ atomic_store_release(&ifv->ifv_mib, nmib);
pserialize_perform(ifv->ifv_psz);
psref_target_destroy(&omib->ifvm_psref, ifvm_psref_class);
Home |
Main Index |
Thread Index |
Old Index