udp: elide zerocopy operation in hot path

With MSG_ZEROCOPY, each skb holds a reference to a struct ubuf_info.
Release of its last reference triggers a completion notification.

The TCP stack in tcp_sendmsg_locked holds an extra ref independent of
the skbs, because it can build, send and free skbs within its loop,
possibly reaching refcount zero and freeing the ubuf_info too soon.

The UDP stack currently also takes this extra ref, but does not need
it as all skbs are sent after return from __ip(6)_append_data.

Avoid the extra refcount_inc and refcount_dec_and_test, and generally
the sock_zerocopy_put in the common path, by passing the initial
reference to the first skb.

This approach is taken instead of initializing the refcount to 0, as
that would generate error "refcount_t: increment on 0" on the
next skb_zcopy_set.

Changes
  v3 -> v4
    - Move skb_zcopy_set below the only kfree_skb that might cause
      a premature uarg destroy before skb_zerocopy_put_abort
      - Move the entire skb_shinfo assignment block, to keep that
        cacheline access in one place

Signed-off-by: Willem de Bruijn <willemb@google.com>
Acked-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 04f52e7195714080f3be60bad053c90c96c18bb7..75d50ab7997cdabba047dff05eb0297db651f369 100644 (file)
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -481,7 +481,7 @@ static inline void sock_zerocopy_get(struct ubuf_info *uarg)
 }
 
 void sock_zerocopy_put(struct ubuf_info *uarg);
-void sock_zerocopy_put_abort(struct ubuf_info *uarg);
+void sock_zerocopy_put_abort(struct ubuf_info *uarg, bool have_uref);
 
 void sock_zerocopy_callback(struct ubuf_info *uarg, bool success);
 
@@ -1326,10 +1326,14 @@ static inline struct ubuf_info *skb_zcopy(struct sk_buff *skb)
        return is_zcopy ? skb_uarg(skb) : NULL;
 }
 
-static inline void skb_zcopy_set(struct sk_buff *skb, struct ubuf_info *uarg)
+static inline void skb_zcopy_set(struct sk_buff *skb, struct ubuf_info *uarg,
+                                bool *have_ref)
 {
        if (skb && uarg && !skb_zcopy(skb)) {
-               sock_zerocopy_get(uarg);
+               if (unlikely(have_ref && *have_ref))
+                       *have_ref = false;
+               else
+                       sock_zerocopy_get(uarg);
                skb_shinfo(skb)->destructor_arg = uarg;
                skb_shinfo(skb)->tx_flags |= SKBTX_ZEROCOPY_FRAG;
        }
@@ -1374,7 +1378,7 @@ static inline void skb_zcopy_abort(struct sk_buff *skb)
        struct ubuf_info *uarg = skb_zcopy(skb);
 
        if (uarg) {
-               sock_zerocopy_put_abort(uarg);
+               sock_zerocopy_put_abort(uarg, false);
                skb_shinfo(skb)->tx_flags &= ~SKBTX_ZEROCOPY_FRAG;
        }
 }

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1350901c5cb828bf4f019f3d25337acdbe7b5ba6..c78ce114537e097d532eb112d8c4eb86c724f6ea 100644 (file)
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1089,7 +1089,7 @@ void sock_zerocopy_put(struct ubuf_info *uarg)
 }
 EXPORT_SYMBOL_GPL(sock_zerocopy_put);
 
-void sock_zerocopy_put_abort(struct ubuf_info *uarg)
+void sock_zerocopy_put_abort(struct ubuf_info *uarg, bool have_uref)
 {
        if (uarg) {
                struct sock *sk = skb_from_uarg(uarg)->sk;
@@ -1097,7 +1097,8 @@ void sock_zerocopy_put_abort(struct ubuf_info *uarg)
                atomic_dec(&sk->sk_zckey);
                uarg->len--;
 
-               sock_zerocopy_put(uarg);
+               if (have_uref)
+                       sock_zerocopy_put(uarg);
        }
 }
 EXPORT_SYMBOL_GPL(sock_zerocopy_put_abort);
@@ -1137,7 +1138,7 @@ int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb,
                return err;
        }
 
-       skb_zcopy_set(skb, uarg);
+       skb_zcopy_set(skb, uarg, NULL);
        return skb->len - orig_len;
 }
 EXPORT_SYMBOL_GPL(skb_zerocopy_iter_stream);
@@ -1157,7 +1158,7 @@ static int skb_zerocopy_clone(struct sk_buff *nskb, struct sk_buff *orig,
                        if (skb_copy_ubufs(nskb, GFP_ATOMIC))
                                return -EIO;
                }
-               skb_zcopy_set(nskb, skb_uarg(orig));
+               skb_zcopy_set(nskb, skb_uarg(orig), NULL);
        }
        return 0;
 }

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 6f843aff628c0dbfe7bb577413fc4cd1766223f5..78f028bdad30b4c6f2d89a9488f2bd79c79f672c 100644 (file)
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -881,8 +881,8 @@ static int __ip_append_data(struct sock *sk,
        int csummode = CHECKSUM_NONE;
        struct rtable *rt = (struct rtable *)cork->dst;
        unsigned int wmem_alloc_delta = 0;
+       bool paged, extra_uref;
        u32 tskey = 0;
-       bool paged;
 
        skb = skb_peek_tail(queue);
 
@@ -921,12 +921,13 @@ static int __ip_append_data(struct sock *sk,
                uarg = sock_zerocopy_realloc(sk, length, skb_zcopy(skb));
                if (!uarg)
                        return -ENOBUFS;
+               extra_uref = true;
                if (rt->dst.dev->features & NETIF_F_SG &&
                    csummode == CHECKSUM_PARTIAL) {
                        paged = true;
                } else {
                        uarg->zerocopy = 0;
-                       skb_zcopy_set(skb, uarg);
+                       skb_zcopy_set(skb, uarg, &extra_uref);
                }
        }
 
@@ -1015,13 +1016,6 @@ alloc_new_skb:
                        skb->csum = 0;
                        skb_reserve(skb, hh_len);
 
-                       /* only the initial fragment is time stamped */
-                       skb_shinfo(skb)->tx_flags = cork->tx_flags;
-                       cork->tx_flags = 0;
-                       skb_shinfo(skb)->tskey = tskey;
-                       tskey = 0;
-                       skb_zcopy_set(skb, uarg);
-
                        /*
                         *      Find where to start putting bytes.
                         */
@@ -1054,6 +1048,13 @@ alloc_new_skb:
                        exthdrlen = 0;
                        csummode = CHECKSUM_NONE;
 
+                       /* only the initial fragment is time stamped */
+                       skb_shinfo(skb)->tx_flags = cork->tx_flags;
+                       cork->tx_flags = 0;
+                       skb_shinfo(skb)->tskey = tskey;
+                       tskey = 0;
+                       skb_zcopy_set(skb, uarg, &extra_uref);
+
                        if ((flags & MSG_CONFIRM) && !skb_prev)
                                skb_set_dst_pending_confirm(skb, 1);
 
@@ -1124,13 +1125,12 @@ alloc_new_skb:
 
        if (wmem_alloc_delta)
                refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
-       sock_zerocopy_put(uarg);
        return 0;
 
 error_efault:
        err = -EFAULT;
 error:
-       sock_zerocopy_put_abort(uarg);
+       sock_zerocopy_put_abort(uarg, extra_uref);
        cork->length -= length;
        IP_INC_STATS(sock_net(sk), IPSTATS_MIB_OUTDISCARDS);
        refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 215e4d3b361657c434ff019ebe638437b65dd351..dc68c408bba09af3273882a1c7ffae1ec03ba1d3 100644 (file)
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1423,7 +1423,7 @@ do_error:
        if (copied + copied_syn)
                goto out;
 out_err:
-       sock_zerocopy_put_abort(uarg);
+       sock_zerocopy_put_abort(uarg, true);
        err = sk_stream_error(sk, flags, err);
        /* make sure we wake any epoll edge trigger waiter */
        if (unlikely(skb_queue_len(&sk->sk_write_queue) == 0 &&

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 7df04d20a91f766fdafb8da19fc7f232a225c7d6..ec8c235ea891a7dfdc978e61fa71aee0d5abcd17 100644 (file)
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1258,7 +1258,7 @@ static int __ip6_append_data(struct sock *sk,
        int csummode = CHECKSUM_NONE;
        unsigned int maxnonfragsize, headersize;
        unsigned int wmem_alloc_delta = 0;
-       bool paged;
+       bool paged, extra_uref;
 
        skb = skb_peek_tail(queue);
        if (!skb) {
@@ -1327,12 +1327,13 @@ emsgsize:
                uarg = sock_zerocopy_realloc(sk, length, skb_zcopy(skb));
                if (!uarg)
                        return -ENOBUFS;
+               extra_uref = true;
                if (rt->dst.dev->features & NETIF_F_SG &&
                    csummode == CHECKSUM_PARTIAL) {
                        paged = true;
                } else {
                        uarg->zerocopy = 0;
-                       skb_zcopy_set(skb, uarg);
+                       skb_zcopy_set(skb, uarg, &extra_uref);
                }
        }
 
@@ -1454,13 +1455,6 @@ alloc_new_skb:
                        skb_reserve(skb, hh_len + sizeof(struct frag_hdr) +
                                    dst_exthdrlen);
 
-                       /* Only the initial fragment is time stamped */
-                       skb_shinfo(skb)->tx_flags = cork->tx_flags;
-                       cork->tx_flags = 0;
-                       skb_shinfo(skb)->tskey = tskey;
-                       tskey = 0;
-                       skb_zcopy_set(skb, uarg);
-
                        /*
                         *      Find where to start putting bytes
                         */
@@ -1492,6 +1486,13 @@ alloc_new_skb:
                        exthdrlen = 0;
                        dst_exthdrlen = 0;
 
+                       /* Only the initial fragment is time stamped */
+                       skb_shinfo(skb)->tx_flags = cork->tx_flags;
+                       cork->tx_flags = 0;
+                       skb_shinfo(skb)->tskey = tskey;
+                       tskey = 0;
+                       skb_zcopy_set(skb, uarg, &extra_uref);
+
                        if ((flags & MSG_CONFIRM) && !skb_prev)
                                skb_set_dst_pending_confirm(skb, 1);
 
@@ -1562,13 +1563,12 @@ alloc_new_skb:
 
        if (wmem_alloc_delta)
                refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
-       sock_zerocopy_put(uarg);
        return 0;
 
 error_efault:
        err = -EFAULT;
 error:
-       sock_zerocopy_put_abort(uarg);
+       sock_zerocopy_put_abort(uarg, extra_uref);
        cork->length -= length;
        IP6_INC_STATS(sock_net(sk), rt->rt6i_idev, IPSTATS_MIB_OUTDISCARDS);
        refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);