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

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

[Yafang Shao]

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

Per discussion with Eric.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 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

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

[Eric Dumazet]

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.

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

[Yafang Shao]

On Tue, Apr 24, 2018 at 8:38 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> 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.
>

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

Thanks
Yafang

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

[Yafang Shao]

On Tue, Apr 24, 2018 at 8:38 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> 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.
>

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

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

[Eric Dumazet]

On 04/24/2018 08:12 AM, Yafang Shao wrote:
> On Tue, Apr 24, 2018 at 8:38 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>> 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.
>>
> 
> 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

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

[Yafang Shao]

On Tue, Apr 24, 2018 at 11:49 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 04/24/2018 08:12 AM, Yafang Shao wrote:
>> On Tue, Apr 24, 2018 at 8:38 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>
>>>
>>> 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.
>>>
>>
>> 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

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

[Eric Dumazet]

On 04/24/2018 08:59 AM, Yafang Shao wrote:
> On Tue, Apr 24, 2018 at 11:49 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>> On 04/24/2018 08:12 AM, Yafang Shao wrote:
>>> On Tue, Apr 24, 2018 at 8:38 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>>
>>>>
>>>> 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.
>>>>
>>>
>>> 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.