Messages in this thread | | | Date | Mon, 13 Dec 2021 18:29:27 +0100 | From | Marco Elver <> | Subject | Re: [PATCH v2 8/9] atomic,x86: Alternative atomic_*_overflow() scheme |
| |
On Mon, Dec 13, 2021 at 05:43PM +0100, Peter Zijlstra wrote: > On Fri, Dec 10, 2021 at 05:16:26PM +0100, Peter Zijlstra wrote: > > Shift the overflow range from [0,INT_MIN] to [-1,INT_MIN], this allows > > optimizing atomic_inc_overflow() to use "jle" to detect increment > > from free-or-negative (with -1 being the new free and it's increment > > being 0 which sets ZF). > > > > This then obviously changes atomic_dec*_overflow() since it must now > > detect the 0->-1 transition rather than the 1->0. Luckily this is > > reflected in the carry flag (since we need to borrow to decrement 0). > > However this means decrement must now use the SUB instruction with a > > literal, since DEC doesn't set CF. > > > > This then gives the following primitives: > > > > [-1, INT_MIN] [0, INT_MIN] > > > > inc() inc() > > lock inc %[var] mov $-1, %[reg] > > jle error-free-or-negative lock xadd %[reg], %[var] > > test %[reg], %[reg] > > jle error-zero-or-negative > > > > dec() dec() > > lock sub $1, %[var] lock dec %[var] > > jc error-to-free jle error-zero-or-negative > > jl error-from-negative > > > > dec_and_test() dec_and_test() > > lock sub $1, %[var] lock dec %[var] > > jc do-free jl error-from-negative > > jl error-from-negative je do-free > > > > Make sure to set ATOMIC_OVERFLOW_OFFSET to 1 such that other code > > interacting with these primitives can re-center 0. > > So Marco was expressing doubt about this exact interface for the > atomic_*_overflow() functions, since it's extremely easy to get the > whole ATOMIC_OVERFLOW_OFFSET thing wrong. > > Since the current ops are strictly those that require inline asm, the > interface is fairly incomplete, which forces anybody who's going to use > these to provide whatever is missing. eg. atomic_inc_not_zero_overflow() > for example. > > Another proposal had the user supply the offset as a compile time > constant to the function itself, raising a build-bug for any unsupported > offset. This would ensure the caller is at least aware of any non-zero > offset... still not going to really be dummy proof either.
In the spirit of making the interface harder to misuse, this would at least ensure that non-refcount_t code that wants to use atomic_*overflow() is 100% aware of this. Which is half of the issue I think.
The other half is code using the actual values, and ensuring it's offset correctly. This might also be an issue in e.g. refcount_t, if someone wants to modify or extend it, although it's easy enough to audit and review in such central data structures as refcount_t.
> Alternatively we could provide a more complete set of ops and/or a whole > new type, but... I'm not sure about that either. > > I suppose I can try and do something like refcount_overflow_t and > implement the whole current refcount API in terms of that. Basically > everywhere we currently do refcount_warn_saturate() would become goto > label. > > And then refcount_t could be a thin wrapper on top of that. But urgh... > lots of work, very little gain. > > So what do we do? Keep things as is, and think about it again once we > got the first bug in hand, preemptively add a few ops or go completely > overboard? > > Obviously I'm all for keeping things as is (less work for this lazy > bastard etc..)
I think an entirely new type might be overkill, but at the very least designing the interface such that it's
A. either impossible to not notice the fact atomic_*overflow() works in terms of offsets, or B. not even exposing this detail.
#A can be achieved with supplying offsets to atomic_*overflow(). #B can be achieved with new wrapper types -- however, if we somehow ensure that refcount_t remains the only user of atomic_*overflow(), I'd consider refcount_t a wrapper type already, so no need to add more.
Regarding the interface, it'd be nice if it could be made harder to misuse, but I don't know how much it'll buy over what it is right now, since we don't even know if there'll be other users of this yet.
But here are some more issues I just thought of:
1. A minor issue is inspecting raw values, like in register dumps. refcount_t will now look different on x86 vs. other architectures.
2. Yet another potentially larger issue is if some code kmalloc()s some structs containing refcount_t, and relies on GFP_ZERO (kzalloc()) to initialize their data assuming that a freshly initialized refcount_t contains 0.
I think #1 is a cosmetic issue, which we might be able to live with.
However, I have absolutely no idea how we can audit or even prevent #2 from happening. With #2 in mind, and with C's lack of enforcing any kind of "constructors", the interface and implementation we end up with is going to result in near-impossible to debug issues sooner or later.
Thanks, -- Marco
| |