On Wed, Mar 09, 2022 at 08:45:35PM +0100, Alexander Bluhm wrote:
> 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.
In general, atomic_* functions have not provided implicit memory
barriers on OpenBSD.
I am not sure if the data dependency barrier is needed where
atomic_load_int() and atomic_load_long() are used. The memory ordering
guarantee is very weak and does not seem useful in any of the use cases
in the patch. However, the barrier does not appear to make things worse
in terms of correctness. Except maybe in assertions where they cause
subtle side effects.
However, the patch looks good.
OK visa@
> 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 .
>
