From: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
To: <kuniyu@amazon.co.jp>
Cc: <kuni1840@gmail.com>, <netdev@vger.kernel.org>,
<bpf@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 bpf-next 01/11] tcp: Keep TCP_CLOSE sockets in the reuseport group.
Date: Sun, 6 Dec 2020 13:38:23 +0900 [thread overview]
Message-ID: <20201206043823.27524-1-kuniyu@amazon.co.jp> (raw)
In-Reply-To: <20201205013103.eo5chfx57kf25pz3@kafai-mbp.dhcp.thefacebook.com>
I'm sending this mail just for logging because I failed to send mails only
to LKML, netdev, and bpf yesterday.
From: Martin KaFai Lau <kafai@fb.com>
Date: Fri, 4 Dec 2020 17:31:03 -0800
> On Tue, Dec 01, 2020 at 11:44:08PM +0900, Kuniyuki Iwashima wrote:
> > This patch is a preparation patch to migrate incoming connections in the
> > later commits and adds a field (num_closed_socks) to the struct
> > sock_reuseport to keep TCP_CLOSE sockets in the reuseport group.
> >
> > When we close a listening socket, to migrate its connections to another
> > listener in the same reuseport group, we have to handle two kinds of child
> > sockets. One is that a listening socket has a reference to, and the other
> > is not.
> >
> > The former is the TCP_ESTABLISHED/TCP_SYN_RECV sockets, and they are in the
> > accept queue of their listening socket. So, we can pop them out and push
> > them into another listener's queue at close() or shutdown() syscalls. On
> > the other hand, the latter, the TCP_NEW_SYN_RECV socket is during the
> > three-way handshake and not in the accept queue. Thus, we cannot access
> > such sockets at close() or shutdown() syscalls. Accordingly, we have to
> > migrate immature sockets after their listening socket has been closed.
> >
> > Currently, if their listening socket has been closed, TCP_NEW_SYN_RECV
> > sockets are freed at receiving the final ACK or retransmitting SYN+ACKs. At
> > that time, if we could select a new listener from the same reuseport group,
> > no connection would be aborted. However, it is impossible because
> > reuseport_detach_sock() sets NULL to sk_reuseport_cb and forbids access to
> > the reuseport group from closed sockets.
> >
> > This patch allows TCP_CLOSE sockets to remain in the reuseport group and to
> > have access to it while any child socket references to them. The point is
> > that reuseport_detach_sock() is called twice from inet_unhash() and
> > sk_destruct(). At first, it moves the socket backwards in socks[] and
> > increments num_closed_socks. Later, when all migrated connections are
> > accepted, it removes the socket from socks[], decrements num_closed_socks,
> > and sets NULL to sk_reuseport_cb.
> >
> > By this change, closed sockets can keep sk_reuseport_cb until all child
> > requests have been freed or accepted. Consequently calling listen() after
> > shutdown() can cause EADDRINUSE or EBUSY in reuseport_add_sock() or
> > inet_csk_bind_conflict() which expect that such sockets should not have the
> > reuseport group. Therefore, this patch also loosens such validation rules
> > so that the socket can listen again if it has the same reuseport group with
> > other listening sockets.
> >
> > Reviewed-by: Benjamin Herrenschmidt <benh@amazon.com>
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > ---
> > include/net/sock_reuseport.h | 5 ++-
> > net/core/sock_reuseport.c | 79 +++++++++++++++++++++++++++------
> > net/ipv4/inet_connection_sock.c | 7 ++-
> > 3 files changed, 74 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/net/sock_reuseport.h b/include/net/sock_reuseport.h
> > index 505f1e18e9bf..0e558ca7afbf 100644
> > --- a/include/net/sock_reuseport.h
> > +++ b/include/net/sock_reuseport.h
> > @@ -13,8 +13,9 @@ extern spinlock_t reuseport_lock;
> > struct sock_reuseport {
> > struct rcu_head rcu;
> >
> > - u16 max_socks; /* length of socks */
> > - u16 num_socks; /* elements in socks */
> > + u16 max_socks; /* length of socks */
> > + u16 num_socks; /* elements in socks */
> > + u16 num_closed_socks; /* closed elements in socks */
> > /* The last synq overflow event timestamp of this
> > * reuse->socks[] group.
> > */
> > diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
> > index bbdd3c7b6cb5..fd133516ac0e 100644
> > --- a/net/core/sock_reuseport.c
> > +++ b/net/core/sock_reuseport.c
> > @@ -98,16 +98,21 @@ static struct sock_reuseport *reuseport_grow(struct sock_reuseport *reuse)
> > return NULL;
> >
> > more_reuse->num_socks = reuse->num_socks;
> > + more_reuse->num_closed_socks = reuse->num_closed_socks;
> > more_reuse->prog = reuse->prog;
> > more_reuse->reuseport_id = reuse->reuseport_id;
> > more_reuse->bind_inany = reuse->bind_inany;
> > more_reuse->has_conns = reuse->has_conns;
> > + more_reuse->synq_overflow_ts = READ_ONCE(reuse->synq_overflow_ts);
> >
> > memcpy(more_reuse->socks, reuse->socks,
> > reuse->num_socks * sizeof(struct sock *));
> > - more_reuse->synq_overflow_ts = READ_ONCE(reuse->synq_overflow_ts);
> > + memcpy(more_reuse->socks +
> > + (more_reuse->max_socks - more_reuse->num_closed_socks),
> > + reuse->socks + reuse->num_socks,
> > + reuse->num_closed_socks * sizeof(struct sock *));
> >
> > - for (i = 0; i < reuse->num_socks; ++i)
> > + for (i = 0; i < reuse->max_socks; ++i)
> > rcu_assign_pointer(reuse->socks[i]->sk_reuseport_cb,
> > more_reuse);
> >
> > @@ -129,6 +134,25 @@ static void reuseport_free_rcu(struct rcu_head *head)
> > kfree(reuse);
> > }
> >
> > +static int reuseport_sock_index(struct sock_reuseport *reuse, struct sock *sk,
> > + bool closed)
> > +{
> > + int left, right;
> > +
> > + if (!closed) {
> > + left = 0;
> > + right = reuse->num_socks;
> > + } else {
> > + left = reuse->max_socks - reuse->num_closed_socks;
> > + right = reuse->max_socks;
> > + }
> > +
> > + for (; left < right; left++)
> > + if (reuse->socks[left] == sk)
> > + return left;
> > + return -1;
> > +}
> > +
> > /**
> > * reuseport_add_sock - Add a socket to the reuseport group of another.
> > * @sk: New socket to add to the group.
> > @@ -153,12 +177,23 @@ int reuseport_add_sock(struct sock *sk, struct sock *sk2, bool bind_inany)
> > lockdep_is_held(&reuseport_lock));
> > old_reuse = rcu_dereference_protected(sk->sk_reuseport_cb,
> > lockdep_is_held(&reuseport_lock));
> > - if (old_reuse && old_reuse->num_socks != 1) {
> > +
> > + if (old_reuse == reuse) {
> > + int i = reuseport_sock_index(reuse, sk, true);
> > +
> > + if (i == -1) {
> When will this happen?
I understood the original code did nothing if the sk was not found in
socks[], so I rewrote it this way, but I also think `i` will never be -1.
If I rewrite, it will be like:
---8<---
for (; left < right; left++)
if (reuse->socks[left] == sk)
break;
return left;
---8<---
> I found the new logic in the closed sk shuffling within socks[] quite
> complicated to read. I can see why the closed sk wants to keep its
> sk->sk_reuseport_cb. However, does it need to stay
> in socks[]?
Currently, I do not use closed sockets in socks[], so the only thing I need
to do seems to be to count num_closed_socks to free struct sock_reuseport.
I will change the code only to keep sk_reuseport_cb and count
num_closed_socks.
(As a side note, I wrote the code while thinking of stack and heap to share
the same array, but I also feel a bit difficult to read.)
next prev parent reply other threads:[~2020-12-06 4:39 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-01 14:44 [PATCH v1 bpf-next 00/11] Socket migration for SO_REUSEPORT Kuniyuki Iwashima
2020-12-01 14:44 ` [PATCH v1 bpf-next 01/11] tcp: Keep TCP_CLOSE sockets in the reuseport group Kuniyuki Iwashima
2020-12-05 1:31 ` Martin KaFai Lau
2020-12-06 4:38 ` Kuniyuki Iwashima [this message]
2020-12-01 14:44 ` [PATCH v1 bpf-next 02/11] bpf: Define migration types for SO_REUSEPORT Kuniyuki Iwashima
2020-12-01 14:44 ` [PATCH v1 bpf-next 03/11] tcp: Migrate TCP_ESTABLISHED/TCP_SYN_RECV sockets in accept queues Kuniyuki Iwashima
2020-12-01 15:25 ` Eric Dumazet
2020-12-03 14:14 ` Kuniyuki Iwashima
2020-12-03 14:31 ` Eric Dumazet
2020-12-03 15:41 ` Kuniyuki Iwashima
2020-12-07 20:33 ` Martin KaFai Lau
2020-12-08 6:31 ` Kuniyuki Iwashima
2020-12-08 7:34 ` Martin KaFai Lau
2020-12-08 8:17 ` Kuniyuki Iwashima
2020-12-09 3:09 ` Martin KaFai Lau
2020-12-09 8:05 ` Kuniyuki Iwashima
2020-12-09 16:57 ` Kuniyuki Iwashima
2020-12-10 1:53 ` Martin KaFai Lau
2020-12-10 5:58 ` Kuniyuki Iwashima
2020-12-10 19:33 ` Martin KaFai Lau
2020-12-14 17:16 ` Kuniyuki Iwashima
2020-12-05 1:42 ` Martin KaFai Lau
2020-12-06 4:41 ` Kuniyuki Iwashima
[not found] ` <20201205160307.91179-1-kuniyu@amazon.co.jp>
2020-12-07 20:14 ` Martin KaFai Lau
2020-12-08 6:27 ` Kuniyuki Iwashima
2020-12-08 8:13 ` Martin KaFai Lau
2020-12-08 9:02 ` Kuniyuki Iwashima
2020-12-08 6:54 ` Martin KaFai Lau
2020-12-08 7:42 ` Kuniyuki Iwashima
2020-12-01 14:44 ` [PATCH v1 bpf-next 04/11] tcp: Migrate TFO requests causing RST during TCP_SYN_RECV Kuniyuki Iwashima
2020-12-01 15:30 ` Eric Dumazet
2020-12-01 14:44 ` [PATCH v1 bpf-next 05/11] tcp: Migrate TCP_NEW_SYN_RECV requests Kuniyuki Iwashima
2020-12-01 15:13 ` Eric Dumazet
2020-12-03 14:12 ` Kuniyuki Iwashima
2020-12-10 0:07 ` Martin KaFai Lau
2020-12-10 5:15 ` Kuniyuki Iwashima
2020-12-10 18:49 ` Martin KaFai Lau
2020-12-14 17:03 ` Kuniyuki Iwashima
2020-12-15 2:58 ` Martin KaFai Lau
2020-12-16 16:41 ` Kuniyuki Iwashima
2020-12-16 22:24 ` Martin KaFai Lau
2020-12-01 14:44 ` [PATCH v1 bpf-next 06/11] bpf: Introduce two attach types for BPF_PROG_TYPE_SK_REUSEPORT Kuniyuki Iwashima
2020-12-02 2:04 ` Andrii Nakryiko
2020-12-02 19:19 ` Martin KaFai Lau
2020-12-03 4:24 ` Martin KaFai Lau
2020-12-03 14:16 ` Kuniyuki Iwashima
2020-12-04 5:56 ` Martin KaFai Lau
2020-12-06 4:32 ` Kuniyuki Iwashima
2020-12-01 14:44 ` [PATCH v1 bpf-next 07/11] libbpf: Set expected_attach_type " Kuniyuki Iwashima
2020-12-01 14:44 ` [PATCH v1 bpf-next 08/11] bpf: Add migration to sk_reuseport_(kern|md) Kuniyuki Iwashima
2020-12-01 14:44 ` [PATCH v1 bpf-next 09/11] bpf: Support bpf_get_socket_cookie_sock() for BPF_PROG_TYPE_SK_REUSEPORT Kuniyuki Iwashima
2020-12-04 19:58 ` Martin KaFai Lau
2020-12-06 4:36 ` Kuniyuki Iwashima
2020-12-01 14:44 ` [PATCH v1 bpf-next 10/11] bpf: Call bpf_run_sk_reuseport() for socket migration Kuniyuki Iwashima
2020-12-01 14:44 ` [PATCH v1 bpf-next 11/11] bpf: Test BPF_SK_REUSEPORT_SELECT_OR_MIGRATE Kuniyuki Iwashima
2020-12-05 1:50 ` Martin KaFai Lau
2020-12-06 4:43 ` Kuniyuki Iwashima
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201206043823.27524-1-kuniyu@amazon.co.jp \
--to=kuniyu@amazon.co.jp \
--cc=bpf@vger.kernel.org \
--cc=kuni1840@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).