> Date: Thu, 10 Mar 2022 09:05:54 +0000
> From: Visa Hankala <[email protected]>
>
> 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@
I have some doubts about using atomic_load_xxx() in KASSERTs. A
KASSERT really shoudn't have any side-effects. But atomic_load_xxx()
has a memory barrier in it, which does have the side-effect of
ordering memory accesses.
> > 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 <[email protected]>
> > + * Copyright (c) 2022 Alexander Bluhm <[email protected]>
> > *
> > * 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 <[email protected]>
> > +.\" Copyright (c) 2022 Alexander Bluhm <[email protected]>
> > +.\" 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 .
> >
>
>
