diff mbox series

[04/12,v2,RFC] skbuff: Push status and refcounts into sock_zerocopy_callback

Message ID 20201222000926.1054993-5-jonathan.lemon@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Generic zcopy_* functions | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 18 maintainers not CCed: decui@microsoft.com gustavoars@kernel.org gnault@redhat.com fw@strlen.de rdunlap@infradead.org nogikh@google.com jakub@cloudflare.com davem@davemloft.net viro@zeniv.linux.org.uk willemb@google.com bsd@fb.com pabeni@redhat.com pablo@netfilter.org linmiaohe@huawei.com kuba@kernel.org elver@google.com kyk.segfault@gmail.com steffen.klassert@secunet.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 9407 this patch: 9407
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: From:/Signed-off-by: email address mismatch: 'From: Jonathan Lemon <bsd@fb.com>' != 'Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>'
netdev/build_allmodconfig_warn success Errors and warnings before: 9499 this patch: 9499
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Jonathan Lemon Dec. 22, 2020, 12:09 a.m. UTC 
From: Jonathan Lemon <bsd@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.lemon@gmail.com>
---
 include/linux/skbuff.h |  3 ---
 net/core/skbuff.c      | 14 +++++++++++---
 2 files changed, 11 insertions(+), 6 deletions(-)

Comments

Willem de Bruijn Dec. 22, 2020, 2:43 p.m. UTC | #1 
On Mon, Dec 21, 2020 at 7:09 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote:
>
> From: Jonathan Lemon <bsd@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.lemon@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.
David Ahern Dec. 22, 2020, 4:49 p.m. UTC | #2 
On 12/22/20 7:43 AM, Willem de Bruijn wrote:
> 
>>  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.
> 


sock_zerocopy_callback seems to be the only one at the moment - or the
most dominant if I goofed my search. Could use the INDIRECT_CALL macros
for it.
Jonathan Lemon Dec. 22, 2020, 5:48 p.m. UTC | #3 
On Tue, Dec 22, 2020 at 09:43:39AM -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>
> >
> > 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.lemon@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.

I think the helper makes the logic clearer, and prevents misuse of
the success variable in the main function (use of last value vs the
latched value).  If you really feel that strongly about it, I'll 
fold it into one function.


> >  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.

Yes, this was the source of my original comment about this being 
a slight pessimization - moving the refcount into the function.

I briefly considered adding a flag like 'use_refcnt', so the logic
becomes:

    if (uarg && (!uarg->use_refcnt || refcount_dec_and_test(&uarg->refcnt)))

But thought this might be too much micro-optimization.  But if 
the call overhead is significant, I can put this back in.
Willem de Bruijn Dec. 22, 2020, 10:38 p.m. UTC | #4 
On Tue, Dec 22, 2020 at 12:48 PM Jonathan Lemon
<jonathan.lemon@gmail.com> wrote:
>
> On Tue, Dec 22, 2020 at 09:43:39AM -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>
> > >
> > > 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.lemon@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.
>
> I think the helper makes the logic clearer, and prevents misuse of
> the success variable in the main function (use of last value vs the
> latched value).  If you really feel that strongly about it, I'll
> fold it into one function.

Ok. Thanks, no, it's fine.

>
> > >  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.
>
> Yes, this was the source of my original comment about this being
> a slight pessimization - moving the refcount into the function.
>
> I briefly considered adding a flag like 'use_refcnt', so the logic
> becomes:
>
>     if (uarg && (!uarg->use_refcnt || refcount_dec_and_test(&uarg->refcnt)))
>
> But thought this might be too much micro-optimization.  But if
> the call overhead is significant, I can put this back in.

Agreed on the premature optimization. Let's find out :)
diff mbox series

Patch

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);
 
 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);