| Message ID | 20241226170710.1159679-12-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | move per-vma lock into vm_area_struct | expand |
On 12/26/24 18:07, Suren Baghdasaryan wrote: > Introduce functions to increase refcount but with a top limit above > which they will fail to increase. Setting the limit to 0 indicates > no limit. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > include/linux/refcount.h | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/include/linux/refcount.h b/include/linux/refcount.h > index 35f039ecb272..e51a49179307 100644 > --- a/include/linux/refcount.h > +++ b/include/linux/refcount.h > @@ -137,13 +137,19 @@ static inline unsigned int refcount_read(const refcount_t *r) > } > > static inline __must_check __signed_wrap > -bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) > +bool __refcount_add_not_zero_limited(int i, refcount_t *r, int *oldp, > + int limit) > { > int old = refcount_read(r); > > do { > if (!old) > break; > + if (limit && old + i > limit) { Should this be e.g. "old > limit - i" to avoid overflow and false negative if someone sets limit close to INT_MAX? > + if (oldp) > + *oldp = old; > + return false; > + } > } while (!atomic_try_cmpxchg_relaxed(&r->refs, &old, old + i)); > > if (oldp) > @@ -155,6 +161,12 @@ bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) > return old; > } > > +static inline __must_check __signed_wrap > +bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) > +{ > + return __refcount_add_not_zero_limited(i, r, oldp, 0); > +} > + > /** > * refcount_add_not_zero - add a value to a refcount unless it is 0 > * @i: the value to add to the refcount > @@ -213,6 +225,12 @@ static inline void refcount_add(int i, refcount_t *r) > __refcount_add(i, r, NULL); > } > > +static inline __must_check bool __refcount_inc_not_zero_limited(refcount_t *r, > + int *oldp, int limit) > +{ > + return __refcount_add_not_zero_limited(1, r, oldp, limit); > +} > + > static inline __must_check bool __refcount_inc_not_zero(refcount_t *r, int *oldp) > { > return __refcount_add_not_zero(1, r, oldp);
On Wed, Jan 08, 2025 at 10:16:04AM +0100, Vlastimil Babka wrote: > > static inline __must_check __signed_wrap > > -bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) > > +bool __refcount_add_not_zero_limited(int i, refcount_t *r, int *oldp, > > + int limit) > > { > > int old = refcount_read(r); > > > > do { > > if (!old) > > break; > > + if (limit && old + i > limit) { > > Should this be e.g. "old > limit - i" to avoid overflow and false negative > if someone sets limit close to INT_MAX? Although 'i' might also be INT_MAX, whereas we know that old < limit. So "i > limit - old" is the correct condition to check, IMO. I'd further suggest that using a limit of 0 to mean "unlimited" introduces an unnecessary arithmetic operation. Make 'limit' inclusive instead of exclusive, pass INT_MAX instead of 0, and Vlastimil's suggestion, and this becomes: if (i > limit - old) > > + if (oldp) > > + *oldp = old; > > + return false; > > + } > > } while (!atomic_try_cmpxchg_relaxed(&r->refs, &old, old + i)); ... > > +static inline __must_check __signed_wrap > > +bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) > > +{ > > + return __refcount_add_not_zero_limited(i, r, oldp, 0); Just to be clear, this becomes: return __refcount_add_not_zero_limited(i, r, oldp, INT_MAX);
On Wed, Jan 8, 2025 at 7:06 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Jan 08, 2025 at 10:16:04AM +0100, Vlastimil Babka wrote: > > > static inline __must_check __signed_wrap > > > -bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) > > > +bool __refcount_add_not_zero_limited(int i, refcount_t *r, int *oldp, > > > + int limit) > > > { > > > int old = refcount_read(r); > > > > > > do { > > > if (!old) > > > break; > > > + if (limit && old + i > limit) { > > > > Should this be e.g. "old > limit - i" to avoid overflow and false negative > > if someone sets limit close to INT_MAX? > > Although 'i' might also be INT_MAX, whereas we know that old < limit. > So "i > limit - old" is the correct condition to check, IMO. > > I'd further suggest that using a limit of 0 to mean "unlimited" introduces > an unnecessary arithmetic operation. Make 'limit' inclusive instead > of exclusive, pass INT_MAX instead of 0, and Vlastimil's suggestion, > and this becomes: > > if (i > limit - old) Thanks for the suggestions, Vlastimil and Matthew! Yes, this looks much better. Will use it in the next version. > > > > + if (oldp) > > > + *oldp = old; > > > + return false; > > > + } > > > } while (!atomic_try_cmpxchg_relaxed(&r->refs, &old, old + i)); > > ... > > > > +static inline __must_check __signed_wrap > > > +bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) > > > +{ > > > + return __refcount_add_not_zero_limited(i, r, oldp, 0); > > Just to be clear, this becomes: > > return __refcount_add_not_zero_limited(i, r, oldp, INT_MAX); Ack. >
On Wed, 8 Jan 2025 15:06:17 +0000 Matthew Wilcox <willy@infradead.org> wrote: > On Wed, Jan 08, 2025 at 10:16:04AM +0100, Vlastimil Babka wrote: > > > static inline __must_check __signed_wrap > > > -bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) > > > +bool __refcount_add_not_zero_limited(int i, refcount_t *r, int *oldp, > > > + int limit) > > > { > > > int old = refcount_read(r); > > > > > > do { > > > if (!old) > > > break; > > > + if (limit && old + i > limit) { > > > > Should this be e.g. "old > limit - i" to avoid overflow and false negative > > if someone sets limit close to INT_MAX? > > Although 'i' might also be INT_MAX, whereas we know that old < limit. > So "i > limit - old" is the correct condition to check, IMO. > > I'd further suggest that using a limit of 0 to mean "unlimited" introduces > an unnecessary arithmetic operation. Make 'limit' inclusive instead > of exclusive, pass INT_MAX instead of 0, and Vlastimil's suggestion, > and this becomes: > > if (i > limit - old) > ... The problem with that is the compiler is unlikely to optimise it away. Perhaps: if (statically_true(!limit || limit == INT_MAX)) continue; if (i > limit - old) { ... David
On Fri, Jan 10, 2025 at 5:32 AM David Laight <david.laight.linux@gmail.com> wrote: > > On Wed, 8 Jan 2025 15:06:17 +0000 > Matthew Wilcox <willy@infradead.org> wrote: > > > On Wed, Jan 08, 2025 at 10:16:04AM +0100, Vlastimil Babka wrote: > > > > static inline __must_check __signed_wrap > > > > -bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) > > > > +bool __refcount_add_not_zero_limited(int i, refcount_t *r, int *oldp, > > > > + int limit) > > > > { > > > > int old = refcount_read(r); > > > > > > > > do { > > > > if (!old) > > > > break; > > > > + if (limit && old + i > limit) { > > > > > > Should this be e.g. "old > limit - i" to avoid overflow and false negative > > > if someone sets limit close to INT_MAX? > > > > Although 'i' might also be INT_MAX, whereas we know that old < limit. > > So "i > limit - old" is the correct condition to check, IMO. > > > > I'd further suggest that using a limit of 0 to mean "unlimited" introduces > > an unnecessary arithmetic operation. Make 'limit' inclusive instead > > of exclusive, pass INT_MAX instead of 0, and Vlastimil's suggestion, > > and this becomes: > > > > if (i > limit - old) > > > ... > > The problem with that is the compiler is unlikely to optimise it away. > Perhaps: > if (statically_true(!limit || limit == INT_MAX)) > continue; > if (i > limit - old) { > ... Thanks for the comment! I think it makes sense. For the reference, the new version of this patch is here: https://lore.kernel.org/all/20250109023025.2242447-11-surenb@google.com/ If I apply your suggestion to that version it should look like this: +bool __refcount_add_not_zero_limited(int i, refcount_t *r, int *oldp, + int limit) { int old = refcount_read(r); do { if (!old) break; + + if (statically_true(limit == INT_MAX)) + continue; + + if (i > limit - old) { + if (oldp) + *oldp = old; + return false; + } } while (!atomic_try_cmpxchg_relaxed(&r->refs, &old, old + i)); I'll update the patch with this and let's see if everyone agrees. > > David > >
diff --git a/include/linux/refcount.h b/include/linux/refcount.h index 35f039ecb272..e51a49179307 100644 --- a/include/linux/refcount.h +++ b/include/linux/refcount.h @@ -137,13 +137,19 @@ static inline unsigned int refcount_read(const refcount_t *r) } static inline __must_check __signed_wrap -bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) +bool __refcount_add_not_zero_limited(int i, refcount_t *r, int *oldp, + int limit) { int old = refcount_read(r); do { if (!old) break; + if (limit && old + i > limit) { + if (oldp) + *oldp = old; + return false; + } } while (!atomic_try_cmpxchg_relaxed(&r->refs, &old, old + i)); if (oldp) @@ -155,6 +161,12 @@ bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) return old; } +static inline __must_check __signed_wrap +bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp) +{ + return __refcount_add_not_zero_limited(i, r, oldp, 0); +} + /** * refcount_add_not_zero - add a value to a refcount unless it is 0 * @i: the value to add to the refcount @@ -213,6 +225,12 @@ static inline void refcount_add(int i, refcount_t *r) __refcount_add(i, r, NULL); } +static inline __must_check bool __refcount_inc_not_zero_limited(refcount_t *r, + int *oldp, int limit) +{ + return __refcount_add_not_zero_limited(1, r, oldp, limit); +} + static inline __must_check bool __refcount_inc_not_zero(refcount_t *r, int *oldp) { return __refcount_add_not_zero(1, r, oldp);
Introduce functions to increase refcount but with a top limit above which they will fail to increase. Setting the limit to 0 indicates no limit. Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- include/linux/refcount.h | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)