6:02 a.m.
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
11:54 a.m.
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
2:11 p.m.
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().
597
days inactive
597
days old
2 comments
2 participants
participants (2)
-
Florian Westphal -
Mat Martineau