[MPTCP] Questions/remarks wrt. mptcp layered prototype

[MPTCP] Questions/remarks wrt. mptcp layered prototype

[Florian Westphal]

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

Hi.

I had a look at the layered prototype, here are some questions/remarks.
1) mptcp_close():

                hlist_del_rcu(node);
                spin_unlock_bh(&msk->conn_list_lock);
                synchronize_rcu();
                pr_debug("conn_list->subflow=%p", subflow);
                sock_release(sock_sk(subflow)->sk_socket);
        } while (1);

What is this synchronize_rcu() supposed to protect/do?
AFAICS it can be removed?

2) struct tcp_options_received should really have IS_ENABLED(CONFIG_MPTCP)
 wrap for the mptcp struct; unlike smc it increases struct size.
So better appease those that have CONFIG_MPTCP=n.

3) running scripts/checkpatch.pl spews out a gazillion of warnings,
   I would suggest fix those up rather sooner than later.
   Its annoting task for sure, but someone has to do it.

4) the networking maintainer enforces reverse-xmas tree for variable
   declarations, so 

 	if (tcp_rsk(req)->is_mptcp) {
		u64 local_key;
 		u64 remote_key;

   must become
 	if (tcp_rsk(req)->is_mptcp) {
 		u64 remote_key;
		u64 local_key;

   This might seems silly, but thats how things are.

5) The line in skbuff.c adding the mptcp extension should look like this
    [SKB_EXT_MPTCP] = SKB_EXT_CHUNKSIZEOF(struct mptcp_skb_cb),
    Also, mptcp_skb_cb still contains obsolete reference count.
    (while at it, probably better to rename struct, mptcp_skb_cb makes
    me think its stored in skb->cb[]).

6) I think it would help to add an mptcp kselftest; I can look at this next.

Thanks,
Florian

Re: [MPTCP] Questions/remarks wrt. mptcp layered prototype

[Florian Westphal]

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

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> > I had a look at the layered prototype, here are some questions/remarks.
> > 1) mptcp_close():
> > 
> >                hlist_del_rcu(node);
> >                spin_unlock_bh(&msk->conn_list_lock);
> >                synchronize_rcu();
> >                pr_debug("conn_list->subflow=%p", subflow);
> >                sock_release(sock_sk(subflow)->sk_socket);
> >        } while (1);
> > 
> > What is this synchronize_rcu() supposed to protect/do?
> > AFAICS it can be removed?
> 
> Readers of conn_list call sock_hold(subflow) while the rcu read lock is
> held, and can do so without any socket locks held. We want to make sure they
> have adjusted the subflow socket reference count before calling
> sock_release() here, which can free the subflow socket.

I see, thanks for explaining.  This will need a different solution,
 synchronize_rcu() can cause delays in oder of several hundreds of milliseconds.

I'll first work on selftest, then look it this one.
It might be as simple as move sock_hold() users to refcount_inc_not_zero().

Re: [MPTCP] Questions/remarks wrt. mptcp layered prototype

[Mat Martineau]

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


Hi Florian -

On Thu, 31 Jan 2019, Florian Westphal wrote:

> Hi.
>
> I had a look at the layered prototype, here are some questions/remarks.
> 1) mptcp_close():
>
>                hlist_del_rcu(node);
>                spin_unlock_bh(&msk->conn_list_lock);
>                synchronize_rcu();
>                pr_debug("conn_list->subflow=%p", subflow);
>                sock_release(sock_sk(subflow)->sk_socket);
>        } while (1);
>
> What is this synchronize_rcu() supposed to protect/do?
> AFAICS it can be removed?

Readers of conn_list call sock_hold(subflow) while the rcu read lock is 
held, and can do so without any socket locks held. We want to make sure 
they have adjusted the subflow socket reference count before calling 
sock_release() here, which can free the subflow socket.

>
> 2) struct tcp_options_received should really have IS_ENABLED(CONFIG_MPTCP)
> wrap for the mptcp struct; unlike smc it increases struct size.
> So better appease those that have CONFIG_MPTCP=n.
>
> 3) running scripts/checkpatch.pl spews out a gazillion of warnings,
>   I would suggest fix those up rather sooner than later.
>   Its annoting task for sure, but someone has to do it.

Matthieu also noted these in his gerrit review. I've fixed up the 
checkpatch issues this week but haven't pushed those changes yet.

>
> 4) the networking maintainer enforces reverse-xmas tree for variable
>   declarations, so
>
> 	if (tcp_rsk(req)->is_mptcp) {
> 		u64 local_key;
> 		u64 remote_key;
>
>   must become
> 	if (tcp_rsk(req)->is_mptcp) {
> 		u64 remote_key;
> 		u64 local_key;
>
>   This might seems silly, but thats how things are.

Thanks for the reminder, I'm aware of the reverse xmas tree convention but 
we were not rigorous about enforcing it.

>
> 5) The line in skbuff.c adding the mptcp extension should look like this
>    [SKB_EXT_MPTCP] = SKB_EXT_CHUNKSIZEOF(struct mptcp_skb_cb),
>    Also, mptcp_skb_cb still contains obsolete reference count.
>    (while at it, probably better to rename struct, mptcp_skb_cb makes
>    me think its stored in skb->cb[]).

Ok, will fix those issues.

>
> 6) I think it would help to add an mptcp kselftest; I can look at this next.
>

That will be very helpful, thank you!

--
Mat Martineau
Intel