[PATCH net-next 0/2] tcp/dccp: Cleanup initialisation code of refcounted.

[PATCH net-next 0/2] tcp/dccp: Cleanup initialisation code of refcounted.

[Kuniyuki Iwashima]

These patches cleanup initialisation of the refcounted which is passed to
__inet_lookup_skb().

Kuniyuki Iwashima (2):
  tcp/dccp: Move initialisation of refcounted into if block.
  tcp/dccp: Remove unnecessary initialization of refcounted.

 include/net/inet6_hashtables.h | 11 +++++++----
 include/net/inet_hashtables.h  | 11 +++++++----
 net/dccp/ipv4.c                |  1 -
 net/dccp/ipv6.c                |  1 -
 net/ipv4/tcp_ipv4.c            |  1 -
 net/ipv6/tcp_ipv6.c            |  1 -
 6 files changed, 14 insertions(+), 12 deletions(-)

-- 
2.17.2 (Apple Git-113)

[PATCH net-next 0/2] tcp/dccp: Cleanup initialisation code of refcounted.

[Kuniyuki Iwashima]

These patches cleanup initialisation of the refcounted which is passed to
__inet_lookup_skb().

Kuniyuki Iwashima (2):
  tcp/dccp: Move initialisation of refcounted into if block.
  tcp/dccp: Remove unnecessary initialization of refcounted.

 include/net/inet6_hashtables.h | 11 +++++++----
 include/net/inet_hashtables.h  | 11 +++++++----
 net/dccp/ipv4.c                |  1 -
 net/dccp/ipv6.c                |  1 -
 net/ipv4/tcp_ipv4.c            |  1 -
 net/ipv6/tcp_ipv6.c            |  1 -
 6 files changed, 14 insertions(+), 12 deletions(-)

-- 
2.17.2 (Apple Git-113)

[PATCH net-next 1/2] tcp/dccp: Move initialisation of refcounted into if block.

[Kuniyuki Iwashima]

The refcounted is initialised at most three times.

  - __inet_lookup_skb() sets it true.
  - skb_steal_sock() is false and __inet_lookup() sets it true.
  - __inet_lookup_established() is false and __inet_lookup() sets it false.

We do not need to initialise refcounted again and again, so we should do
it just before return.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
---
 include/net/inet6_hashtables.h | 11 +++++++----
 include/net/inet_hashtables.h  | 11 +++++++----
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
index fe96bf247aac..9b6c97100d54 100644
--- a/include/net/inet6_hashtables.h
+++ b/include/net/inet6_hashtables.h
@@ -70,9 +70,11 @@ static inline struct sock *__inet6_lookup(struct net *net,
 	struct sock *sk = __inet6_lookup_established(net, hashinfo, saddr,
 						     sport, daddr, hnum,
 						     dif, sdif);
-	*refcounted = true;
-	if (sk)
+	if (sk) {
+		*refcounted = true;
 		return sk;
+	}
+
 	*refcounted = false;
 	return inet6_lookup_listener(net, hashinfo, skb, doff, saddr, sport,
 				     daddr, hnum, dif, sdif);
@@ -87,9 +89,10 @@ static inline struct sock *__inet6_lookup_skb(struct inet_hashinfo *hashinfo,
 {
 	struct sock *sk = skb_steal_sock(skb);
 
-	*refcounted = true;
-	if (sk)
+	if (sk) {
+		*refcounted = true;
 		return sk;
+	}
 
 	return __inet6_lookup(dev_net(skb_dst(skb)->dev), hashinfo, skb,
 			      doff, &ipv6_hdr(skb)->saddr, sport,
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index d0019d3395cf..aa859bf8607b 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -345,9 +345,11 @@ static inline struct sock *__inet_lookup(struct net *net,
 
 	sk = __inet_lookup_established(net, hashinfo, saddr, sport,
 				       daddr, hnum, dif, sdif);
-	*refcounted = true;
-	if (sk)
+	if (sk) {
+		*refcounted = true;
 		return sk;
+	}
+
 	*refcounted = false;
 	return __inet_lookup_listener(net, hashinfo, skb, doff, saddr,
 				      sport, daddr, hnum, dif, sdif);
@@ -382,9 +384,10 @@ static inline struct sock *__inet_lookup_skb(struct inet_hashinfo *hashinfo,
 	struct sock *sk = skb_steal_sock(skb);
 	const struct iphdr *iph = ip_hdr(skb);
 
-	*refcounted = true;
-	if (sk)
+	if (sk) {
+		*refcounted = true;
 		return sk;
+	}
 
 	return __inet_lookup(dev_net(skb_dst(skb)->dev), hashinfo, skb,
 			     doff, iph->saddr, sport,
-- 
2.17.2 (Apple Git-113)

[PATCH net-next 1/2] tcp/dccp: Move initialisation of refcounted into if block.

[Kuniyuki Iwashima]

The refcounted is initialised at most three times.

  - __inet_lookup_skb() sets it true.
  - skb_steal_sock() is false and __inet_lookup() sets it true.
  - __inet_lookup_established() is false and __inet_lookup() sets it false.

We do not need to initialise refcounted again and again, so we should do
it just before return.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
---
 include/net/inet6_hashtables.h | 11 +++++++----
 include/net/inet_hashtables.h  | 11 +++++++----
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
index fe96bf247aac..9b6c97100d54 100644
--- a/include/net/inet6_hashtables.h
+++ b/include/net/inet6_hashtables.h
@@ -70,9 +70,11 @@ static inline struct sock *__inet6_lookup(struct net *net,
 	struct sock *sk = __inet6_lookup_established(net, hashinfo, saddr,
 						     sport, daddr, hnum,
 						     dif, sdif);
-	*refcounted = true;
-	if (sk)
+	if (sk) {
+		*refcounted = true;
 		return sk;
+	}
+
 	*refcounted = false;
 	return inet6_lookup_listener(net, hashinfo, skb, doff, saddr, sport,
 				     daddr, hnum, dif, sdif);
@@ -87,9 +89,10 @@ static inline struct sock *__inet6_lookup_skb(struct inet_hashinfo *hashinfo,
 {
 	struct sock *sk = skb_steal_sock(skb);
 
-	*refcounted = true;
-	if (sk)
+	if (sk) {
+		*refcounted = true;
 		return sk;
+	}
 
 	return __inet6_lookup(dev_net(skb_dst(skb)->dev), hashinfo, skb,
 			      doff, &ipv6_hdr(skb)->saddr, sport,
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index d0019d3395cf..aa859bf8607b 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -345,9 +345,11 @@ static inline struct sock *__inet_lookup(struct net *net,
 
 	sk = __inet_lookup_established(net, hashinfo, saddr, sport,
 				       daddr, hnum, dif, sdif);
-	*refcounted = true;
-	if (sk)
+	if (sk) {
+		*refcounted = true;
 		return sk;
+	}
+
 	*refcounted = false;
 	return __inet_lookup_listener(net, hashinfo, skb, doff, saddr,
 				      sport, daddr, hnum, dif, sdif);
@@ -382,9 +384,10 @@ static inline struct sock *__inet_lookup_skb(struct inet_hashinfo *hashinfo,
 	struct sock *sk = skb_steal_sock(skb);
 	const struct iphdr *iph = ip_hdr(skb);
 
-	*refcounted = true;
-	if (sk)
+	if (sk) {
+		*refcounted = true;
 		return sk;
+	}
 
 	return __inet_lookup(dev_net(skb_dst(skb)->dev), hashinfo, skb,
 			     doff, iph->saddr, sport,
-- 
2.17.2 (Apple Git-113)

[PATCH net-next 2/2] tcp/dccp: Remove unnecessary initialization of refcounted.

[Kuniyuki Iwashima]

When we get a TCP_NEW_SYN_RECV/DCCP_NEW_SYN_RECV socket by
__inet_lookup_skb(), refcounted is already set true, so it is not
necessary to do it again.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
---
 net/dccp/ipv4.c     | 1 -
 net/dccp/ipv6.c     | 1 -
 net/ipv4/tcp_ipv4.c | 1 -
 net/ipv6/tcp_ipv6.c | 1 -
 4 files changed, 4 deletions(-)

diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index d19557c6d04b..c63b6bd68284 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -838,7 +838,6 @@ static int dccp_v4_rcv(struct sk_buff *skb)
 			goto lookup;
 		}
 		sock_hold(sk);
-		refcounted = true;
 		nsk = dccp_check_req(sk, skb, req);
 		if (!nsk) {
 			reqsk_put(req);
diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index 1e5e08cc0bfc..b3ca9b1ef32a 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -740,7 +740,6 @@ static int dccp_v6_rcv(struct sk_buff *skb)
 			goto lookup;
 		}
 		sock_hold(sk);
-		refcounted = true;
 		nsk = dccp_check_req(sk, skb, req);
 		if (!nsk) {
 			reqsk_put(req);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index df1166b76126..b59a89d8fa69 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1931,7 +1931,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		 * as we might lose it too soon.
 		 */
 		sock_hold(sk);
-		refcounted = true;
 		nsk = NULL;
 		if (!tcp_filter(sk, skb)) {
 			th = (const struct tcphdr *)skb->data;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index eaf09e6b7844..3a587c40ca52 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1603,7 +1603,6 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 			goto lookup;
 		}
 		sock_hold(sk);
-		refcounted = true;
 		nsk = NULL;
 		if (!tcp_filter(sk, skb)) {
 			th = (const struct tcphdr *)skb->data;
-- 
2.17.2 (Apple Git-113)

[PATCH net-next 2/2] tcp/dccp: Remove unnecessary initialization of refcounted.

[Kuniyuki Iwashima]

When we get a TCP_NEW_SYN_RECV/DCCP_NEW_SYN_RECV socket by
__inet_lookup_skb(), refcounted is already set true, so it is not
necessary to do it again.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
---
 net/dccp/ipv4.c     | 1 -
 net/dccp/ipv6.c     | 1 -
 net/ipv4/tcp_ipv4.c | 1 -
 net/ipv6/tcp_ipv6.c | 1 -
 4 files changed, 4 deletions(-)

diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index d19557c6d04b..c63b6bd68284 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -838,7 +838,6 @@ static int dccp_v4_rcv(struct sk_buff *skb)
 			goto lookup;
 		}
 		sock_hold(sk);
-		refcounted = true;
 		nsk = dccp_check_req(sk, skb, req);
 		if (!nsk) {
 			reqsk_put(req);
diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index 1e5e08cc0bfc..b3ca9b1ef32a 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -740,7 +740,6 @@ static int dccp_v6_rcv(struct sk_buff *skb)
 			goto lookup;
 		}
 		sock_hold(sk);
-		refcounted = true;
 		nsk = dccp_check_req(sk, skb, req);
 		if (!nsk) {
 			reqsk_put(req);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index df1166b76126..b59a89d8fa69 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1931,7 +1931,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
 		 * as we might lose it too soon.
 		 */
 		sock_hold(sk);
-		refcounted = true;
 		nsk = NULL;
 		if (!tcp_filter(sk, skb)) {
 			th = (const struct tcphdr *)skb->data;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index eaf09e6b7844..3a587c40ca52 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1603,7 +1603,6 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
 			goto lookup;
 		}
 		sock_hold(sk);
-		refcounted = true;
 		nsk = NULL;
 		if (!tcp_filter(sk, skb)) {
 			th = (const struct tcphdr *)skb->data;
-- 
2.17.2 (Apple Git-113)

Re: [PATCH net-next 1/2] tcp/dccp: Move initialisation of refcounted into if block.

[Eric Dumazet]

On Mon, Mar 23, 2020 at 11:18 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>
> The refcounted is initialised at most three times.
>
>   - __inet_lookup_skb() sets it true.
>   - skb_steal_sock() is false and __inet_lookup() sets it true.
>   - __inet_lookup_established() is false and __inet_lookup() sets it false.
>
> We do not need to initialise refcounted again and again, so we should do
> it just before return.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> ---

Well, I do not believe this patch (and the following one) makes things
more readable.

I doubt setting a boolean in a register or a stack variable has any cost,
I prefer letting the compiler optimize this.

The ehash lookup cost is at least 2 or 3 cache lines, this is the
major contribution.

Re: [PATCH net-next 1/2] tcp/dccp: Move initialisation of refcounted into if block.

[Eric Dumazet]

On Mon, Mar 23, 2020 at 11:18 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>
> The refcounted is initialised at most three times.
>
>   - __inet_lookup_skb() sets it true.
>   - skb_steal_sock() is false and __inet_lookup() sets it true.
>   - __inet_lookup_established() is false and __inet_lookup() sets it false.
>
> We do not need to initialise refcounted again and again, so we should do
> it just before return.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> ---

Well, I do not believe this patch (and the following one) makes things
more readable.

I doubt setting a boolean in a register or a stack variable has any cost,
I prefer letting the compiler optimize this.

The ehash lookup cost is at least 2 or 3 cache lines, this is the
major contribution.

Re: [PATCH net-next 2/2] tcp/dccp: Remove unnecessary initialization of refcounted.

[Eric Dumazet]

On Mon, Mar 23, 2020 at 11:22 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>
> When we get a TCP_NEW_SYN_RECV/DCCP_NEW_SYN_RECV socket by
> __inet_lookup_skb(), refcounted is already set true, so it is not
> necessary to do it again.

This changelog is absolutely not accurate.

sk is a listener here.
(because sk was set to req->rsk_listener;)

Please do not add confusion by mixing different things.

I prefer not relying on the old value of 'refcounted', since we
switched sk value.

Note that we call reqsk_put(req); regardless of 'refcounted'

I would rather not change this code and make future backports more complicated.

Thanks.

Re: [PATCH net-next 2/2] tcp/dccp: Remove unnecessary initialization of refcounted.

[Eric Dumazet]

On Mon, Mar 23, 2020 at 11:22 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>
> When we get a TCP_NEW_SYN_RECV/DCCP_NEW_SYN_RECV socket by
> __inet_lookup_skb(), refcounted is already set true, so it is not
> necessary to do it again.

This changelog is absolutely not accurate.

sk is a listener here.
(because sk was set to req->rsk_listener;)

Please do not add confusion by mixing different things.

I prefer not relying on the old value of 'refcounted', since we
switched sk value.

Note that we call reqsk_put(req); regardless of 'refcounted'

I would rather not change this code and make future backports more complicated.

Thanks.

Re: [PATCH net-next 2/2] tcp/dccp: Remove unnecessary initialization

[Kuniyuki Iwashima]

From:   Eric Dumazet <edumazet@google.com>
Date:   Mon, 23 Mar 2020 11:47:17 -0700
> On Mon, Mar 23, 2020 at 11:22 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> >
> > When we get a TCP_NEW_SYN_RECV/DCCP_NEW_SYN_RECV socket by
> > __inet_lookup_skb(), refcounted is already set true, so it is not
> > necessary to do it again.
> 
> This changelog is absolutely not accurate.
> 
> sk is a listener here.
> (because sk was set to req->rsk_listener;)
> 
> Please do not add confusion by mixing different things.
> 
> I prefer not relying on the old value of 'refcounted', since we
> switched sk value.
> 
> Note that we call reqsk_put(req); regardless of 'refcounted'

Certainly, the refcounted has diffrent meaning in the context, sorry. 


> I would rather not change this code and make future backports more complicated.
> 
> Thanks.

I did not think about backports, thank you!

Re: [PATCH net-next 2/2] tcp/dccp: Remove unnecessary initialization

[Kuniyuki Iwashima]

From:   Eric Dumazet <edumazet@google.com>
Date:   Mon, 23 Mar 2020 11:47:17 -0700
> On Mon, Mar 23, 2020 at 11:22 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> >
> > When we get a TCP_NEW_SYN_RECV/DCCP_NEW_SYN_RECV socket by
> > __inet_lookup_skb(), refcounted is already set true, so it is not
> > necessary to do it again.
> 
> This changelog is absolutely not accurate.
> 
> sk is a listener here.
> (because sk was set to req->rsk_listener;)
> 
> Please do not add confusion by mixing different things.
> 
> I prefer not relying on the old value of 'refcounted', since we
> switched sk value.
> 
> Note that we call reqsk_put(req); regardless of 'refcounted'

Certainly, the refcounted has diffrent meaning in the context, sorry. 


> I would rather not change this code and make future backports more complicated.
> 
> Thanks.

I did not think about backports, thank you!

Re: [PATCH net-next 1/2] tcp/dccp: Move initialisation of refcounted

[Kuniyuki Iwashima]

From:   Eric Dumazet <edumazet@google.com>
Date:   Mon, 23 Mar 2020 11:45:48 -0700
> On Mon, Mar 23, 2020 at 11:18 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> >
> > The refcounted is initialised at most three times.
> >
> >   - __inet_lookup_skb() sets it true.
> >   - skb_steal_sock() is false and __inet_lookup() sets it true.
> >   - __inet_lookup_established() is false and __inet_lookup() sets it false.
> >
> > We do not need to initialise refcounted again and again, so we should do
> > it just before return.
> >
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > ---
> 
> Well, I do not believe this patch (and the following one) makes things
> more readable.
> 
> I doubt setting a boolean in a register or a stack variable has any cost,
> I prefer letting the compiler optimize this.
> 
> The ehash lookup cost is at least 2 or 3 cache lines, this is the
> major contribution.

I confirmed that GCC (v7.3.1) does not make any change with this patch,
but the original code is bit confusing. I think this patch makes the code
bit more readable so that someone (like me...) will not doubt about
complier optimization and write patches.

I will respin only this patch because the changelog can be incorrect by
complier optimization.

Thanks.

Re: [PATCH net-next 1/2] tcp/dccp: Move initialisation of refcounted

[Kuniyuki Iwashima]

From:   Eric Dumazet <edumazet@google.com>
Date:   Mon, 23 Mar 2020 11:45:48 -0700
> On Mon, Mar 23, 2020 at 11:18 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> >
> > The refcounted is initialised at most three times.
> >
> >   - __inet_lookup_skb() sets it true.
> >   - skb_steal_sock() is false and __inet_lookup() sets it true.
> >   - __inet_lookup_established() is false and __inet_lookup() sets it false.
> >
> > We do not need to initialise refcounted again and again, so we should do
> > it just before return.
> >
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > ---
> 
> Well, I do not believe this patch (and the following one) makes things
> more readable.
> 
> I doubt setting a boolean in a register or a stack variable has any cost,
> I prefer letting the compiler optimize this.
> 
> The ehash lookup cost is at least 2 or 3 cache lines, this is the
> major contribution.

I confirmed that GCC (v7.3.1) does not make any change with this patch,
but the original code is bit confusing. I think this patch makes the code
bit more readable so that someone (like me...) will not doubt about
complier optimization and write patches.

I will respin only this patch because the changelog can be incorrect by
complier optimization.

Thanks.