On Fri, Mar 11, 2022 at 11:51:31AM +0100, Alexander Bluhm wrote:
> 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.
A skeptical take on memory barriers is that they do not push anything;
they only make certain things happen in a specific order but the exact
time frame remains open. To ensure immediacy, the code needs to use
a read-modify-write atomic operation or something similar that the
processor cannot delay.
For example, on octeon, membar_producer() prevents subsequent writes
from getting reordered with earlier writes in the write buffer.
However, the operation does not block the pipeline, it takes effect
in the background.
> 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.
To make a reasonable API, one should consider the intended usage.
Putting the barriers inside refcnt code keeps the API sane. Otherwise
callers would have to remember issue barriers, which would also be
wasteful on systems where RMW atomics do enforce memory order.
> Not sure about the purpose of cond. Maybe membar_enter/exit is the
> way to go. Does it guatantee anything about memomy access?
The caller of cond_signal() has state that the caller of cond_wait()
wants to see.
cond_signal() should make the (local) state visible to other CPUs
before the clearing of c_wait becomes visible. membar_exit() does
that.
cond_wait() should prevent premature peeking into the data from
happening before the clearing of c_wait has been seen. I think
membar_sync(), and not membar_enter(), is the right choice.
My earlier suggestion about membar_enter() is wrong. This barrier
orders subsequent reads and writes relative to earlier writes. The
pivot in cond_wait() is a read!
> > > 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.
I believe the membars are necessary if f_count is updated using atomic
operations. An alternative is to wrap the reference count updates with
a mutex which invokes membars internally but with increased total cost.
Below is an updated patch that acknowledges the acquire semantics
aspect of FRELE() when reference count has dropped to zero. The
destructor wants to see the (aggregate) state that follows the
f_count 1->0 transition.
Index: kern/kern_descrip.c
===================================================================
RCS file: src/sys/kern/kern_descrip.c,v
retrieving revision 1.205
diff -u -p -r1.205 kern_descrip.c
--- kern/kern_descrip.c 20 Jan 2022 11:06:57 -0000 1.205
+++ kern/kern_descrip.c 11 Mar 2022 14:17:22 -0000
@@ -1268,7 +1268,16 @@ fdrop(struct file *fp, struct proc *p)
{
int error;
- KASSERTMSG(fp->f_count == 0, "count (%u) != 0", fp->f_count);
+ membar_exit_before_atomic();
+ if (atomic_dec_int_nv(&fp->f_count) > 0)
+ return 0;
+
+ /*
+ * Make this CPU see the latest state relative to f_count updates.
+ * Note this memory barrier is redundant because the following
+ * critical section provides an acquire-release barrier.
+ */
+ /* membar_enter_after_atomic(); */
mtx_enter(&fhdlk);
if (fp->f_iflags & FIF_INSERTED)
Index: sys/file.h
===================================================================
RCS file: src/sys/sys/file.h,v
retrieving revision 1.65
diff -u -p -r1.65 file.h
--- sys/file.h 20 Jan 2022 03:43:31 -0000 1.65
+++ sys/file.h 11 Mar 2022 14:17:22 -0000
@@ -113,8 +113,7 @@ struct file {
atomic_inc_int(&(fp)->f_count); \
} while (0)
-#define FRELE(fp,p) \
- (atomic_dec_int_nv(&fp->f_count) == 0 ? fdrop(fp, p) : 0)
+#define FRELE(fp, p) fdrop(fp, p)
#define FDUP_MAX_COUNT (UINT_MAX - 2 * MAXCPUS)
