Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 24 Mar 2017 12:08:32 -0700 | Subject | Re: locking/atomic: Introduce atomic_try_cmpxchg() |
| |
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
| |