| Message ID | 20161110235714.GR3568@worktop.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2016-11-11 at 00:57 +0100, Peter Zijlstra wrote: > On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote: > > > > On Thu, Nov 10, 2016 at 2:27 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote: > > > > > > > > > > > > > > As it stands kref is a pointless wrapper. If it were to provide > > > > something actually useful, like wrap protection, then it might actually > > > > make sense to use it. > > > > > > It provides the correct cleanup ability for a reference count and the > > > object it is in, so it's not all that pointless :) > > I really fail to see the point of: > > kref_put(&obj->ref, obj_free); > > over: > > if (atomic_dec_and_test(&obj->ref)) > obj_free(obj); > > Utter pointless wrappery afaict. > > > > > > > > > But I'm always willing to change it to make it work better for people, > > > if kref did the wrapping protection (i.e. used a non-wrapping atomic > > > type), then you would have that. I thought that was what this patchset > > > provided... > > So kref could simply do something like the below patch. But this patch > set completely rapes the atomic interface. > > > > > > > > > And yes, this is a horridly large patchset. I've looked at these > > > changes, and in almost all of them, people are using atomic_t as merely > > > a "counter" for something (sequences, rx/tx stats, etc), to get away > > > without having to lock it with an external lock. > > > > > > So, does it make more sense to just provide a "pointless" api for this > > > type of "counter" pattern: > > > counter_inc() > > > counter_dec() > > > counter_read() > > > counter_set() > > > counter_add() > > > counter_subtract() > > > Those would use the wrapping atomic type, as they can wrap all they want > > > and no one really is in trouble. Once those changes are done, just make > > > atomic_t not wrap and all should be fine, no other code should need to > > > be changed. > > Still hate; yes there's a lot of stats which are just fine to wrap. But > there's a lot more atomic out there than refcounts and stats. > > The locking primitives for example use atomic_t, and they really don't > want the extra overhead associated with this overflow crap, or the > namespace pollution. > > > > > reference counters (say, "refcount" implemented with new atomic_nowrap_t) > > > > statistic counters (say, "statcount" implemented with new atomic_wrap_t) > > > > everything else (named "atomic_t", implemented as either > > atomic_nowrap_t or atomic_wrap_t, depending on CONFIG) > > So the problem is that atomic_t has _much_ _much_ more than just add/sub > operations, which are the only ones modified for this patch set. > > The whole wrap/nowrap thing doesn't make any bloody sense what so ever > for !arith operators like bitops or just plain cmpxchg. I wonder if we didn't make a confusion between naming and specifications. I have thought about Kees idea and what you're saying: - The name "atomic_t" name didn't tell anything about if the variable can wrap or not. It just tells there is no race condition on concurrent access, nothing else, and users are well with that. OK then, we don't modify atomic_t, it makes sense. - Hence, let's say a new type "refcount_t". It names exactly what we try to protect in this patch set. A much more simpler interface than atomic_t would be needed, and it protects on race condition and overflows (precisely what is expected of a counter reference). Not an opt-in solution, but it is much less invasive since we "just" have to modify the kref implementation and some vfs reference counters. That didn't tell us how actually implements refcount_t: reuse some atomic_t code or not (it would be simpler anyways, since we don't have to implement the whole atomic_t interface). Still, this is another problem. Sounds better? Thanks, Colin
On Fri, Nov 11, 2016 at 01:29:21AM +0100, Colin Vidal wrote: > On Fri, 2016-11-11 at 00:57 +0100, Peter Zijlstra wrote: > > On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote: > > > On Thu, Nov 10, 2016 at 2:27 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote: > I wonder if we didn't make a confusion between naming and > specifications. I have thought about Kees idea and what you're saying: > > - The name "atomic_t" name didn't tell anything about if the variable > can wrap or not. It just tells there is no race condition on > concurrent access, nothing else, and users are well with that. OK > then, we don't modify atomic_t, it makes sense. > > - Hence, let's say a new type "refcount_t". It names exactly what we > try to protect in this patch set. A much more simpler interface than > atomic_t would be needed, and it protects on race condition and > overflows (precisely what is expected of a counter reference). Not > an opt-in solution, but it is much less invasive since we "just" > have to modify the kref implementation and some vfs reference > counters. > > That didn't tell us how actually implements refcount_t: reuse some > atomic_t code or not (it would be simpler anyways, since we don't have > to implement the whole atomic_t interface). Still, this is another > problem. > > Sounds better? Regardless of atomic_t semantics, a refcount_t would be far more obvious to developers than atomic_t and/or kref, and better documents the intent of code using it. We'd still see abuse of atomic_t (and so this won't solve the problems Kees mentioned), but even as something orthogonal I think that would make sense to have. Thanks, Mark.
On Fri, Nov 11, 2016 at 12:41:27PM +0000, Mark Rutland wrote: > On Fri, Nov 11, 2016 at 01:29:21AM +0100, Colin Vidal wrote: > > I wonder if we didn't make a confusion between naming and > > specifications. I have thought about Kees idea and what you're saying: > > > > - The name "atomic_t" name didn't tell anything about if the variable > > can wrap or not. It just tells there is no race condition on > > concurrent access, nothing else, and users are well with that. OK > > then, we don't modify atomic_t, it makes sense. > > > > - Hence, let's say a new type "refcount_t". It names exactly what we > > try to protect in this patch set. A much more simpler interface than > > atomic_t would be needed, and it protects on race condition and > > overflows (precisely what is expected of a counter reference). Not > > an opt-in solution, but it is much less invasive since we "just" > > have to modify the kref implementation and some vfs reference > > counters. > > > > That didn't tell us how actually implements refcount_t: reuse some > > atomic_t code or not (it would be simpler anyways, since we don't have > > to implement the whole atomic_t interface). Still, this is another > > problem. > > > > Sounds better? > > Regardless of atomic_t semantics, a refcount_t would be far more obvious > to developers than atomic_t and/or kref, and better documents the intent > of code using it. > > We'd still see abuse of atomic_t (and so this won't solve the problems > Kees mentioned), but even as something orthogonal I think that would > make sense to have. Furthermore, you could implement that refcount_t stuff using atomic_cmpxchg() in generic code. While that is sub-optimal for ll/sc architectures you at least get generic code that works to get started. Also, I suspect that if your refcounts are heavily contended, you'll have other problems than the performance of these primitives. Code for refcount_inc(), refcount_inc_not_zero() and refcount_sub_and_test() can be copy-pasted from the kref patch I send yesterday.
On Fri, Nov 11, 2016 at 12:57:14AM +0100, Peter Zijlstra wrote: > On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote: > > On Thu, Nov 10, 2016 at 2:27 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote: > > > >> As it stands kref is a pointless wrapper. If it were to provide > > >> something actually useful, like wrap protection, then it might actually > > >> make sense to use it. > > > > > > It provides the correct cleanup ability for a reference count and the > > > object it is in, so it's not all that pointless :) > > I really fail to see the point of: > > kref_put(&obj->ref, obj_free); > > over: > > if (atomic_dec_and_test(&obj->ref)) > obj_free(obj); > > Utter pointless wrappery afaict. No, it's an abstraction that shows what you are trying to have the driver code do (drop a reference), and is simple to track and verify that the driver is doing stuff properly (automated tools can do it too). It's also easier to review and audit, if I see a developer use a kref, I know how to easily read the code to verify it works correctly. If they use a "raw" atomic, I need to spend more time and effort to review that they got it all correctly (always test the same way, call the correct function the same way, handle the lock that needs to be somewhere involved here, etc.) So it saves both new developers time and effort (how do I write a reference count properly...) and experienced developers (easy to audit and read), which is why we have kref in the first place. I'm all for having it use a wrap-free type, if possible, to catch these types of errors, and yes, you are right in that it would only take 3 functions to implement it correctly. I'd love to have it, but please, don't say that krefs are pointless, they aren't. thanks, greg k-h
diff --git a/include/linux/kref.h b/include/linux/kref.h index e15828fd71f1..6af1f9344793 100644 --- a/include/linux/kref.h +++ b/include/linux/kref.h @@ -39,11 +39,26 @@ static inline void kref_init(struct kref *kref) */ static inline void kref_get(struct kref *kref) { - /* If refcount was 0 before incrementing then we have a race + /* + * If refcount was 0 before incrementing then we have a race * condition when this kref is freeing by some other thread right now. * In this case one should use kref_get_unless_zero() */ - WARN_ON_ONCE(atomic_inc_return(&kref->refcount) < 2); + unsigned int old, new, val = atomic_read(&kref->refcount); + + for (;;) { + WARN_ON_ONCE(val < 1); + + new = val + 1; + if (new < val) + BUG(); /* overflow */ + + old = atomic_cmpxchg_relaxed(&kref->refcount, val, new); + if (old == val) + break; + + val = old; + } } /** @@ -67,9 +82,23 @@ static inline void kref_get(struct kref *kref) static inline int kref_sub(struct kref *kref, unsigned int count, void (*release)(struct kref *kref)) { + unsigned int old, new, val = atomic_read(&kref->refcount); + WARN_ON(release == NULL); - if (atomic_sub_and_test((int) count, &kref->refcount)) { + for (;;) { + new = val - count; + if (new > val) + BUG(); /* underflow */ + + old = atomic_cmpxchg_release(&kref->refcount, val, new); + if (old == val) + break; + + val = old; + } + + if (!new) { release(kref); return 1; } @@ -102,6 +131,7 @@ static inline int kref_put_mutex(struct kref *kref, void (*release)(struct kref *kref), struct mutex *lock) { + /* XXX also fix */ WARN_ON(release == NULL); if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) { mutex_lock(lock); @@ -133,6 +163,23 @@ static inline int kref_put_mutex(struct kref *kref, */ static inline int __must_check kref_get_unless_zero(struct kref *kref) { - return atomic_add_unless(&kref->refcount, 1, 0); + unsigned int old, new, val = atomic_read(&kref->refcount); + + for (;;) { + if (!val) + return 0; + + new = val + 1; + if (new < val) + BUG(); /* overflow */ + + old = atomic_cmpxchg_relaxed(&kref->refcount, val, new); + if (old == val) + break; + + val = old; + } + + return 1; } #endif /* _KREF_H_ */