On Tue, Dec 22, 2020 at 12:48 PM Jonathan Lemon
<jonathan.le...@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.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.
>
> 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 :)
