[PATCH net] udp: Remove an unnecessary variable in udp[46]_lib_lookup2().

[PATCH net] udp: Remove an unnecessary variable in udp[46]_lib_lookup2().

[Kuniyuki Iwashima]

This patch removes an unnecessary variable in udp[46]_lib_lookup2() and
makes it easier to resolve a merge conflict with bpf-next reported in
the link below.

Link: https://lore.kernel.org/linux-next/20200722132143.700a5ccc@canb.auug.org.au/
Fixes: efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
---
 net/ipv4/udp.c | 15 ++++++++-------
 net/ipv6/udp.c | 15 ++++++++-------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 4077d589b72e..22fb231e27c3 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -416,7 +416,7 @@ static struct sock *udp4_lib_lookup2(struct net *net,
 				     struct udp_hslot *hslot2,
 				     struct sk_buff *skb)
 {
-	struct sock *sk, *result, *reuseport_result;
+	struct sock *sk, *result;
 	int score, badness;
 	u32 hash = 0;
 
@@ -426,19 +426,20 @@ static struct sock *udp4_lib_lookup2(struct net *net,
 		score = compute_score(sk, net, saddr, sport,
 				      daddr, hnum, dif, sdif);
 		if (score > badness) {
-			reuseport_result = NULL;
+			result = NULL;
 
 			if (sk->sk_reuseport &&
 			    sk->sk_state != TCP_ESTABLISHED) {
 				hash = udp_ehashfn(net, daddr, hnum,
 						   saddr, sport);
-				reuseport_result = reuseport_select_sock(sk, hash, skb,
-									 sizeof(struct udphdr));
-				if (reuseport_result && !reuseport_has_conns(sk, false))
-					return reuseport_result;
+				result = reuseport_select_sock(sk, hash, skb,
+							       sizeof(struct udphdr));
+				if (result && !reuseport_has_conns(sk, false))
+					return result;
 			}
 
-			result = reuseport_result ? : sk;
+			if (!result)
+				result = sk;
 			badness = score;
 		}
 	}
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index a8d74f44056a..29c7bb2609c4 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -148,7 +148,7 @@ static struct sock *udp6_lib_lookup2(struct net *net,
 		int dif, int sdif, struct udp_hslot *hslot2,
 		struct sk_buff *skb)
 {
-	struct sock *sk, *result, *reuseport_result;
+	struct sock *sk, *result;
 	int score, badness;
 	u32 hash = 0;
 
@@ -158,20 +158,21 @@ static struct sock *udp6_lib_lookup2(struct net *net,
 		score = compute_score(sk, net, saddr, sport,
 				      daddr, hnum, dif, sdif);
 		if (score > badness) {
-			reuseport_result = NULL;
+			result = NULL;
 
 			if (sk->sk_reuseport &&
 			    sk->sk_state != TCP_ESTABLISHED) {
 				hash = udp6_ehashfn(net, daddr, hnum,
 						    saddr, sport);
 
-				reuseport_result = reuseport_select_sock(sk, hash, skb,
-									 sizeof(struct udphdr));
-				if (reuseport_result && !reuseport_has_conns(sk, false))
-					return reuseport_result;
+				result = reuseport_select_sock(sk, hash, skb,
+							       sizeof(struct udphdr));
+				if (result && !reuseport_has_conns(sk, false))
+					return result;
 			}
 
-			result = reuseport_result ? : sk;
+			if (!result)
+				result = sk;
 			badness = score;
 		}
 	}
-- 
2.17.2 (Apple Git-113)

Re: [PATCH net] udp: Remove an unnecessary variable in udp[46]_lib_lookup2().

[David Miller]

From: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
Date: Thu, 23 Jul 2020 01:52:27 +0900

> This patch removes an unnecessary variable in udp[46]_lib_lookup2() and
> makes it easier to resolve a merge conflict with bpf-next reported in
> the link below.
> 
> Link: https://lore.kernel.org/linux-next/20200722132143.700a5ccc@canb.auug.org.au/
> Fixes: efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>

This doesn't apply to net-next.

Re: [PATCH net] udp: Remove an unnecessary variable in udp[46]_lib_lookup2().

[Kuniyuki Iwashima]

From:   David Miller <davem@davemloft.net>
Date:   Thu, 23 Jul 2020 15:10:51 -0700 (PDT)
> From: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> Date: Thu, 23 Jul 2020 01:52:27 +0900
> 
> > This patch removes an unnecessary variable in udp[46]_lib_lookup2() and
> > makes it easier to resolve a merge conflict with bpf-next reported in
> > the link below.
> > 
> > Link: https://lore.kernel.org/linux-next/20200722132143.700a5ccc@canb.auug.org.au/
> > Fixes: efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> 
> This doesn't apply to net-next.

Yes. I think this kind of patch should be submitted to net-next, but this
is for the net tree. Please let me add more description.

Currently, the net and net-next trees conflict in udp[46]_lib_lookup2()
between

   efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")

and

   7629c73a1466 ("udp: Extract helper for selecting socket from reuseport group")
   2a08748cd384 ("udp6: Extract helper for selecting socket from reuseport group")
.

The conflict is reported in the link[0] and Jakub suggested how to resolve
it[1]. To ease the merge conflict, Jakub and I have to send follow up patches to
the bpf-next and net trees.

Now, his patchset (7629c73a1466 and 2a08748cd384) to bpf-next is merged
into net-next, and his follow up patch is applied in bpf-next[2].

I fixed a bug in efc6b6f6c311, but it introduced an unnecessary variable
and made the conflict worse. So I sent this follow up patch to net tree.

However, I do not know the best way to resolve the conflict, so any comments
are welcome.

[0] https://lore.kernel.org/linux-next/20200722132143.700a5ccc@canb.auug.org.au/
[1] https://lore.kernel.org/linux-next/87wo2vwxq6.fsf@cloudflare.com/
[2] https://lore.kernel.org/netdev/20200722165902.51857-1-kuniyu@amazon.co.jp/T/#t


Best Regards,
Kuniyuki

Re: [PATCH net] udp: Remove an unnecessary variable in udp[46]_lib_lookup2().

[Willem de Bruijn]

On Fri, Jul 24, 2020 at 2:13 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>
> From:   David Miller <davem@davemloft.net>
> Date:   Thu, 23 Jul 2020 15:10:51 -0700 (PDT)
> > From: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > Date: Thu, 23 Jul 2020 01:52:27 +0900
> >
> > > This patch removes an unnecessary variable in udp[46]_lib_lookup2() and
> > > makes it easier to resolve a merge conflict with bpf-next reported in
> > > the link below.
> > >
> > > Link: https://lore.kernel.org/linux-next/20200722132143.700a5ccc@canb.auug.org.au/
> > > Fixes: efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> > > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> >
> > This doesn't apply to net-next.
>
> Yes. I think this kind of patch should be submitted to net-next, but this
> is for the net tree. Please let me add more description.
>
> Currently, the net and net-next trees conflict in udp[46]_lib_lookup2()
> between
>
>    efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
>
> and
>
>    7629c73a1466 ("udp: Extract helper for selecting socket from reuseport group")
>    2a08748cd384 ("udp6: Extract helper for selecting socket from reuseport group")
> .
>
> The conflict is reported in the link[0] and Jakub suggested how to resolve
> it[1]. To ease the merge conflict, Jakub and I have to send follow up patches to
> the bpf-next and net trees.
>
> Now, his patchset (7629c73a1466 and 2a08748cd384) to bpf-next is merged
> into net-next, and his follow up patch is applied in bpf-next[2].
>
> I fixed a bug in efc6b6f6c311, but it introduced an unnecessary variable
> and made the conflict worse. So I sent this follow up patch to net tree.
>
> However, I do not know the best way to resolve the conflict, so any comments
> are welcome.

Perhaps simpler is to apply this change to bpf-next:

"
                        badness = score;
-                       result = sk;
+                       if (!result)
+                               result = sk;
"

After which the remaining conflict between bpf-next and net is

"
++<<<<<<< HEAD
 +                      result = lookup_reuseport(net, sk, skb,
 +                                                saddr, sport, daddr, hnum);
 +                      if (result && !reuseport_has_conns(sk, false))
 +                              return result;
 +
 +                      badness = score;
 +                      if (!result)
 +                              result = sk;
++=======
+                       reuseport_result = NULL;
+
+                       if (sk->sk_reuseport &&
+                           sk->sk_state != TCP_ESTABLISHED) {
+                               hash = udp_ehashfn(net, daddr, hnum,
+                                                  saddr, sport);
+                               reuseport_result =
reuseport_select_sock(sk, hash, skb,
+
  sizeof(struct udphdr));
+                               if (reuseport_result &&
!reuseport_has_conns(sk, false))
+                                       return reuseport_result;
+                       }
+
+                       result = reuseport_result ? : sk;
+                       badness = score;
++>>>>>>> netdev-net/master
"

And we can just take bpf-next HEAD. Either that or handle the
corresponding change in the merge fix-up itself.

(note that bpf-next is one patch ahead of netdev-nn)

Re: [PATCH net] udp: Remove an unnecessary variable in udp[46]_lib_lookup2().

[Alexei Starovoitov]

On Fri, Jul 24, 2020 at 6:38 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Fri, Jul 24, 2020 at 2:13 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> >
> > From:   David Miller <davem@davemloft.net>
> > Date:   Thu, 23 Jul 2020 15:10:51 -0700 (PDT)
> > > From: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > > Date: Thu, 23 Jul 2020 01:52:27 +0900
> > >
> > > > This patch removes an unnecessary variable in udp[46]_lib_lookup2() and
> > > > makes it easier to resolve a merge conflict with bpf-next reported in
> > > > the link below.
> > > >
> > > > Link: https://lore.kernel.org/linux-next/20200722132143.700a5ccc@canb.auug.org.au/
> > > > Fixes: efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
> > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > > > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> > > > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > >
> > > This doesn't apply to net-next.
> >
> > Yes. I think this kind of patch should be submitted to net-next, but this
> > is for the net tree. Please let me add more description.
> >
> > Currently, the net and net-next trees conflict in udp[46]_lib_lookup2()
> > between
> >
> >    efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
> >
> > and
> >
> >    7629c73a1466 ("udp: Extract helper for selecting socket from reuseport group")
> >    2a08748cd384 ("udp6: Extract helper for selecting socket from reuseport group")
> > .
> >
> > The conflict is reported in the link[0] and Jakub suggested how to resolve
> > it[1]. To ease the merge conflict, Jakub and I have to send follow up patches to
> > the bpf-next and net trees.
> >
> > Now, his patchset (7629c73a1466 and 2a08748cd384) to bpf-next is merged
> > into net-next, and his follow up patch is applied in bpf-next[2].
> >
> > I fixed a bug in efc6b6f6c311, but it introduced an unnecessary variable
> > and made the conflict worse. So I sent this follow up patch to net tree.
> >
> > However, I do not know the best way to resolve the conflict, so any comments
> > are welcome.
>
> Perhaps simpler is to apply this change to bpf-next:

I'm fine whichever way.
Could you please submit an official patch?

Re: [PATCH net] udp: Remove an unnecessary variable in udp[46]_lib_lookup2().

[David Miller]

From: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
Date: Fri, 24 Jul 2020 15:13:04 +0900

> Yes. I think this kind of patch should be submitted to net-next, but
> this is for the net tree. Please let me add more description.

This does not fix a bug, therefore 'net' is not appropriate.

The merge conflicts should be handled by the appropriate maintainer
when the merges happen.

Re: [PATCH net] udp: Remove an unnecessary variable in udp[46]_lib_lookup2().

[Willem de Bruijn]

On Fri, Jul 24, 2020 at 3:13 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Jul 24, 2020 at 6:38 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Fri, Jul 24, 2020 at 2:13 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> > >
> > > From:   David Miller <davem@davemloft.net>
> > > Date:   Thu, 23 Jul 2020 15:10:51 -0700 (PDT)
> > > > From: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > > > Date: Thu, 23 Jul 2020 01:52:27 +0900
> > > >
> > > > > This patch removes an unnecessary variable in udp[46]_lib_lookup2() and
> > > > > makes it easier to resolve a merge conflict with bpf-next reported in
> > > > > the link below.
> > > > >
> > > > > Link: https://lore.kernel.org/linux-next/20200722132143.700a5ccc@canb.auug.org.au/
> > > > > Fixes: efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
> > > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > > > > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> > > > > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > > >
> > > > This doesn't apply to net-next.
> > >
> > > Yes. I think this kind of patch should be submitted to net-next, but this
> > > is for the net tree. Please let me add more description.
> > >
> > > Currently, the net and net-next trees conflict in udp[46]_lib_lookup2()
> > > between
> > >
> > >    efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
> > >
> > > and
> > >
> > >    7629c73a1466 ("udp: Extract helper for selecting socket from reuseport group")
> > >    2a08748cd384 ("udp6: Extract helper for selecting socket from reuseport group")
> > > .
> > >
> > > The conflict is reported in the link[0] and Jakub suggested how to resolve
> > > it[1]. To ease the merge conflict, Jakub and I have to send follow up patches to
> > > the bpf-next and net trees.
> > >
> > > Now, his patchset (7629c73a1466 and 2a08748cd384) to bpf-next is merged
> > > into net-next, and his follow up patch is applied in bpf-next[2].
> > >
> > > I fixed a bug in efc6b6f6c311, but it introduced an unnecessary variable
> > > and made the conflict worse. So I sent this follow up patch to net tree.
> > >
> > > However, I do not know the best way to resolve the conflict, so any comments
> > > are welcome.
> >
> > Perhaps simpler is to apply this change to bpf-next:
>
> I'm fine whichever way.
> Could you please submit an official patch?

http://patchwork.ozlabs.org/project/netdev/patch/20200725025457.1004164-1-willemdebruijn.kernel@gmail.com/

Not sure whether it helps vs doing this as part of the merge conflict
(which remains). Either way after conflict resolution should be

"
static struct sock *udp4_lib_lookup2(struct net *net,
                                     __be32 saddr, __be16 sport,
                                     __be32 daddr, unsigned int hnum,
                                     int dif, int sdif,
                                     struct udp_hslot *hslot2,
                                     struct sk_buff *skb)
{
        struct sock *sk, *result;
        int score, badness;

        result = NULL;
        badness = 0;
        udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
                score = compute_score(sk, net, saddr, sport,
                                      daddr, hnum, dif, sdif);
                if (score > badness) {
                        result = lookup_reuseport(net, sk, skb,
                                                  saddr, sport, daddr, hnum);
                        if (result && !reuseport_has_conns(sk, false))
                                return result;

                        badness = score;
                        if (!result)
                                result = sk;
                }
        }
        return result;
}
"

Re: [PATCH net] udp: Remove an unnecessary variable in udp[46]_lib_lookup2().

[Kuniyuki Iwashima]

From:   David Miller <davem@davemloft.net>
Date:   Fri, 24 Jul 2020 16:48:47 -0700 (PDT)
> From: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> Date: Fri, 24 Jul 2020 15:13:04 +0900
> 
> > Yes. I think this kind of patch should be submitted to net-next, but
> > this is for the net tree. Please let me add more description.
> 
> This does not fix a bug, therefore 'net' is not appropriate.

Exactly, I am sorry for the confusion.


> The merge conflicts should be handled by the appropriate maintainer
> when the merges happen.

I will keep this in mind, thank you.

Best Regards,
Kuniyuki