On Tue, Mar 08, 2022 at 04:55:56PM +0100, Alexander Bluhm wrote:
> 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, atomic_inc, atomic_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?
mvs@ mentioned that FreeBSD has atomic load and store instructions
for that. I decided to implement them as static inline functions
as they provide stronger type checks. Also I add them for int and
long only, everything else is not atomic.
> If yes, here is the change for struct refcnt and cond. While there
> rename the field to r_refs which is easier to grep.
Note that the _init functions do not need atomic operations. But
the whole idea is to make it consistent and have a simple rule. If
an MP variable locking is marked as atomic, use the atomic_ functions.
As a bonus alpha gets the membar it needs.
ok?
bluhm
Index: sys/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
--- sys/dev/pci/if_iwm.c 8 Feb 2022 14:24:36 -0000 1.391
+++ sys/dev/pci/if_iwm.c 9 Mar 2022 18:52:52 -0000
@@ -9975,7 +9975,7 @@ iwm_init(struct ifnet *ifp)
generation = ++sc->sc_generation;
- KASSERT(sc->task_refs.refs == 0);
+ KASSERT(atomic_load_int(&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(atomic_load_int(&sc->task_refs.r_refs) >= 1);
refcnt_finalize(&sc->task_refs, "iwmstop");
iwm_stop_device(sc);
Index: sys/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
--- sys/dev/pci/if_iwx.c 21 Jan 2022 15:51:02 -0000 1.134
+++ sys/dev/pci/if_iwx.c 9 Mar 2022 18:53:50 -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(atomic_load_int(&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(atomic_load_int(&sc->task_refs.r_refs) >= 1);
refcnt_finalize(&sc->task_refs, "iwxstop");
iwx_stop_device(sc);
Index: sys/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
--- sys/kern/kern_synch.c 19 Feb 2022 23:56:18 -0000 1.182
+++ sys/kern/kern_synch.c 9 Mar 2022 18:57:53 -0000
@@ -804,7 +804,7 @@ sys___thrwakeup(struct proc *p, void *v,
void
refcnt_init(struct refcnt *r)
{
- r->refs = 1;
+ atomic_store_int(&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 = atomic_load_int(&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;
+ atomic_store_int(&c->c_wait, 1);
}
void
cond_signal(struct cond *c)
{
- c->c_wait = 0;
+ atomic_store_int(&c->c_wait, 0);
wakeup_one(c);
}
@@ -870,12 +870,12 @@ void
cond_wait(struct cond *c, const char *wmesg)
{
struct sleep_state sls;
- int wait;
+ unsigned int wait;
- wait = c->c_wait;
+ wait = atomic_load_int(&c->c_wait);
while (wait) {
sleep_setup(&sls, c, PWAIT, wmesg, 0);
- wait = c->c_wait;
+ wait = atomic_load_int(&c->c_wait);
sleep_finish(&sls, wait);
}
}
Index: sys/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
--- sys/netinet/ip_ipsp.c 4 Jan 2022 06:32:39 -0000 1.268
+++ sys/netinet/ip_ipsp.c 9 Mar 2022 18:50:02 -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/sys/atomic.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/atomic.h,v
retrieving revision 1.6
diff -u -p -r1.6 atomic.h
--- sys/sys/atomic.h 9 Mar 2019 06:14:21 -0000 1.6
+++ sys/sys/atomic.h 9 Mar 2022 18:48:42 -0000
@@ -1,6 +1,7 @@
/* $OpenBSD: atomic.h,v 1.6 2019/03/09 06:14:21 visa Exp $ */
/*
* Copyright (c) 2014 David Gwynne <d...@openbsd.org>
+ * Copyright (c) 2022 Alexander Bluhm <bl...@openbsd.org>
*
* Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
@@ -194,6 +195,50 @@ atomic_sub_long_nv(volatile unsigned lon
#ifndef atomic_dec_long
#define atomic_dec_long(_p) ((void)atomic_dec_long_nv(_p))
#endif
+
+#ifdef _KERNEL
+/*
+ * atomic_load_* - read from memory
+ */
+
+static void membar_datadep_consumer(void);
+
+static inline unsigned int
+atomic_load_int(volatile unsigned int *p)
+{
+ unsigned int v;
+
+ v = *p;
+ membar_datadep_consumer();
+ return v;
+}
+
+static inline unsigned long
+atomic_load_long(volatile unsigned long *p)
+{
+ unsigned long v;
+
+ v = *p;
+ membar_datadep_consumer();
+ return v;
+}
+
+/*
+ * atomic_store_* - write to memory
+ */
+
+static inline void
+atomic_store_int(volatile unsigned int *p, unsigned int v)
+{
+ *p = v;
+}
+
+static inline void
+atomic_store_long(volatile unsigned long *p, unsigned long v)
+{
+ *p = v;
+}
+#endif /* _KERNEL */
/*
* memory barriers
Index: sys/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/sys/proc.h 25 Feb 2022 18:05:49 -0000 1.328
+++ sys/sys/proc.h 9 Mar 2022 19:01:14 -0000
@@ -589,10 +589,10 @@ struct sleep_state {
};
struct cond {
- int c_wait;
+ unsigned int c_wait; /* [a] initialized and waiting */
};
-#define COND_INITIALIZER() { 1 }
+#define COND_INITIALIZER() { .c_wait = 1 }
#if defined(MULTIPROCESSOR)
void proc_trampoline_mp(void); /* XXX */
Index: sys/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/sys/refcnt.h 7 Jun 2016 07:53:33 -0000 1.4
+++ sys/sys/refcnt.h 9 Mar 2022 18:58:45 -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
Index: share/man/man9/Makefile
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/share/man/man9/Makefile,v
retrieving revision 1.306
diff -u -p -r1.306 Makefile
--- share/man/man9/Makefile 1 Aug 2020 08:40:20 -0000 1.306
+++ share/man/man9/Makefile 9 Mar 2022 18:46:17 -0000
@@ -4,8 +4,8 @@
# Makefile for section 9 (kernel function and variable) manual pages.
MAN= aml_evalnode.9 atomic_add_int.9 atomic_cas_uint.9 \
- atomic_dec_int.9 atomic_inc_int.9 atomic_setbits_int.9 \
- atomic_sub_int.9 atomic_swap_uint.9 \
+ atomic_dec_int.9 atomic_inc_int.9 atomic_load_int.9 \
+ atomic_setbits_int.9 atomic_sub_int.9 atomic_swap_uint.9 \
audio.9 autoconf.9 \
bemtoh32.9 bio_register.9 bintimeadd.9 boot.9 bpf_mtap.9 buffercache.9 \
bufq_init.9 bus_dma.9 bus_space.9 \
Index: share/man/man9/atomic_load_int.9
===================================================================
RCS file: share/man/man9/atomic_load_int.9
diff -N share/man/man9/atomic_load_int.9
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ share/man/man9/atomic_load_int.9 9 Mar 2022 10:20:06 -0000
@@ -0,0 +1,68 @@
+.\" $OpenBSD$
+.\"
+.\" Copyright (c) 2014 David Gwynne <d...@openbsd.org>
+.\" Copyright (c) 2022 Alexander Bluhm <bl...@openbsd.org>
+.\" All rights reserved.
+.\"
+.\" Permission to use, copy, modify, and distribute this software for any
+.\" purpose with or without fee is hereby granted, provided that the above
+.\" copyright notice and this permission notice appear in all copies.
+.\"
+.\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+.\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+.\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+.\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+.\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+.\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+.\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+.\"
+.Dd $Mdocdate: February 13 2014 $
+.Dt ATOMIC_LOAD_INT 9
+.Os
+.Sh NAME
+.Nm atomic_load_int ,
+.Nm atomic_load_long ,
+.Nm atomic_store_long ,
+.Nm atomic_store_int
+.Nd atomic read and write memory operations
+.Sh SYNOPSIS
+.In sys/atomic.h
+.Ft unsigned int
+.Fn atomic_load_int "volatile unsigned int *p"
+.Ft unsigned long
+.Fn atomic_load_long "volatile unsigned long *p"
+.Ft void
+.Fn atomic_store_int "volatile unsigned int *p" "unsigned int v"
+.Ft void
+.Fn atomic_store_long "volatile unsigned long *p" "unsigned long v"
+.Sh DESCRIPTION
+The atomic_load and atomic_store set of functions provide an interface
+for atomically performing read or write memory operations with
+respect to interrupts and multiple processors in the system.
+.Pp
+The atomic_store functions change the value referenced by the pointer
+.Fa p
+to the value
+.Fa v .
+.Sh CONTEXT
+.Fn atomic_load_int ,
+.Fn atomic_load_long ,
+.Fn atomic_store_int ,
+and
+.Fn atomic_store_long
+can all be called during autoconf, from process context, or from
+interrupt context.
+.Sh RETURN VALUES
+.Nm atomic_load_int
+and
+.Nm atomic_load_long
+return the value at
+.Fa p .
+.Sh SEE ALSO
+.Xr atomic_add_int 9 ,
+.Xr atomic_add_long 9 ,
+.Xr atomic_sub_int 9 ,
+.Xr atomic_sub_long 9
+.Sh HISTORY
+The atomic_load and atomic_store functions first appeared in
+.Ox 7.1 .
