| Message ID | 20200204153534.23813-1-fw@strlen.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | subflow: place outgoing connections on the join list | expand |
On Tue, 2020-02-04 at 16:35 +0100, Florian Westphal wrote: > squashto: subflow: place further subflows on new 'join_list' > > in case the subflow can't be established (timeout, reset) the subflow > socket is leaked. > > Add it to the join list for now. This isn't nice because the socket > will be added while its known to not be established, and its not removed > at any time even if its not useable. > > The list_del_init() call can be removed because the subflow context > ->node is still unused / not on a list in all cases. > > On a different note: > > I think it makes sense to remove the 'splice join to conn list' approach > and instead have the worker check the join and conn lists: > > 1. subflows are added from join to conn list once they are established. > 2. subflows on join list that are still in SYN_SENT are ignored > 3. subflows on join and/or conn list that are closed already are removed > and free'd. > > Alternatively, we could probably extend the sk_state_change callback > to do this and keep checking outside of send/receive code paths. > > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > net/mptcp/subflow.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index 547d5ffef070..154fb2b42243 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c > @@ -821,11 +821,14 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local, > if (err && err != -EINPROGRESS) > goto failed; > > + spin_lock_bh(&msk->join_list_lock); > + list_add_tail(&subflow->node, &msk->join_list); > + spin_unlock_bh(&msk->join_list_lock); > + > release_sock(sk); > return err; > > failed: > - list_del_init(&subflow->node); > release_sock(sk); > sock_release(sf); > return err; whooops ... this one fell outside my radar for a long time. mp_join self-tests (WIP) demonstrated the need for this one! LGTM, thanks Florian! /P
Hi Florian, Paolo, On 20/02/2020 17:00, Paolo Abeni wrote: > On Tue, 2020-02-04 at 16:35 +0100, Florian Westphal wrote: >> squashto: subflow: place further subflows on new 'join_list' >> >> in case the subflow can't be established (timeout, reset) the subflow >> socket is leaked. >> >> Add it to the join list for now. This isn't nice because the socket >> will be added while its known to not be established, and its not removed >> at any time even if its not useable. >> >> The list_del_init() call can be removed because the subflow context >> ->node is still unused / not on a list in all cases. >> >> On a different note: >> >> I think it makes sense to remove the 'splice join to conn list' approach >> and instead have the worker check the join and conn lists: >> >> 1. subflows are added from join to conn list once they are established. >> 2. subflows on join list that are still in SYN_SENT are ignored >> 3. subflows on join and/or conn list that are closed already are removed >> and free'd. >> >> Alternatively, we could probably extend the sk_state_change callback >> to do this and keep checking outside of send/receive code paths. [...] > whooops ... this one fell outside my radar for a long time. > > mp_join self-tests (WIP) demonstrated the need for this one! > > LGTM, thanks Florian! Sorry for the delay, I missed that too at first and got other things scheduled after. Please insist if a patch has been reviewed but not applied! :) - f20d350cb8d5: "squashed" in "subflow: place further subflows on new 'join_list'" - 676a5c51ff36..8b57e57b1ba4: result Tests + export are going to be launched soon. Cheers, Matt
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 547d5ffef070..154fb2b42243 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -821,11 +821,14 @@ int mptcp_subflow_connect(struct sock *sk, struct sockaddr *local, if (err && err != -EINPROGRESS) goto failed; + spin_lock_bh(&msk->join_list_lock); + list_add_tail(&subflow->node, &msk->join_list); + spin_unlock_bh(&msk->join_list_lock); + release_sock(sk); return err; failed: - list_del_init(&subflow->node); release_sock(sk); sock_release(sf); return err;
squashto: subflow: place further subflows on new 'join_list' in case the subflow can't be established (timeout, reset) the subflow socket is leaked. Add it to the join list for now. This isn't nice because the socket will be added while its known to not be established, and its not removed at any time even if its not useable. The list_del_init() call can be removed because the subflow context ->node is still unused / not on a list in all cases. On a different note: I think it makes sense to remove the 'splice join to conn list' approach and instead have the worker check the join and conn lists: 1. subflows are added from join to conn list once they are established. 2. subflows on join list that are still in SYN_SENT are ignored 3. subflows on join and/or conn list that are closed already are removed and free'd. Alternatively, we could probably extend the sk_state_change callback to do this and keep checking outside of send/receive code paths. Signed-off-by: Florian Westphal <fw@strlen.de> --- net/mptcp/subflow.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)