Hi,
Once we had the discussion where we need the READ_ONCE() macro. As
modern C compiler has much freedom how to access memory, I came to
the conclusion that it would be wise to use READ_ONCE() and
WRITE_ONCE() everywhere when we use atomic operations variables.
Using atomic operations on one side and do whatever the compiler
thinks at the other side of the variable feels wrong.
The rule use READ_ONCE, WRITE_ONCE, atmic_inc, atmic_dec consistently
would be easy to follow. Thinking about where the compiler might
reorder things and break MP code is much more complicated.
Do we want to go this direction?
If yes, here is the change for struct refcnt and cond. While there
rename the filed to r_refs which is easier to grep.
ok?
bluhm
Index: dev/pci/if_iwm.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.391
diff -u -p -r1.391 if_iwm.c
--- dev/pci/if_iwm.c 8 Feb 2022 14:24:36 -0000 1.391
+++ dev/pci/if_iwm.c 8 Mar 2022 14:57:26 -0000
@@ -9975,7 +9975,7 @@ iwm_init(struct ifnet *ifp)
generation = ++sc->sc_generation;
- KASSERT(sc->task_refs.refs == 0);
+ KASSERT(READ_ONCE(sc->task_refs.r_refs) == 0);
refcnt_init(&sc->task_refs);
err = iwm_preinit(sc);
@@ -10116,7 +10116,7 @@ iwm_stop(struct ifnet *ifp)
iwm_del_task(sc, systq, &sc->mac_ctxt_task);
iwm_del_task(sc, systq, &sc->phy_ctxt_task);
iwm_del_task(sc, systq, &sc->bgscan_done_task);
- KASSERT(sc->task_refs.refs >= 1);
+ KASSERT(READ_ONCE(sc->task_refs.r_refs) >= 1);
refcnt_finalize(&sc->task_refs, "iwmstop");
iwm_stop_device(sc);
Index: dev/pci/if_iwx.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_iwx.c,v
retrieving revision 1.134
diff -u -p -r1.134 if_iwx.c
--- dev/pci/if_iwx.c 21 Jan 2022 15:51:02 -0000 1.134
+++ dev/pci/if_iwx.c 8 Mar 2022 14:57:26 -0000
@@ -8017,7 +8017,7 @@ iwx_init(struct ifnet *ifp)
if (sc->sc_nvm.sku_cap_11n_enable)
iwx_setup_ht_rates(sc);
- KASSERT(sc->task_refs.refs == 0);
+ KASSERT(READ_ONCE(sc->task_refs.r_refs) == 0);
refcnt_init(&sc->task_refs);
ifq_clr_oactive(&ifp->if_snd);
ifp->if_flags |= IFF_RUNNING;
@@ -8139,7 +8139,7 @@ iwx_stop(struct ifnet *ifp)
iwx_del_task(sc, systq, &sc->mac_ctxt_task);
iwx_del_task(sc, systq, &sc->phy_ctxt_task);
iwx_del_task(sc, systq, &sc->bgscan_done_task);
- KASSERT(sc->task_refs.refs >= 1);
+ KASSERT(READ_ONCE(sc->task_refs.r_refs) >= 1);
refcnt_finalize(&sc->task_refs, "iwxstop");
iwx_stop_device(sc);
Index: kern/kern_synch.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_synch.c,v
retrieving revision 1.182
diff -u -p -r1.182 kern_synch.c
--- kern/kern_synch.c 19 Feb 2022 23:56:18 -0000 1.182
+++ kern/kern_synch.c 8 Mar 2022 14:57:26 -0000
@@ -804,7 +804,7 @@ sys___thrwakeup(struct proc *p, void *v,
void
refcnt_init(struct refcnt *r)
{
- r->refs = 1;
+ WRITE_ONCE(r->r_refs, 1);
}
void
@@ -813,10 +813,10 @@ refcnt_take(struct refcnt *r)
#ifdef DIAGNOSTIC
u_int refcnt;
- refcnt = atomic_inc_int_nv(&r->refs);
+ refcnt = atomic_inc_int_nv(&r->r_refs);
KASSERT(refcnt != 0);
#else
- atomic_inc_int(&r->refs);
+ atomic_inc_int(&r->r_refs);
#endif
}
@@ -825,7 +825,7 @@ refcnt_rele(struct refcnt *r)
{
u_int refcnt;
- refcnt = atomic_dec_int_nv(&r->refs);
+ refcnt = atomic_dec_int_nv(&r->r_refs);
KASSERT(refcnt != ~0);
return (refcnt == 0);
@@ -844,10 +844,10 @@ refcnt_finalize(struct refcnt *r, const
struct sleep_state sls;
u_int refcnt;
- refcnt = atomic_dec_int_nv(&r->refs);
+ refcnt = atomic_dec_int_nv(&r->r_refs);
while (refcnt) {
sleep_setup(&sls, r, PWAIT, wmesg, 0);
- refcnt = r->refs;
+ refcnt = READ_ONCE(r->r_refs);
sleep_finish(&sls, refcnt);
}
}
@@ -855,13 +855,13 @@ refcnt_finalize(struct refcnt *r, const
void
cond_init(struct cond *c)
{
- c->c_wait = 1;
+ WRITE_ONCE(c->c_wait, 1);
}
void
cond_signal(struct cond *c)
{
- c->c_wait = 0;
+ WRITE_ONCE(c->c_wait, 0);
wakeup_one(c);
}
@@ -872,10 +872,10 @@ cond_wait(struct cond *c, const char *wm
struct sleep_state sls;
int wait;
- wait = c->c_wait;
+ wait = READ_ONCE(c->c_wait);
while (wait) {
sleep_setup(&sls, c, PWAIT, wmesg, 0);
- wait = c->c_wait;
+ wait = READ_ONCE(c->c_wait);
sleep_finish(&sls, wait);
}
}
Index: netinet/ip_ipsp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.268
diff -u -p -r1.268 ip_ipsp.c
--- netinet/ip_ipsp.c 4 Jan 2022 06:32:39 -0000 1.268
+++ netinet/ip_ipsp.c 8 Mar 2022 14:57:26 -0000
@@ -565,7 +565,7 @@ tdb_printit(void *addr, int full, int (*
DUMP(inext, "%p");
DUMP(onext, "%p");
DUMP(xform, "%p");
- pr("%18s: %d\n", "refcnt", tdb->tdb_refcnt.refs);
+ pr("%18s: %d\n", "refcnt", tdb->tdb_refcnt.r_refs);
DUMP(encalgxform, "%p");
DUMP(authalgxform, "%p");
DUMP(compalgxform, "%p");
@@ -625,7 +625,7 @@ tdb_printit(void *addr, int full, int (*
pr(" %s", ipsp_address(&tdb->tdb_src, buf, sizeof(buf)));
pr("->%s", ipsp_address(&tdb->tdb_dst, buf, sizeof(buf)));
pr(":%d", tdb->tdb_sproto);
- pr(" #%d", tdb->tdb_refcnt.refs);
+ pr(" #%d", tdb->tdb_refcnt.r_refs);
pr(" %08x\n", tdb->tdb_flags);
}
}
Index: sys/proc.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/proc.h,v
retrieving revision 1.328
diff -u -p -r1.328 proc.h
--- sys/proc.h 25 Feb 2022 18:05:49 -0000 1.328
+++ sys/proc.h 8 Mar 2022 14:57:26 -0000
@@ -589,7 +589,7 @@ struct sleep_state {
};
struct cond {
- int c_wait;
+ int c_wait; /* [a] initialized and waiting */
};
#define COND_INITIALIZER() { 1 }
Index: sys/refcnt.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/refcnt.h,v
retrieving revision 1.4
diff -u -p -r1.4 refcnt.h
--- sys/refcnt.h 7 Jun 2016 07:53:33 -0000 1.4
+++ sys/refcnt.h 8 Mar 2022 14:34:31 -0000
@@ -19,11 +19,16 @@
#ifndef _SYS_REFCNT_H_
#define _SYS_REFCNT_H_
+/*
+ * Locks used to protect struct members in this file:
+ * a atomic operations
+ */
+
struct refcnt {
- unsigned int refs;
+ unsigned int r_refs; /* [a] reference counter */
};
-#define REFCNT_INITIALIZER() { .refs = 1 }
+#define REFCNT_INITIALIZER() { .r_refs = 1 }
#ifdef _KERNEL
