Re: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Sep 04, 2017 at 10:31:54AM +0000, Reshetova, Elena wrote:
> > > But can they make "fast" implementation on ARM that would give stronger
> > > memory guarantees?
> > 
> > Whatever for?
> 
> Well, maybe just by default when arch.-specific implementation is
> done. But I was just trying to speculate to understand. I will resend
> this one with new comment added. 

So the generic lib/refcount.c already has weak ordering. It doesn't make
sense for an arch specific implementation (on a weakly ordered machine)
to provide stronger guarantees (it would make things slower).

The weaker ordering of the refcount_t primitives is sufficient if we're
talking pure refcounts. If for some reason code relies on stronger
ordering there _SHOULD_ be a comment with describing the additional
ordering requirements.

But that's a fairly big 'should'. I can well imagine the comment not
being there. In fact, see below.

> Still not sure if I need to resend the whole series with updated
> commits or break this up by individual patches further for the
> separate merges. 

I've yet to look at the ones targeted at subsystems I do, I'm forever
and terminally behind on review :/

I called out the issue on futex in particular because it is fairly
tricky code that.

Now Thomas would like you to mention the fact that refcount_t doesn't
provide the exact same ordering as the atomic_t usages it replaces and
I think it would be good if you could hand-wave an argument on why the
futex code doesn't care.


Now, suppose we were to convert i_count to refcount_t (yes, I know, my
initial conversion wasn't well received), then we need to add
futex_get_inode() similar to futex_get_mm().

That is, smp_mb__{before,after}_atomic() works as expected and can be
used to fortify the implied barriers by refcount_t.

---
Subject: fs,inode: Add comment explaining additional ordering

Add a note to ihold() to document the ordering futex relies upon.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 fs/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/inode.c b/fs/inode.c
index 50370599e371..17192ba92fef 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -395,6 +395,10 @@ void __iget(struct inode *inode)
  */
 void ihold(struct inode *inode)
 {
+	/*
+	 * Note: futex.c:get_futex_key_refs() relies on this function
+	 * implying an smp_mb().
+	 */
 	WARN_ON(atomic_inc_return(&inode->i_count) < 2);
 }
 EXPORT_SYMBOL(ihold);