netdev - Re: [PATCH net-next] net: use indirect call wrappers for skb_copy_datagram

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Mar 2020 10:55:50 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net-next] net: use indirect call wrappers for skb_copy_datagram_iter()

On Wed, Mar 25, 2020 at 7:52 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Tue, 2020-03-24 at 19:23 -0700, Eric Dumazet wrote:
> > TCP recvmsg() calls skb_copy_datagram_iter(), which
> > calls an indirect function (cb pointing to simple_copy_to_iter())
> > for every MSS (fragment) present in the skb.
> >
> > CONFIG_RETPOLINE=y forces a very expensive operation
> > that we can avoid thanks to indirect call wrappers.
> >
> > This patch gives a 13% increase of performance on
> > a single flow, if the bottleneck is the thread reading
> > the TCP socket.
> >
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>

Acked-by: Willem de Bruijn <willemb@...gle.com>

> > @@ -438,8 +445,9 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
> >
> >                       if (copy > len)
> >                               copy = len;
> > -                     n = cb(vaddr + skb_frag_off(frag) + offset - start,
> > -                            copy, data, to);
> > +                     n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
> > +                                     vaddr + skb_frag_off(frag) + offset - start,
> > +                                     copy, data, to);
> >                       kunmap(page);
> >                       offset += n;
> >                       if (n != copy)
>
> I wondered if we could add a second argument for
> 'csum_and_copy_to_iter', but I guess that is a slower path anyway and
> more datapoint would be needed. The patch LGTM, thanks!
>
> Acked-by: Paolo Abeni <pabeni@...hat.com>

On the UDP front this reminded me of another indirect function call
without indirect call wrapper: getfrag in __ip_append_data.

That is called for each datagram once per linear + once per page. That
said, the noise in my quick RR test was too great to measure any
benefit from the following. Paolo, did you happen to also look at that
when introducing the indirect callers? Seems like it won't hurt to
add.

 static int
 ip_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
@@ -1128,7 +1129,8 @@ static int __ip_append_data(struct sock *sk,
                        }

                        copy = datalen - transhdrlen - fraggap - pagedlen;
-                       if (copy > 0 && getfrag(from, data +
transhdrlen, offset, copy, fraggap, skb) < 0) {
+                       if (copy > 0 &&
+                           INDIRECT_CALL_1(getfrag,
ip_generic_getfrag, from, data + transhdrlen, offset, copy, fraggap,
skb) < 0) {
                                err = -EFAULT;
                                kfree_skb(skb);
                                goto error;
@@ -1170,7 +1172,7 @@ static int __ip_append_data(struct sock *sk,
                        unsigned int off;

                        off = skb->len;
-                       if (getfrag(from, skb_put(skb, copy),
+                       if (INDIRECT_CALL_1(getfrag,
ip_generic_getfrag, from, skb_put(skb, copy),
                                        offset, copy, off, skb) < 0) {
                                __skb_trim(skb, off);
                                err = -EFAULT;
@@ -1195,7 +1197,7 @@ static int __ip_append_data(struct sock *sk,
                                get_page(pfrag->page);
                        }
                        copy = min_t(int, copy, pfrag->size - pfrag->offset);
-                       if (getfrag(from,
+                       if (INDIRECT_CALL_1(getfrag, ip_generic_getfrag, from,