On Fri, Sep 14, 2018 at 11:37 PM Subash Abhinov Kasiviswanathan
<subas...@codeaurora.org> wrote:
>
> On 2018-09-14 11:59, Willem de Bruijn wrote:
> > From: Willem de Bruijn <will...@google.com>
> >
> > Avoid the socket lookup cost in udp_gro_receive if no socket has a
> > gro callback configured.
> >
> > Signed-off-by: Willem de Bruijn <will...@google.com>
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index 4f6aa95a9b12..f44fe328aa0f 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -405,7 +405,7 @@ static struct sk_buff *udp4_gro_receive(struct
> > list_head *head,
> > {
> > struct udphdr *uh = udp_gro_udphdr(skb);
> >
> > - if (unlikely(!uh))
> > + if (unlikely(!uh) ||
> > !static_branch_unlikely(&udp_encap_needed_key))
> > goto flush;
> >
>
> Hi Willem
>
> Does this need to be
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 6dd3f0a..fcd5589 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -407,7 +407,7 @@ static struct sk_buff *udp4_gro_receive(struct
> list_head *head,
> {
> struct udphdr *uh = udp_gro_udphdr(skb);
>
> - if (unlikely(!uh) ||
> !static_branch_unlikely(&udp_encap_needed_key))
> + if (unlikely(!uh) ||
> static_branch_unlikely(&udp_encap_needed_key))
> goto flush;
>
> /* Don't bother verifying checksum if we're going to flush
> anyway. */
>
> I tried setting UDP_GRO socket option and I had to make this change to
> exercise the udp_gro_receive_cb code path.
Thanks for trying it out, Subash.
The static_branch logic is correct as is. It skips the gro logic if
the static key is not enabled.
But there is indeed a bug. the UDP_GRO setsockopt has to enable
the static key. A full patch is more complete, but this should fix it for
now:
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2506,6 +2506,7 @@ int udp_lib_setsockopt(struct sock *sk, int
level, int optname,
if (!udp_sk(sk)->gro_receive) {
udp_sk(sk)->gro_complete = udp_gro_complete_cb;
udp_sk(sk)->gro_receive = udp_gro_receive_cb;
+ udp_encap_enable();
sorry about that. I had tested the two patches independently, but
apparently not on top of each other.
Independent of udp gro, I tested the static key by running udp_rr
with perf record -a -g and looking for __udp4_lib_lookup. With the
patch, all calls are from __udp4_lib_rcv else udp_gro_receive also
shows up. I had to stress the porttable by opening ~32K ports.
Also, the simple patch to udpgso_bench_rx.c that I used to test
gro:
diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c
b/tools/testing/selftests/net/udpgso_bench_rx.c
index 727cf67a3f75..35ba8567fc56 100644
--- a/tools/testing/selftests/net/udpgso_bench_rx.c
+++ b/tools/testing/selftests/net/udpgso_bench_rx.c
@@ -31,6 +31,11 @@
#include <sys/wait.h>
#include <unistd.h>
+#ifndef UDP_GRO
+#define UDP_GRO 104
+#endif
+
+static bool cfg_do_gro;
static int cfg_port = 8000;
static bool cfg_tcp;
static bool cfg_verify;
@@ -89,6 +94,11 @@ static int do_socket(bool do_tcp)
if (setsockopt(fd, SOL_SOCKET, SO_REUSEPORT, &val, sizeof(val)))
error(1, errno, "setsockopt reuseport");
+ if (cfg_do_gro) {
+ if (setsockopt(fd, SOL_UDP, UDP_GRO, &val, sizeof(val)))
+ error(1, errno, "setsockopt gro");
+ }
+
addr.sin6_family = PF_INET6;
addr.sin6_port = htons(cfg_port);
addr.sin6_addr = in6addr_any;
@@ -199,8 +209,11 @@ static void parse_opts(int argc, char **argv)
{
int c;
- while ((c = getopt(argc, argv, "ptv")) != -1) {
+ while ((c = getopt(argc, argv, "gptv")) != -1) {
switch (c) {
+ case 'g':
+ cfg_do_gro = true;
+ break;
case 'p':
cfg_port = htons(strtoul(optarg, NULL, 0));
break;
