On Thu, Mar 10, 2022 at 07:17:43PM +0100, Alexander Bluhm wrote:
> On Thu, Mar 10, 2022 at 04:39:49PM +0100, Alexander Bluhm wrote:
> > > Below is a patch that shows how to accomplish release semantics with
> > > the file and refcnt APIs. (The added memory barriers ensure that the
> > > CPU completes its loads and stores to the object before dropping the
> > > reference. Another CPU might delete the object immediately after.
> > > The barrier might not be strictly necessary with refcnt_finalize(),
> > > though.)
>
> The enter and exit membars that protect the critical section should
> be symmetric. Maybe this diff is better.
No, symmetry is not always present. See below.
> And to avoid memory barriers the nobody understands we should convert
> FRELE to refcnt_rele.
I am not sure about that. I think as long as file reference counting
does unusual things that refcnt does not implement, f_count handling
should be separate.
> Index: kern/kern_synch.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_synch.c,v
> retrieving revision 1.183
> diff -u -p -r1.183 kern_synch.c
> --- kern/kern_synch.c 10 Mar 2022 15:21:08 -0000 1.183
> +++ kern/kern_synch.c 10 Mar 2022 18:11:39 -0000
> @@ -805,6 +805,7 @@ void
> refcnt_init(struct refcnt *r)
> {
> atomic_store_int(&r->r_refs, 1);
> + membar_enter_after_atomic();
> }
I think membar is unnecessary here. Concurrent access can only happen
after the object has been "published", and the publishing should have
the appropriate memory barriers.
Without proper publishing, the code is prone to race conditions.
Also note that membar_enter_after_atomic() is not valid with
atomic_store_* or atomic_load_*. See my comment about cond_signal().
>
> void
> @@ -818,6 +819,7 @@ refcnt_take(struct refcnt *r)
> #else
> atomic_inc_int(&r->r_refs);
> #endif
> + membar_enter_after_atomic();
> }
This is unnecessary. The caller already has a reference to the object.
refcnt_take() only has the intent of increasing the reference count.
>
> int
> @@ -825,6 +827,7 @@ refcnt_rele(struct refcnt *r)
> {
> u_int refcnt;
>
> + membar_exit_before_atomic();
> refcnt = atomic_dec_int_nv(&r->r_refs);
> KASSERT(refcnt != ~0);
>
> @@ -844,6 +847,7 @@ refcnt_finalize(struct refcnt *r, const
> struct sleep_state sls;
> u_int refcnt;
>
> + membar_exit_before_atomic();
> refcnt = atomic_dec_int_nv(&r->r_refs);
> while (refcnt) {
> sleep_setup(&sls, r, PWAIT, wmesg, 0);
> @@ -856,11 +860,13 @@ void
> cond_init(struct cond *c)
> {
> atomic_store_int(&c->c_wait, 1);
> + membar_enter_after_atomic();
> }
Same point here as with refcnt_init().
>
> void
> cond_signal(struct cond *c)
> {
> + membar_exit_before_atomic();
> atomic_store_int(&c->c_wait, 0);
This should use membar_exit(). membar_exit_before_atomic() is valid
only when accompanied with a true read-modify-write atomic operation.
The atomic_ prefix with the store and load instructions confuses this
somewhat.
The wakeup call that follows provides a membar function, but it comes
too late as c_wait has already been cleared.
>
> wakeup_one(c);
> @@ -872,9 +878,11 @@ cond_wait(struct cond *c, const char *wm
> struct sleep_state sls;
> unsigned int wait;
>
> + membar_exit_before_atomic();
> wait = atomic_load_int(&c->c_wait);
> while (wait) {
> sleep_setup(&sls, c, PWAIT, wmesg, 0);
> + membar_exit_before_atomic();
> wait = atomic_load_int(&c->c_wait);
> sleep_finish(&sls, wait);
> }
>
I think this should use membar_enter() after the loop.
cond_wait() is supposed to provide acquire semantics; once cond_wait()
returns, the CPU sees a state that is at least as recent as the one
that the caller of cond_signal() saw.
In a way, cond_wait() is similar to lock acquisition, and cond_signal()
similar to lock release.
