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

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

[Yafang Shao]

With sk_cookie we can identify a socket, that is very helpful for
traceing and statistic, i.e. tcp tracepiont and ebpf.
So we'd better init it by default for inet socket.
When using it, we just need call atomic64_read(&sk->sk_cookie).

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/sock_diag.h | 9 +++++++++
 net/ipv4/tcp_input.c      | 8 +++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/sock_diag.h b/include/linux/sock_diag.h
index 15fe980..5c916e6 100644
--- a/include/linux/sock_diag.h
+++ b/include/linux/sock_diag.h
@@ -25,6 +25,15 @@ 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 0396fb9..e6a64a3 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -78,6 +78,7 @@
 #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;
 
@@ -6188,10 +6189,15 @@ 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] net: init sk_cookie for inet socket

[David Miller]

From: Yafang Shao <laoar.shao@gmail.com>
Date: Sun, 22 Apr 2018 21:50:04 +0800

> With sk_cookie we can identify a socket, that is very helpful for
> traceing and statistic, i.e. tcp tracepiont and ebpf.
> So we'd better init it by default for inet socket.
> When using it, we just need call atomic64_read(&sk->sk_cookie).
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Applied, thank you.

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

[Eric Dumazet]

On 04/23/2018 08:58 AM, David Miller wrote:
> From: Yafang Shao <laoar.shao@gmail.com>
> Date: Sun, 22 Apr 2018 21:50:04 +0800
> 
>> With sk_cookie we can identify a socket, that is very helpful for
>> traceing and statistic, i.e. tcp tracepiont and ebpf.
>> So we'd better init it by default for inet socket.
>> When using it, we just need call atomic64_read(&sk->sk_cookie).
>>
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> 
> Applied, thank you.
> 

This is adding yet another atomic_inc on a global cache line.

Most applications do not need the cookie being ever set.

The existing mechanism was fine. Set it on demand.

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

[Yafang Shao]

On Tue, Apr 24, 2018 at 12:09 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 04/23/2018 08:58 AM, David Miller wrote:
>> From: Yafang Shao <laoar.shao@gmail.com>
>> Date: Sun, 22 Apr 2018 21:50:04 +0800
>>
>>> With sk_cookie we can identify a socket, that is very helpful for
>>> traceing and statistic, i.e. tcp tracepiont and ebpf.
>>> So we'd better init it by default for inet socket.
>>> When using it, we just need call atomic64_read(&sk->sk_cookie).
>>>
>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>
>> Applied, thank you.
>>
>
> This is adding yet another atomic_inc on a global cache line.
>

That's a trade-off.

> Most applications do not need the cookie being ever set.
>
> The existing mechanism was fine. Set it on demand.

There are some drawback in the existing mechanism.
- we have to set the net->cookie_gen and then sk->sk_cookie when we
want to get the sk_cookie, that's a little expensive as well.
  After that change, sock_gen_cookie() could be replaced by
atomic64_read(&sk->sk_cookie) in most places.

- If the application want to get the sk_cookie, it must set it first.
   What if the application don't have the permision to write?
   Furthermore, maybe it is a security concern ?


Thanks
Yafang

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

[Eric Dumazet]

On 04/23/2018 09:39 PM, Yafang Shao wrote:
> On Tue, Apr 24, 2018 at 12:09 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>> On 04/23/2018 08:58 AM, David Miller wrote:
>>> From: Yafang Shao <laoar.shao@gmail.com>
>>> Date: Sun, 22 Apr 2018 21:50:04 +0800
>>>
>>>> With sk_cookie we can identify a socket, that is very helpful for
>>>> traceing and statistic, i.e. tcp tracepiont and ebpf.
>>>> So we'd better init it by default for inet socket.
>>>> When using it, we just need call atomic64_read(&sk->sk_cookie).
>>>>
>>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>>
>>> Applied, thank you.
>>>
>>
>> This is adding yet another atomic_inc on a global cache line.
>>
> 
> That's a trade-off.
> 
>> Most applications do not need the cookie being ever set.
>>
>> The existing mechanism was fine. Set it on demand.
> 
> There are some drawback in the existing mechanism.
> - we have to set the net->cookie_gen and then sk->sk_cookie when we
> want to get the sk_cookie, that's a little expensive as well.

Same cost.

>   After that change, sock_gen_cookie() could be replaced by
> atomic64_read(&sk->sk_cookie) in most places.

Same cost than the helper.

> 
> - If the application want to get the sk_cookie, it must set it first.
>    What if the application don't have the permision to write?
>    Furthermore, maybe it is a security concern ?


Maybe ? Please elaborate.

Your patch destroys SYNFLOOD behavior.

I have spent months of work solving the SYNFLOOD behavior, your patch crushes it.

I am not that happy.

Please revert this patch.

Thank you.

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

[Yafang Shao]

On Tue, Apr 24, 2018 at 7:41 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 04/23/2018 09:39 PM, Yafang Shao wrote:
>> On Tue, Apr 24, 2018 at 12:09 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>
>>>
>>> On 04/23/2018 08:58 AM, David Miller wrote:
>>>> From: Yafang Shao <laoar.shao@gmail.com>
>>>> Date: Sun, 22 Apr 2018 21:50:04 +0800
>>>>
>>>>> With sk_cookie we can identify a socket, that is very helpful for
>>>>> traceing and statistic, i.e. tcp tracepiont and ebpf.
>>>>> So we'd better init it by default for inet socket.
>>>>> When using it, we just need call atomic64_read(&sk->sk_cookie).
>>>>>
>>>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>>>
>>>> Applied, thank you.
>>>>
>>>
>>> This is adding yet another atomic_inc on a global cache line.
>>>
>>
>> That's a trade-off.
>>
>>> Most applications do not need the cookie being ever set.
>>>
>>> The existing mechanism was fine. Set it on demand.
>>
>> There are some drawback in the existing mechanism.
>> - we have to set the net->cookie_gen and then sk->sk_cookie when we
>> want to get the sk_cookie, that's a little expensive as well.
>
> Same cost.
>
>>   After that change, sock_gen_cookie() could be replaced by
>> atomic64_read(&sk->sk_cookie) in most places.
>
> Same cost than the helper.
>
>>
>> - If the application want to get the sk_cookie, it must set it first.
>>    What if the application don't have the permision to write?
>>    Furthermore, maybe it is a security concern ?
>
>
> Maybe ? Please elaborate.
>
> Your patch destroys SYNFLOOD behavior.
>
> I have spent months of work solving the SYNFLOOD behavior, your patch crushes it.
>

Could you pls. explain the issue to me ?

> I am not that happy.
>
> Please revert this patch.
>

OK. I will revert it.

> Thank you.

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

[Eric Dumazet]

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

> 
> Could you pls. explain the issue to me ?

Just run a synflood test on your host, it will definitely show the atomic
consuming most cpu cycles in inet_reqsk_alloc(), because of huge contention
on a cache line shared by all cpus.

Performance is reduced from ~5 Mpps to ~3.8 Mpps with 16 RX queues on my host.

atomic64_inc_return(&sock_net(sk)->cookie_gen) was not meant to be used in the
normal case (when a socket cookie is not ever requested/needed)