On Fri, Mar 11, 2022 at 05:32:11AM +0000, Visa Hankala wrote:
> 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.
In general if you want to move data from one CPU to another, you
have to push them out and pull them in. That is where the symetry
comes from and it should be reflected in code. At least that is
my understanding, but understanding the whole topic is hard.
As kettenis mentioned atomic_membar does not fit atomic_load/store
in amd64. So membar_enter/exit would be better.
I came to the conclusion that refcnt does not need membars at all.
It does not protect other data, it is only looking at one counter
variable. When using refcnt, it protects the livetime of an object.
For the data you need another lock which brings its own barriers.
Not sure about the purpose of cond. Maybe membar_enter/exit is the
way to go. Does it guatantee anything about memomy access?
> > 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.
Let's keep FRELE out of the discussion. Either it works as it is
or it should use suitable primitives. But please no membars in the
file system code.
bluhm
>
> > 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.
