Re: [MPTCP] some squashing needed?

Re: [MPTCP] some squashing needed?

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> Hi Paolo,
> 
> On 26/09/2019 19:20, Paolo Abeni wrote:
> > On Thu, 2019-09-26 at 17:05 +0200, Florian Westphal wrote:
> 
> [...]
> 
> >> from Paolo.
> >>
> >> But I figured it would make sense to share this now and discuss
> >> how to proceed from here first.
> > 
> > The above branch is based on mptcp_net-next export branch hash
> > a45bdbe6701b
> 
> Great, still the last one. So everything is in it.
> 
> > There is a small diff vs such hash, see below. Mostily checkpack fixes,
> > a trivial functional fix in mptcp_setsockopt(), and a bunch of likely
> > not interesting debug messages dropped - side note: we may want remove
> > more of them.
> 
> I guess that we indeed need to remove most of these pr_debug() at some
> points. Thank you for having done that!
> Also great to have looked at checkpatch and making sure each commit can
> compile :-)
> 
> For the fix in mptcp_setsockopt(), was it due to a bad "squash"?

No, that is intentional -- I don't think returning 0 when nothing
was done is good.

I can fold Peters mptcp prefix patch for crypto where needed and push a
v2, let me know.

Thanks.

Re: [MPTCP] some squashing needed?

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> On 27/09/2019 17:51, Florian Westphal wrote:
> > I can fold Peters mptcp prefix patch for crypto where needed and push a
> > v2, let me know.
> 
> I just pushed a new tree on mptcp_net-next repo on Github based on your
> branch :)

Awesome.

> I didn't want to block anybody but I can re-create a new one again later
> if needed!

Great.

> Also I don't mind squashing this patch and also do the same for the
> remaining functions (subflow_ ones I guess). I can do that on Monday if
> it is OK for you.

Sure.

I squashed Peters patch, you can get a branch with it folded by pulling
git://git.breakpoint.cc/fw/mptcp-next.git export_squash_11

The diff between current export and this branch is Peters patch, nothing
else.

If you could prefix the path management api that would be great (to avoid
confusion with 'power management') subflow would be good too.

> Also, before recreating the new tree, I remove all (Git) refs to the
> previous ones. If someone uses our repo with Topgit (we never know :) ),
> please do the same on your side!

Sorry, not following.  What would you like me to do?

I plan to work on a v2 of the snmp counters on Monday, esp. dropping all
the unused/dubious counters.

If there is anything else please let me know.

I still need to do v2 of the 'poll' patch set, I will do that next after
the snmp counters are done.

Thanks,
Florian

Re: [MPTCP] some squashing needed?

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]

Hi Florian,

On 27/09/2019 17:51, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>> Hi Paolo,
>>
>> On 26/09/2019 19:20, Paolo Abeni wrote:
>>> On Thu, 2019-09-26 at 17:05 +0200, Florian Westphal wrote:
>>
>> [...]
>>
>>>> from Paolo.
>>>>
>>>> But I figured it would make sense to share this now and discuss
>>>> how to proceed from here first.
>>>
>>> The above branch is based on mptcp_net-next export branch hash
>>> a45bdbe6701b
>>
>> Great, still the last one. So everything is in it.
>>
>>> There is a small diff vs such hash, see below. Mostily checkpack fixes,
>>> a trivial functional fix in mptcp_setsockopt(), and a bunch of likely
>>> not interesting debug messages dropped - side note: we may want remove
>>> more of them.
>>
>> I guess that we indeed need to remove most of these pr_debug() at some
>> points. Thank you for having done that!
>> Also great to have looked at checkpatch and making sure each commit can
>> compile :-)
>>
>> For the fix in mptcp_setsockopt(), was it due to a bad "squash"?
> 
> No, that is intentional -- I don't think returning 0 when nothing
> was done is good.
> 
> I can fold Peters mptcp prefix patch for crypto where needed and push a
> v2, let me know.

I just pushed a new tree on mptcp_net-next repo on Github based on your
branch :)

I didn't want to block anybody but I can re-create a new one again later
if needed!

Also I don't mind squashing this patch and also do the same for the
remaining functions (subflow_ ones I guess). I can do that on Monday if
it is OK for you.

Also, before recreating the new tree, I remove all (Git) refs to the
previous ones. If someone uses our repo with Topgit (we never know :) ),
please do the same on your side!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] some squashing needed?

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

Hi Paolo,

On 26/09/2019 19:20, Paolo Abeni wrote:
> On Thu, 2019-09-26 at 17:05 +0200, Florian Westphal wrote:

[...]

>> from Paolo.
>>
>> But I figured it would make sense to share this now and discuss
>> how to proceed from here first.
> 
> The above branch is based on mptcp_net-next export branch hash
> a45bdbe6701b

Great, still the last one. So everything is in it.

> There is a small diff vs such hash, see below. Mostily checkpack fixes,
> a trivial functional fix in mptcp_setsockopt(), and a bunch of likely
> not interesting debug messages dropped - side note: we may want remove
> more of them.

I guess that we indeed need to remove most of these pr_debug() at some
points. Thank you for having done that!
Also great to have looked at checkpatch and making sure each commit can
compile :-)

For the fix in mptcp_setsockopt(), was it due to a bad "squash"?

For the diff, it looks good to me.

*@Peter/@Mat*: is it also OK for you?

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] some squashing needed?

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 8935 bytes --]

On Thu, 2019-09-26 at 17:05 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > We (well, mainly poor Florian) are working to a rebase. Main reason for
> > rebase is that the squashing proved to be quite complex. Likely a pull
> > request could be shared very soon, but any export branch change from
> > now one will likely be omitted.
> 
> Paolo placed current iteration here:
> 
> https://github.com/pabeni/mptcp/commits/export_squash_09_more_nit
> 
> Following patches were folded (some were split and merged in
> several other patches).
> 
> Florian:
> mptcp: add and use mptcp_subflow_hold
> mptcp: selftests: switch to netns+veth based tests
> mptcp: accept: don't leak mptcp socket structure
> mptcp: switch sublist to mptcp socket lock protection
> mptcp: token: add mptcp prefixes to functions
> 
> Mat:
> mptcp: selftests: Add capture option
> 
> Peter:
> tcp: Make connection_list a real list of subflows
> 
> This results in 40 patches. The series is bisectable.
> 
> There are a few more candidates, e.g.
>   mptcp: harmonize locking on all socket operations
> 
> from Paolo.
> 
> But I figured it would make sense to share this now and discuss
> how to proceed from here first.

The above branch is based on mptcp_net-next export branch hash
a45bdbe6701b

There is a small diff vs such hash, see below. Mostily checkpack fixes,
a trivial functional fix in mptcp_setsockopt(), and a bunch of likely
not interesting debug messages dropped - side note: we may want remove
more of them.
---
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 439d6744c8e2..e0ae0a2542d1 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -4,6 +4,8 @@
  * Copyright (c) 2017 - 2019, Intel Corporation.
  */
 
+#define pr_fmt(fmt) "MPTCP: " fmt
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
@@ -291,8 +293,8 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	}
 
 	collapsed = skb == tcp_write_queue_tail(ssk);
-	BUG_ON(collapsed && !can_collapse);
 	if (collapsed) {
+		WARN_ON_ONCE(!can_collapse);
 		/* when collapsing mpext always exists */
 		mpext->data_len += ret;
 		goto out;
@@ -847,8 +849,6 @@ static int __mptcp_init_sock(struct sock *sk)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 
-	pr_debug("msk=%p", msk);
-
 	INIT_LIST_HEAD(&msk->conn_list);
 	INIT_LIST_HEAD(&msk->rtx_queue);
 
@@ -977,6 +977,7 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
 		msk->remote_key = subflow->remote_key;
 		msk->local_key = subflow->local_key;
 		msk->token = subflow->token;
+
 		mptcp_token_update_accept(new_sock->sk, new_mptcp_sock);
 		msk->subflow = NULL;
 
@@ -1036,7 +1037,7 @@ static int mptcp_setsockopt(struct sock *sk, int level, int optname,
 	/* @@ the meaning of setsockopt() when the socket is connected and
 	 * there are multiple subflows is not defined.
 	 */
-	return 0;
+	return -EOPNOTSUPP;
 }
 
 static int mptcp_getsockopt(struct sock *sk, int level, int optname,
@@ -1147,7 +1148,6 @@ void mptcp_finish_connect(struct sock *sk, int mp_capable)
 		subflow->map_seq = ack_seq;
 		subflow->map_subflow_seq = 1;
 		subflow->rel_write_seq = 1;
-
 		list_add(&subflow->node, &msk->conn_list);
 		msk->subflow = NULL;
 		bh_unlock_sock(sk);
@@ -1241,8 +1241,6 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	struct socket *ssock;
 	int err = -ENOTSUPP;
 
-	pr_debug("msk=%p", msk);
-
 	if (uaddr->sa_family != AF_INET) // @@ allow only IPv4 for now
 		return err;
 
@@ -1262,8 +1260,6 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	struct socket *ssock;
 	int err = -ENOTSUPP;
 
-	pr_debug("msk=%p", msk);
-
 	if (uaddr->sa_family != AF_INET) // @@ allow only IPv4 for now
 		return err;
 
@@ -1284,8 +1280,6 @@ static int mptcp_getname(struct socket *sock, struct sockaddr *uaddr,
 	struct sock *ssk;
 	int ret;
 
-	pr_debug("msk=%p", msk);
-
 	if (sock->sk->sk_prot == &tcp_prot) {
 		/* we are being invoked from __sys_accept4, after
 		 * mptcp_accept() has just accepted a non-mp-capable
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index d0fd4a9523db..f3f293c82058 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -8,8 +8,8 @@
 #define __MPTCP_PROTOCOL_H
 
 #include <linux/random.h>
-#include <linux/spinlock.h>
 #include <net/tcp.h>
+#include <net/inet_connection_sock.h>
 
 /* MPTCP option bits */
 #define OPTION_MPTCP_MPC_SYN	BIT(0)
@@ -199,10 +199,10 @@ struct subflow_context {
 	struct	list_head node;/* conn_list of subflows */
 	u64	local_key;
 	u64	remote_key;
+	u64	idsn;
+	u64	map_seq;
 	u32	token;
 	u32     rel_write_seq;
-	u64     idsn;
-	u64	map_seq;
 	u32	map_subflow_seq;
 	u32	ssn_offset;
 	u16	map_data_len;
@@ -284,6 +284,7 @@ static inline void crypto_key_gen_sha1(u64 *key, u32 *token, u64 *idsn)
 	get_random_bytes(key, sizeof(u64));
 	crypto_key_sha1(*key, token, idsn);
 }
+
 void crypto_hmac_sha1(u64 key1, u64 key2, u32 nonce1, u32 nonce2,
 		      u32 *hash_out);
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 465c1fcc59ac..80d297ce11d2 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -121,7 +121,6 @@ static void subflow_v4_init_req(struct request_sock *req,
 		    listener->request_cksum)
 			subflow_req->checksum = 1;
 		subflow_req->remote_key = rx_opt.mptcp.sndr_key;
-		pr_debug("syn seq=%u", TCP_SKB_CB(skb)->seq);
 		subflow_req->ssn_offset = TCP_SKB_CB(skb)->seq;
 	} else if (rx_opt.mptcp.mp_join && listener->request_mptcp) {
 		subflow_req->mp_join = 1;
@@ -249,13 +248,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 					  bool *own_req)
 {
 	struct subflow_context *listener = subflow_ctx(sk);
-	struct subflow_request_sock *subflow_req = subflow_rsk(req);
+	struct subflow_request_sock *subflow_req;
 	struct tcp_options_received opt_rx;
 	struct sock *child;
 
 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
 
 	/* if the sk is MP_CAPABLE, we already received the client key */
+	subflow_req = subflow_rsk(req);
 	if (!subflow_req->mp_capable && subflow_req->mp_join) {
 		opt_rx.mptcp.mp_join = 0;
 		mptcp_get_options(skb, &opt_rx);
@@ -458,14 +458,11 @@ static void subflow_ulp_clone(const struct request_sock *req,
 			      const gfp_t priority)
 {
 	struct subflow_request_sock *subflow_req = subflow_rsk(req);
-	struct subflow_context *old_ctx;
+	struct subflow_context *old_ctx = subflow_ctx(newsk);
 	struct subflow_context *new_ctx;
 
-	old_ctx = subflow_ctx(newsk);
-
 	/* newsk->sk_socket is NULL at this point */
 	new_ctx = subflow_create_ctx(newsk, NULL, priority);
-
 	if (!new_ctx)
 		return;
 
@@ -482,7 +479,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
 		new_ctx->token = subflow_req->token;
 		new_ctx->ssn_offset = subflow_req->ssn_offset;
 		new_ctx->idsn = subflow_req->idsn;
-		pr_debug("token=%u", new_ctx->token);
 	} else if (subflow_req->mp_join) {
 		new_ctx->mp_join = 1;
 		new_ctx->fourth_ack = 1;
@@ -490,7 +486,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
 		new_ctx->local_id = subflow_req->local_id;
 		new_ctx->token = subflow_req->token;
 		new_ctx->thmac = subflow_req->thmac;
-		pr_debug("token=%u", new_ctx->token);
 	}
 }
 
diff --git a/net/mptcp/token.c b/net/mptcp/token.c
index f493ebb4c2d8..27cd0acc6ea9 100644
--- a/net/mptcp/token.c
+++ b/net/mptcp/token.c
@@ -54,16 +54,15 @@ int mptcp_token_new_request(struct request_sock *req)
 	struct subflow_request_sock *subflow_req = subflow_rsk(req);
 	int err;
 
-	pr_debug("subflow_req=%p", req);
 	while (1) {
 		u32 token;
 
 		crypto_key_gen_sha1(&subflow_req->local_key,
 				    &subflow_req->token,
 				    &subflow_req->idsn);
-		pr_debug("local_key=%llu, token=%u, idsn=%llu\n",
-			 subflow_req->local_key,
-			 subflow_req->token, subflow_req->idsn);
+		pr_debug("req=%p local_key=%llu, token=%u, idsn=%llu\n",
+			 req, subflow_req->local_key, subflow_req->token,
+			 subflow_req->idsn);
 
 		token = subflow_req->token;
 		spin_lock_bh(&token_tree_lock);
@@ -101,17 +100,14 @@ int mptcp_token_new_connect(struct sock *sk)
 	struct sock *mptcp_sock = subflow->conn;
 	int err;
 
-	pr_debug("subflow=%p", sk);
-
 	while (1) {
 		u32 token;
 
 		crypto_key_gen_sha1(&subflow->local_key, &subflow->token,
 				    &subflow->idsn);
 
-		pr_debug("local_key=%llu, token=%u, idsn=%llu\n",
-			 subflow->local_key,
-			 subflow->token, subflow->idsn);
+		pr_debug("ssk=%p, local_key=%llu, token=%u, idsn=%llu\n",
+			 sk, subflow->local_key, subflow->token, subflow->idsn);
 
 		token = subflow->token;
 		spin_lock_bh(&token_tree_lock);

Re: [MPTCP] some squashing needed?

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

Paolo Abeni <pabeni(a)redhat.com> wrote:
> We (well, mainly poor Florian) are working to a rebase. Main reason for
> rebase is that the squashing proved to be quite complex. Likely a pull
> request could be shared very soon, but any export branch change from
> now one will likely be omitted.

Paolo placed current iteration here:

https://github.com/pabeni/mptcp/commits/export_squash_09_more_nit

Following patches were folded (some were split and merged in
several other patches).

Florian:
mptcp: add and use mptcp_subflow_hold
mptcp: selftests: switch to netns+veth based tests
mptcp: accept: don't leak mptcp socket structure
mptcp: switch sublist to mptcp socket lock protection
mptcp: token: add mptcp prefixes to functions

Mat:
mptcp: selftests: Add capture option

Peter:
tcp: Make connection_list a real list of subflows

This results in 40 patches. The series is bisectable.

There are a few more candidates, e.g.
  mptcp: harmonize locking on all socket operations

from Paolo.

But I figured it would make sense to share this now and discuss
how to proceed from here first.

Re: [MPTCP] some squashing needed?

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

Hi Paolo, Florian,

On 25/09/2019 18:45, Paolo Abeni wrote:
> Hi,
> 
> On Mon, 2019-09-23 at 14:03 +0200, Matthieu Baerts wrote:
>>> Resending the whole squashed series/specific patches for a
>>> partial/complete replace?
>>
>> I understand that "because of" Topgit, some people might not want to do
>> the rebase. But if someone prefers to do the rebase himself, it is
>> indeed still possible to do that directly with Git using the last
>> "export" branch.
> 
> We (well, mainly poor Florian) are working to a rebase. Main reason for
> rebase is that the squashing proved to be quite complex. Likely a pull
> request could be shared very soon, but any export branch change from
> now one will likely be omitted.

Thank you for this notification and for working on that!

Do you want me to temporary stop the CI job which recreates the "export"
branch each night?

> Can we please postpone a bit additional patches till the
> rebase/squashing is completed?
There were also some ideas requiring changes in many commits:
- prefix mptcp_ for all exposed functions: in fact, only the ones only
used in net/mptcp needs to be updated.
- Split protocol.h and have an header file for each .c file we have in
net/mptcp
- Move some code out of protocol.c in different files: e.g. input.c /
output.c
- IPv6 support? Can we also do that in commits at the end?

Do we still want to do that before the initial submission? Should we do
that now if we are starting a big work of rebase.

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] some squashing needed?

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

Hi,

On Mon, 2019-09-23 at 14:03 +0200, Matthieu Baerts wrote:
> > Resending the whole squashed series/specific patches for a
> > partial/complete replace?
> 
> I understand that "because of" Topgit, some people might not want to do
> the rebase. But if someone prefers to do the rebase himself, it is
> indeed still possible to do that directly with Git using the last
> "export" branch.

We (well, mainly poor Florian) are working to a rebase. Main reason for
rebase is that the squashing proved to be quite complex. Likely a pull
request could be shared very soon, but any export branch change from
now one will likely be omitted.

Can we please postpone a bit additional patches till the
rebase/squashing is completed?

Thanks!

Paolo

Re: [MPTCP] some squashing needed?

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

Hi Paolo,

Thank you for asking these questions!

On 23/09/2019 13:41, Paolo Abeni wrote:
> Hi all,
> 
> the current series is pretty big, and it will likely grow before it
> will be feature complete.
> 
> I think we should look after some squashing, but how do we do that? 
> Just asking good/poor Matt: "please squash chunk 1 of patch X into
> patch Y, <etc...>" ? (Yep Matt, you are authorized to hate me after
> that suggestion!)

That's fine, Belgium was not stopped at the last soccer World Cup
because of Italy, I will not hate you even after having proposed this
suggestion! :-D

I am really fine with this! After each rebase, I can send the diff
(which should be empty).

I can also split files, move and rename functions but maybe better if I
can do that directly in Git without having to first do some changes on
top of the series, drop them and do the job commit by commit. After
these changes directly in Git, I can send the diff for the review.

> Resending the whole squashed series/specific patches for a
> partial/complete replace?

I understand that "because of" Topgit, some people might not want to do
the rebase. But if someone prefers to do the rebase himself, it is
indeed still possible to do that directly with Git using the last
"export" branch.

We can even re-create a new TopGit tree after this rebase if we plan to
continue maintaining it on the side. The idea would be to still be able
to easily see the squash I do + continue to use the script to "rebase"
it on top of net-next.

> Is there any smarter options available? 

Could we maybe already send some patches to already have some stuff
upstream? :-)

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] some squashing needed?

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 486 bytes --]

Hi all,

the current series is pretty big, and it will likely grow before it
will be feature complete.

I think we should look after some squashing, but how do we do that? 
Just asking good/poor Matt: "please squash chunk 1 of patch X into
patch Y, <etc...>" ? (Yep Matt, you are authorized to hate me after
that suggestion!)
Resending the whole squashed series/specific patches for a
partial/complete replace?
Is there any smarter options available? 

Thanks,

Paolo