On Mon, Apr 25, 2022 at 02:52:08PM +0000, Visa Hankala wrote:
>
> The patch uses membar_sync(), and not membar_enter(), after the loop
> in refcnt_finalize() because subsequent memory operations should hinge
> on the load of r_refs.
>
> membar_enter() is usable when the reference point is a store.
>
> > The other issue I have with the diff is that it documentations the
> > memory ordering in terms of acquire and release which is not what we
> > do in other places such as the membar_enter(9) man page. Maybe this
> > should explicitly call out the memory ordering like what the Linux
> > comment does.
>
> I have updated the documentation, though I am not sure if the outcome
> is an improvement.
OK bluhm@
> Index: share/man/man9/refcnt_init.9
> ===================================================================
> RCS file: src/share/man/man9/refcnt_init.9,v
> retrieving revision 1.2
> diff -u -p -r1.2 refcnt_init.9
> --- share/man/man9/refcnt_init.9 16 Mar 2022 14:13:01 -0000 1.2
> +++ share/man/man9/refcnt_init.9 25 Apr 2022 14:34:05 -0000
> @@ -74,6 +74,17 @@ There may only be one caller to
> per refcnt
> .Fa r .
> .Pp
> +.Fn refcnt_rele ,
> +.Fn refcnt_rele_wake
> +and
> +.Fn refcnt_finalize
> +order prior memory loads and stores before the release of the reference.
> +The functions enforce control dependency so that after the final reference
> +has been released, subsequent loads and stores happen after the release.
> +These ensure that concurrent accesses cease before the object's destructor
> +runs and that the destructor sees all updates done during the lifetime
> +of the object.
> +.Pp
> .Fn refcnt_shared
> tests if the object has multiple references.
> .Pp
> Index: sys/kern/kern_synch.c
> ===================================================================
> RCS file: src/sys/kern/kern_synch.c,v
> retrieving revision 1.185
> diff -u -p -r1.185 kern_synch.c
> --- sys/kern/kern_synch.c 18 Mar 2022 15:32:06 -0000 1.185
> +++ sys/kern/kern_synch.c 25 Apr 2022 14:34:05 -0000
> @@ -822,9 +822,14 @@ refcnt_rele(struct refcnt *r)
> {
> u_int refs;
>
> + membar_exit_before_atomic();
> refs = atomic_dec_int_nv(&r->r_refs);
> KASSERT(refs != ~0);
> - return (refs == 0);
> + if (refs == 0) {
> + membar_enter_after_atomic();
> + return (1);
> + }
> + return (0);
> }
>
> void
> @@ -840,6 +845,7 @@ refcnt_finalize(struct refcnt *r, const
> struct sleep_state sls;
> u_int refs;
>
> + membar_exit_before_atomic();
> refs = atomic_dec_int_nv(&r->r_refs);
> KASSERT(refs != ~0);
> while (refs) {
> @@ -847,6 +853,8 @@ refcnt_finalize(struct refcnt *r, const
> refs = atomic_load_int(&r->r_refs);
> sleep_finish(&sls, refs);
> }
> + /* Order subsequent loads and stores after refs == 0 load. */
> + membar_sync();
> }
>
> int
