On Mon, Dec 21, 2020 at 7:09 PM Jonathan Lemon <jonathan.le...@gmail.com> wrote:
>
> From: Jonathan Lemon <b...@fb.com>
>
> Before this change, the caller of sock_zerocopy_callback would
> need to save the zerocopy status, decrement and check the refcount,
> and then call the callback function - the callback was only invoked
> when the refcount reached zero.
>
> Now, the caller just passes the status into the callback function,
> which saves the status and handles its own refcounts.
>
> This makes the behavior of the sock_zerocopy_callback identical
> to the tpacket and vhost callbacks.
>
> Signed-off-by: Jonathan Lemon <jonathan.le...@gmail.com>
> ---
> include/linux/skbuff.h | 3 ---
> net/core/skbuff.c | 14 +++++++++++---
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index fb6dd6af0f82..c9d7de9d624d 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1482,9 +1482,6 @@ static inline void skb_zcopy_clear(struct sk_buff *skb,
> bool zerocopy)
> if (uarg) {
> if (skb_zcopy_is_nouarg(skb)) {
> /* no notification callback */
> - } else if (uarg->callback == sock_zerocopy_callback) {
> - uarg->zerocopy = uarg->zerocopy && zerocopy;
> - sock_zerocopy_put(uarg);
> } else {
> uarg->callback(uarg, zerocopy);
> }
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index ea32b3414ad6..73699dbdc4a1 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1194,7 +1194,7 @@ static bool skb_zerocopy_notify_extend(struct sk_buff
> *skb, u32 lo, u16 len)
> return true;
> }
>
> -void sock_zerocopy_callback(struct ubuf_info *uarg, bool success)
> +static void __sock_zerocopy_callback(struct ubuf_info *uarg)
> {
> struct sk_buff *tail, *skb = skb_from_uarg(uarg);
> struct sock_exterr_skb *serr;
> @@ -1222,7 +1222,7 @@ void sock_zerocopy_callback(struct ubuf_info *uarg,
> bool success)
> serr->ee.ee_origin = SO_EE_ORIGIN_ZEROCOPY;
> serr->ee.ee_data = hi;
> serr->ee.ee_info = lo;
> - if (!success)
> + if (!uarg->zerocopy)
> serr->ee.ee_code |= SO_EE_CODE_ZEROCOPY_COPIED;
>
> q = &sk->sk_error_queue;
> @@ -1241,11 +1241,19 @@ void sock_zerocopy_callback(struct ubuf_info *uarg,
> bool success)
> consume_skb(skb);
> sock_put(sk);
> }
> +
> +void sock_zerocopy_callback(struct ubuf_info *uarg, bool success)
> +{
> + uarg->zerocopy = uarg->zerocopy & success;
> +
> + if (refcount_dec_and_test(&uarg->refcnt))
> + __sock_zerocopy_callback(uarg);
> +}
> EXPORT_SYMBOL_GPL(sock_zerocopy_callback);
I still think this helper is unnecessary. Just return immediately in
existing sock_zerocopy_callback if refcount is not zero.
> void sock_zerocopy_put(struct ubuf_info *uarg)
> {
> - if (uarg && refcount_dec_and_test(&uarg->refcnt))
> + if (uarg)
> uarg->callback(uarg, uarg->zerocopy);
> }
> EXPORT_SYMBOL_GPL(sock_zerocopy_put);
This does increase the number of indirect function calls. Which are
not cheap post spectre.
In the common case for msg_zerocopy we only have two clones, one sent
out and one on the retransmit queue. So I guess the cost will be
acceptable.
