[PATCH net-next] Revert "net: init sk_cookie for inet socket"

This revert commit <c6849a3ac17e> ("net: init sk_cookie for inet socket")

Per discussion with Eric.

Signed-off-by: Yafang Shao <redacted>
---
 include/linux/sock_diag.h | 9 ---------
 net/ipv4/tcp_input.c      | 8 +-------
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/include/linux/sock_diag.h b/include/linux/sock_diag.h
index 5c916e6..15fe980 100644
--- a/include/linux/sock_diag.h
+++ b/include/linux/sock_diag.h

@@ -25,15 +25,6 @@ struct sock_diag_handler {
 void sock_diag_register_inet_compat(int (*fn)(struct sk_buff *skb, struct nlmsghdr *nlh));
 void sock_diag_unregister_inet_compat(int (*fn)(struct sk_buff *skb, struct nlmsghdr *nlh));
 
-static inline
-void sock_init_cookie(struct sock *sk)
-{
-	u64 res;
-
-	res = atomic64_inc_return(&sock_net(sk)->cookie_gen);
-	atomic64_set(&sk->sk_cookie, res);
-}
-
 u64 sock_gen_cookie(struct sock *sk);
 int sock_diag_check_cookie(struct sock *sk, const __u32 *cookie);
 void sock_diag_save_cookie(struct sock *sk, __u32 *cookie);

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 17b7858..5a17cfc 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c

@@ -78,7 +78,6 @@
 #include <linux/errqueue.h>
 #include <trace/events/tcp.h>
 #include <linux/static_key.h>
-#include <linux/sock_diag.h>
 
 int sysctl_tcp_max_orphans __read_mostly = NR_FILE;
 

@@ -6191,15 +6190,10 @@ struct request_sock *inet_reqsk_alloc(const struct request_sock_ops *ops,
 #if IS_ENABLED(CONFIG_IPV6)
 		ireq->pktopts = NULL;
 #endif
+		atomic64_set(&ireq->ir_cookie, 0);
 		ireq->ireq_state = TCP_NEW_SYN_RECV;
 		write_pnet(&ireq->ireq_net, sock_net(sk_listener));
 		ireq->ireq_family = sk_listener->sk_family;
-
-		BUILD_BUG_ON(offsetof(struct inet_request_sock, ir_cookie) !=
-					offsetof(struct sock, sk_cookie));
-		BUILD_BUG_ON(offsetof(struct inet_request_sock, ireq_net) !=
-					offsetof(struct sock, sk_net));
-		sock_init_cookie((struct sock *)ireq);
 	}
 
 	return req;

-- 
1.8.3.1

On 04/24/2018 05:05 AM, Yafang Shao wrote:

This revert commit <c6849a3ac17e> ("net: init sk_cookie for inet socket")

Per discussion with Eric.

I suggest you include a bit more details, about cache line false sharing.

Thanks.

On Tue, Apr 24, 2018 at 8:38 PM, Eric Dumazet [off-list ref] wrote:

On 04/24/2018 05:05 AM, Yafang Shao wrote:

quoted

This revert commit <c6849a3ac17e> ("net: init sk_cookie for inet socket")

Per discussion with Eric.

I suggest you include a bit more details, about cache line false sharing.

Thanks.

OK.
will update it with your message shared with me.

Thanks
Yafang

On Tue, Apr 24, 2018 at 8:38 PM, Eric Dumazet [off-list ref] wrote:

On 04/24/2018 05:05 AM, Yafang Shao wrote:

quoted

This revert commit <c6849a3ac17e> ("net: init sk_cookie for inet socket")

Per discussion with Eric.

I suggest you include a bit more details, about cache line false sharing.

Coud we adjust the struct common to avoid such kind of cache line
false sharing ?
I mean removing "atomic64_t  skc_cookie;" from struct sock_common and
place it in struct inet_sock ?

Thanks
Yafang

On 04/24/2018 08:12 AM, Yafang Shao wrote:

On Tue, Apr 24, 2018 at 8:38 PM, Eric Dumazet [off-list ref] wrote:

quoted

On 04/24/2018 05:05 AM, Yafang Shao wrote:

quoted

This revert commit <c6849a3ac17e> ("net: init sk_cookie for inet socket")

Per discussion with Eric.

I suggest you include a bit more details, about cache line false sharing.

Coud we adjust the struct common to avoid such kind of cache line
false sharing ?
I mean removing "atomic64_t  skc_cookie;" from struct sock_common and
place it in struct inet_sock ?

The false sharing is not there, it is on net->cookie_gen

On Tue, Apr 24, 2018 at 11:49 PM, Eric Dumazet [off-list ref] wrote:

On 04/24/2018 08:12 AM, Yafang Shao wrote:

quoted

On Tue, Apr 24, 2018 at 8:38 PM, Eric Dumazet [off-list ref] wrote:

quoted

On 04/24/2018 05:05 AM, Yafang Shao wrote:

quoted

This revert commit <c6849a3ac17e> ("net: init sk_cookie for inet socket")

Per discussion with Eric.

I suggest you include a bit more details, about cache line false sharing.

Coud we adjust the struct common to avoid such kind of cache line
false sharing ?
I mean removing "atomic64_t  skc_cookie;" from struct sock_common and
place it in struct inet_sock ?

The false sharing is not there, it is on net->cookie_gen

Yes.
This is the current issue.
May be we should adjust struct net as well.

Regarding sk_cookie, as it is only used by inet_sock now, may be it is
better placed in srtuct inet_sock ?

Thanks
Yafang

On 04/24/2018 08:59 AM, Yafang Shao wrote:

On Tue, Apr 24, 2018 at 11:49 PM, Eric Dumazet [off-list ref] wrote:

quoted

On 04/24/2018 08:12 AM, Yafang Shao wrote:

quoted

On Tue, Apr 24, 2018 at 8:38 PM, Eric Dumazet [off-list ref] wrote:

quoted

On 04/24/2018 05:05 AM, Yafang Shao wrote:

quoted

This revert commit <c6849a3ac17e> ("net: init sk_cookie for inet socket")

Per discussion with Eric.

I suggest you include a bit more details, about cache line false sharing.

Coud we adjust the struct common to avoid such kind of cache line
false sharing ?
I mean removing "atomic64_t  skc_cookie;" from struct sock_common and
place it in struct inet_sock ?

The false sharing is not there, it is on net->cookie_gen

Yes.
This is the current issue.
May be we should adjust struct net as well.

This field will still need to be modified by many cpus.

Its exact placement in memory wont avoid false sharing and stalls.

Regarding sk_cookie, as it is only used by inet_sock now, may be it is
better placed in srtuct inet_sock ?

You are mistaken.

It is used on all sockets really (including request_sock and timewait)

ss -temoia   will give you socket ids for all sockets types.