MIME-Version: 1.0
In-Reply-To: <20170324172342.radlrhk2z6mwmdgk@hirez.programming.kicks-ass.net>
References: <CACT4Y+bG+a0w6j6v1AmBE7fqqMSPyPEm4QimCzCouicmHT8FqA@mail.gmail.com>
 <20170324142140.vpyzl755oj6rb5qv@hirez.programming.kicks-ass.net>
 <20170324164108.ibcxxqbhvx6ao54r@hirez.programming.kicks-ass.net>
 <CALCETrVFOKC91hKWxm+-w2v2FPGJ37RoCOYX5FYpszjMrAaUOQ@mail.gmail.com> <20170324172342.radlrhk2z6mwmdgk@hirez.programming.kicks-ass.net>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Fri, 24 Mar 2017 12:08:32 -0700
Message-ID: <CA+55aFzxsGUesnq7mqyas=8ZK5fvPc0Zs_=OO3ttZ6Cb7Fbz-A@mail.gmail.com>
Subject: Re: locking/atomic: Introduce atomic_try_cmpxchg()
To: Peter Zijlstra <peterz@infradead.org>
Cc: Andy Lutomirski <luto@amacapital.net>,
        Dmitry Vyukov <dvyukov@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andy Lutomirski <luto@kernel.org>, Borislav Petkov <bp@alien8.de>,
        Brian Gerst <brgerst@gmail.com>, Denys Vlasenko <dvlasenk@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Josh Poimboeuf <jpoimboe@redhat.com>,
        Paul McKenney <paulmck@linux.vnet.ibm.com>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2569
Lines: 73

On Fri, Mar 24, 2017 at 10:23 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> I tried a few variants, but nothing really made it better.

So I really hate how your thing has two return values, and fakes the
second one using the pointer value. I dislike it for two different
reasons:

 - it's bad for type checking: it makes the "real" pointer value and
the "old value" pointer value both be pointers of the same type

 - I think it's part of the reason why gcc easily generates crap code.

And I think the problem is fundamental to your interface.

So how about we change the interface entirely, with the goal being
both type safety and good code generation?

In particular, let's make the interface something that might be
optimal given possible future gcc improvements, like the ability to
use "asm goto" with outputs. We can't do that today, but maybe some
day...

So I would suggest that the format of the "try_cmpxhg()" thing be
something like this:

#define try_cmpxchg(ptr, value, new, success_label) ({  \
        bool __txchg_success;                           \
        __typeof__(*(ptr)) __old;                       \
        asm volatile("lock cmpxchgl %3, %1"             \
                : "=@ccz" (__txchg_success),            \
                  "+m" (*ptr),                          \
                  "=a" (__old)                          \
                : "r" (new),                            \
                  "2" (value)                           \
                : "memory");                            \
        if (__txchg_success) goto success_label;        \
        __old; })

which seems to be fairly natural.

Then you do your refcount loop (or pretty much *any* cmpxchg loop) with

        for (;;) {
                if (unlikely(val == UINT_MAX))
                        goto saturated;

                if (unlikely(!val))
                        goto use_after_free;

                new = val + 1;
                val = try_cmpxchg(r, val, new, success);
        }

    success:
        return;

    saturated: use_after_free: .. whatever error handling ..

and I think gcc should generate reasonable code.

Hmm?

NOTE! I would suggest that this odd "macro with a success label" model
of try_cmpxchg() never be used for something that people are expected
to use directly. So I don't think "normal" code should use this label
form.

But it's useful as a helper function that is then used to implement
the "real" ABI, ie to implement that "refcount_inc()" and other things
like that..

                               Linus