Hi Will,
Thanks a lot for chiming in and sorry for the delay. See below.
On 19/12/24 18:20, Will Deacon wrote: [...]
+/**
- refcount_dec_and_lock_sock - return holding locked sock if able to decrement
refcount to 0
- @r: the refcount
- @sock: the sock to be locked
- Similar to atomic_dec_and_lock(), it will WARN on underflow and fail to
- decrement when saturated at REFCOUNT_SATURATED.
- Provides release memory ordering, such that prior loads and stores are done
- before, and provides a control dependency such that free() must come after.
- See the comment on top.
- Return: true and hold sock if able to decrement refcount to 0, false
otherwise- */
+bool refcount_dec_and_lock_sock(refcount_t *r, struct sock *sock) +{
- if (refcount_dec_not_one(r))
return false;- bh_lock_sock(sock);
- if (!refcount_dec_and_test(r)) {
bh_unlock_sock(sock);return false;- }
- return true;
+} +EXPORT_SYMBOL(refcount_dec_and_lock_sock);
It feels a little out-of-place to me having socket-specific functions in lib/refcount.c. I'd suggest sticking this somewhere else _or_ maybe we could generate this pattern of code:
#define REFCOUNT_DEC_AND_LOCKNAME(lockname, locktype, lock, unlock) \ static __always_inline \ bool refcount_dec_and_lock_##lockname(refcount_t *r, locktype *l) \ { \ ...
inside a generator macro in refcount.h, like we do for seqlocks in linux/seqlock.h. The downside of that is the cost of inlining.
Does your suggestion imply that I should convert already existing functions to this macro? In that case I believe the change would be too invasive and other devs may not like the inlining, as you pointed out.
Secondly, I thought about moving this function to net/core/sock.c, but if you look at it, its logic is mostly about refcounting, with a little touch of sock. Hence, sock.c (or any other net file) does not seem appropriate either.
I guess for the time being it is more convenient to keep this function, and is kref counterpart, inside 'ovpn'. They can be moved to better spots once they gained another user.
Best Regards,