lkml.org 
[lkml]   [2017]   [Sep]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 14/15] futex: convert futex_pi_state.refcount to refcount_t
    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);
    \
     
     \ /
      Last update: 2017-09-04 14:01    [W:3.913 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site