On Fri, Dec 18, 2020 at 3:20 PM Jonathan Lemon <jonathan.le...@gmail.com> wrote:
>
> From: Jonathan Lemon <b...@fb.com>
>
> In preparation for further work, the zcopy* routines will
> become basic building blocks, while the zerocopy* ones will
> be specific for the existing zerocopy implementation.
Plural. There already are multiple disjoint zerocopy implementations:
msg_zerocopy, tpacket and vhost_net.
Which API is each intended to use? After this patch
tcp_sendmsg_locked() calls both skb_zcopy_put and
sock_zerocopy_put_abort, so I don't think that that is simplifying the
situation.
This is tricky code. Perhaps best to change only what is needed
instead of targeting a larger cleanup. It's hard to reason that this
patch is safe in all three existing cases, for instance.
> All uargs should have a callback function, (unless nouarg
> is set), so push all special case logic handling down into
> the callbacks. This slightly pessimizes the refcounted cases,
What does this mean?
> but makes the skb_zcopy_*() routines clearer.
>
> Signed-off-by: Jonathan Lemon <jonathan.le...@gmail.com>
> ---
> include/linux/skbuff.h | 19 +++++++++----------
> net/core/skbuff.c | 21 +++++++++------------
> net/ipv4/tcp.c | 2 +-
> 3 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index fb6dd6af0f82..df98d61e8c51 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -499,7 +499,6 @@ static inline void sock_zerocopy_get(struct ubuf_info
> *uarg)
> refcount_inc(&uarg->refcnt);
> }
>
> -void sock_zerocopy_put(struct ubuf_info *uarg);
> void sock_zerocopy_put_abort(struct ubuf_info *uarg, bool have_uref);
The rename of sock_zerocopy_put without rename of
sock_zerocopy_put_abort makes the API less consistent, I believe. See
how the calls are close together in tcp_sendmsg_locked.
> void sock_zerocopy_callback(struct ubuf_info *uarg, bool success);
> @@ -1474,20 +1473,20 @@ static inline void *skb_zcopy_get_nouarg(struct
> sk_buff *skb)
> return (void *)((uintptr_t) skb_shinfo(skb)->destructor_arg & ~0x1UL);
> }
>
> +static inline void skb_zcopy_put(struct ubuf_info *uarg)
> +{
> + if (uarg)
> + uarg->callback(uarg, true);
> +}
> +
Can we just use skb_zcopy_clear?
> /* Release a reference on a zerocopy structure */
> -static inline void skb_zcopy_clear(struct sk_buff *skb, bool zerocopy)
> +static inline void skb_zcopy_clear(struct sk_buff *skb, bool succsss)
succsss -> success. More importantly, why change the argument name?
> {
> struct ubuf_info *uarg = skb_zcopy(skb);
>
> 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);
> - }
> + if (!skb_zcopy_is_nouarg(skb))
> + uarg->callback(uarg, succsss);
>
> skb_shinfo(skb)->zc_flags &= ~SKBZC_FRAGMENTS;
> }
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 327ee8938f78..984760dd670b 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,18 +1241,15 @@ void sock_zerocopy_callback(struct ubuf_info *uarg,
> bool success)
> consume_skb(skb);
> sock_put(sk);
> }
> -EXPORT_SYMBOL_GPL(sock_zerocopy_callback);
>
> -void sock_zerocopy_put(struct ubuf_info *uarg)
> +void sock_zerocopy_callback(struct ubuf_info *uarg, bool success)
> {
> - if (uarg && refcount_dec_and_test(&uarg->refcnt)) {
> - if (uarg->callback)
> - uarg->callback(uarg, uarg->zerocopy);
> - else
> - consume_skb(skb_from_uarg(uarg));
I suppose this can be removed after commit 0a4a060bb204 ("sock: fix
zerocopy_success regression with msg_zerocopy"). Cleaning that up
would better be a separate patch that explains why the removal is
safe.
It's also fine to bundle with moving refcount_dec_and_test into
sock_zerocopy_callback, which indeed follows from it.
> - }
> + uarg->zerocopy = uarg->zerocopy & success;
> +
> + if (refcount_dec_and_test(&uarg->refcnt))
> + __sock_zerocopy_callback(uarg);
This can be wrapped in existing sock_zerocopy_callback. No need for a
__sock_zerocopy_callback.
If you do want a separate API for existing msg_zerocopy distinct from
existing skb_zcopy, then maybe rename the functions only used by
msg_zerocopy to have prefix msg_zerocopy_ instead of sock_zerocopy_
> }
> -EXPORT_SYMBOL_GPL(sock_zerocopy_put);
> +EXPORT_SYMBOL_GPL(sock_zerocopy_callback);
>
> void sock_zerocopy_put_abort(struct ubuf_info *uarg, bool have_uref)
> {
> @@ -1263,7 +1260,7 @@ void sock_zerocopy_put_abort(struct ubuf_info *uarg,
> bool have_uref)
> uarg->len--;
>
> if (have_uref)
> - sock_zerocopy_put(uarg);
> + skb_zcopy_put(uarg);
> }
> }
> EXPORT_SYMBOL_GPL(sock_zerocopy_put_abort);
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index fea9bae370e4..5c38080df13f 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1429,7 +1429,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr
> *msg, size_t size)
> tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
> }
> out_nopush:
> - sock_zerocopy_put(uarg);
> + skb_zcopy_put(uarg);
> return copied + copied_syn;
>
> do_error:
> --
> 2.24.1
>
