| Message ID | 20201222000926.1054993-6-jonathan.lemon@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Generic zcopy_* functions | expand |
On Mon, Dec 21, 2020 at 7:09 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > From: Jonathan Lemon <bsd@fb.com> > > Replace sock_zerocopy_put with the generic skb_zcopy_put() > function. Pass 'true' as the success argument, as this > is identical to no change. > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> uarg->zerocopy may be false if sock_zerocopy_put_abort is called from tcp_sendmsg_locked
On Tue, Dec 22, 2020 at 09:42:40AM -0500, Willem de Bruijn wrote: > On Mon, Dec 21, 2020 at 7:09 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > From: Jonathan Lemon <bsd@fb.com> > > > > Replace sock_zerocopy_put with the generic skb_zcopy_put() > > function. Pass 'true' as the success argument, as this > > is identical to no change. > > > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> > > uarg->zerocopy may be false if sock_zerocopy_put_abort is called from > tcp_sendmsg_locked Yes, it may well be false. The original logic goes: sock_zerocopy_put_abort() sock_zerocopy_put() sock_zerocopy_callback(..., success = uarg->zerocopy) if (success) The new logic is now: sock_zerocopy_put_abort() sock_zerocopy_callback(..., success = true) uarg->zerocopy = uarg->zerocopy & success if (uarg->zerocopy) The success value ls latched into uarg->zerocopy, and any failure is persistent. Hence my comment about passing 'true' not changing the logic. -- Jonathan
On Tue, Dec 22, 2020 at 12:19 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > On Tue, Dec 22, 2020 at 09:42:40AM -0500, Willem de Bruijn wrote: > > On Mon, Dec 21, 2020 at 7:09 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > > > From: Jonathan Lemon <bsd@fb.com> > > > > > > Replace sock_zerocopy_put with the generic skb_zcopy_put() > > > function. Pass 'true' as the success argument, as this > > > is identical to no change. > > > > > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> > > > > uarg->zerocopy may be false if sock_zerocopy_put_abort is called from > > tcp_sendmsg_locked > > Yes, it may well be false. The original logic goes: > > sock_zerocopy_put_abort() > sock_zerocopy_put() > sock_zerocopy_callback(..., success = uarg->zerocopy) > if (success) > > The new logic is now: > > sock_zerocopy_put_abort() > sock_zerocopy_callback(..., success = true) > uarg->zerocopy = uarg->zerocopy & success > if (uarg->zerocopy) > > The success value ls latched into uarg->zerocopy, and any failure > is persistent. Good point. It's not entirely obvious, and a bit unintuitive to pass success in an abort statement. But it works. Abort does not schedule a notification if no skb associated with the uarg was sent. And if it was, the zerocopy state will reflect the & of all those packets. On which note, if renaming the currently inconsistent name, maybe renaming to zerocopy_success is the more descriptive one, then.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index c9d7de9d624d..238166b522f7 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); void sock_zerocopy_callback(struct ubuf_info *uarg, bool success); @@ -1474,6 +1473,12 @@ 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); +} + /* Release a reference on a zerocopy structure */ static inline void skb_zcopy_clear(struct sk_buff *skb, bool zerocopy) { diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 73699dbdc4a1..984760dd670b 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -1251,13 +1251,6 @@ void sock_zerocopy_callback(struct ubuf_info *uarg, bool success) } EXPORT_SYMBOL_GPL(sock_zerocopy_callback); -void sock_zerocopy_put(struct ubuf_info *uarg) -{ - if (uarg) - uarg->callback(uarg, uarg->zerocopy); -} -EXPORT_SYMBOL_GPL(sock_zerocopy_put); - void sock_zerocopy_put_abort(struct ubuf_info *uarg, bool have_uref) { if (uarg) { @@ -1267,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: