> Date: Thu, 10 Mar 2022 21:43:02 +0100 > From: Alexander Bluhm <alexander.bl...@gmx.net> > > On Thu, Mar 10, 2022 at 07:47:16PM +0100, Mark Kettenis wrote: > > > 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. > > The memory barrier is #ifdef __alpha__. Without it the CPU might > read the wrong value. But you are right, a barrier should not > depend on DIAGNOSTIC. I guess not many Alpha MP users have Intel > wireless devices.
If you think about it, the invariants being tested by those KASSERTs should not depend on whether the old or the new value is read if another CPU is modifying that variable at the same time. Unless of course there is a refcounting bug. But even with the barrier we're not guaranteed to catch that bug. > ok? ok kettenis@ > Index: dev/pci/if_iwm.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_iwm.c,v > retrieving revision 1.392 > diff -u -p -r1.392 if_iwm.c > --- dev/pci/if_iwm.c 10 Mar 2022 15:21:08 -0000 1.392 > +++ dev/pci/if_iwm.c 10 Mar 2022 20:34:18 -0000 > @@ -9975,7 +9975,7 @@ iwm_init(struct ifnet *ifp) > > generation = ++sc->sc_generation; > > - KASSERT(atomic_load_int(&sc->task_refs.r_refs) == 0); > + KASSERT(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(atomic_load_int(&sc->task_refs.r_refs) >= 1); > + KASSERT(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.135 > diff -u -p -r1.135 if_iwx.c > --- dev/pci/if_iwx.c 10 Mar 2022 15:21:08 -0000 1.135 > +++ dev/pci/if_iwx.c 10 Mar 2022 20:34:40 -0000 > @@ -8017,7 +8017,7 @@ iwx_init(struct ifnet *ifp) > if (sc->sc_nvm.sku_cap_11n_enable) > iwx_setup_ht_rates(sc); > > - KASSERT(atomic_load_int(&sc->task_refs.r_refs) == 0); > + KASSERT(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(atomic_load_int(&sc->task_refs.r_refs) >= 1); > + KASSERT(sc->task_refs.r_refs >= 1); > refcnt_finalize(&sc->task_refs, "iwxstop"); > > iwx_stop_device(sc); >
