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@xxxxxxxxxxxxx>
---
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);
