Re: [PATCH net-next v16 06/26] kref/refcount: implement kref_put_sock()

[Antonio Quartulli]

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,


--
Antonio Quartulli
OpenVPN Inc.