[MPTCP] Re: [RFC PATCH 3/4] Squash-to: "mptcp: Implement MPTCP receive path"

[MPTCP] Re: [RFC PATCH 3/4] Squash-to: "mptcp: Implement MPTCP receive path"

[Mat Martineau]

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

On Fri, 20 Dec 2019, Paolo Abeni wrote:

> deal with fallback: we must reset sk callback to the TCP one.
> Use the cached ptr in the old ctx.

Hi Paolo -

Is this required if subflow_data_ready(), subflow_state_change(), and 
subflow_write_space() only call through to the normal TCP functions when 
ctx->conn is NULL? It looks like that is what happens in the current code, 
seems like a tradeoff between adding extra storage (on every subflow) and 
the extra "if (subflow->conn)" cycles for these callbacks.

>
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/protocol.h | 13 ++++++++++++-
> net/mptcp/subflow.c  | 11 ++++++++---
> 2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index e4fce9f0ad65..7f3cc7952227 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -121,7 +121,10 @@ struct mptcp_subflow_context {
> 	struct	sock *tcp_sock;	    /* tcp sk backpointer */
> 	struct	sock *conn;	    /* parent mptcp_sock */
> 	const	struct inet_connection_sock_af_ops *icsk_af_ops;
> -	void	(*tcp_sk_data_ready)(struct sock *sk);
> +	void	(*tcp_data_ready)(struct sock *sk);
> +	void	(*tcp_state_change)(struct sock *sk);

If we add these pointers (see question above), should we also use them in 
subflow_data_ready() and subflow_state_change() when subflow->conn is 
NULL? Or do we then expect that subflow->conn is never NULL?

Mat


> +	void	(*tcp_write_space)(struct sock *sk);
> +
> 	struct	rcu_head rcu;
> };
>
> @@ -159,6 +162,14 @@ bool mptcp_subflow_data_available(struct sock *sk);
> void mptcp_subflow_init(void);
> int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock);
>
> +static inline void mptcp_subflow_tcp_fallback(struct sock *sk,
> +					      struct mptcp_subflow_context *ctx)
> +{
> +	sk->sk_data_ready = ctx->tcp_data_ready;
> +	sk->sk_state_change = ctx->tcp_state_change;
> +	sk->sk_write_space = ctx->tcp_write_space;
> +}
> +
> extern const struct inet_connection_sock_af_ops ipv4_specific;
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> extern const struct inet_connection_sock_af_ops ipv6_specific;
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 48bf0b1d8d28..3ac1764f276b 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -504,7 +504,7 @@ static void subflow_data_ready(struct sock *sk)
> 	struct sock *parent = subflow->conn;
>
> 	if (!parent || !subflow->mp_capable) {
> -		subflow->tcp_sk_data_ready(sk);
> +		subflow->tcp_data_ready(sk);
>
> 		if (parent)
> 			parent->sk_data_ready(parent);
> @@ -652,7 +652,9 @@ static int subflow_ulp_init(struct sock *sk)
> 	if (sk->sk_family == AF_INET6)
> 		icsk->icsk_af_ops = &subflow_v6_specific;
> #endif
> -	ctx->tcp_sk_data_ready = sk->sk_data_ready;
> +	ctx->tcp_data_ready = sk->sk_data_ready;
> +	ctx->tcp_state_change = sk->sk_state_change;
> +	ctx->tcp_write_space = sk->sk_write_space;
> 	sk->sk_data_ready = subflow_data_ready;
> 	sk->sk_write_space = subflow_write_space;
> 	sk->sk_state_change = subflow_state_change;
> @@ -683,6 +685,7 @@ static void subflow_ulp_clone(const struct request_sock *req,
>
> 	if (!subflow_req->mp_capable ||
> 	    (new_ctx = subflow_create_ctx(newsk, priority)) == NULL) {
> +		mptcp_subflow_tcp_fallback(newsk, old_ctx);
> 		tcp_sk(newsk)->is_mptcp = 0;
> 		return;
> 	}
> @@ -690,7 +693,9 @@ static void subflow_ulp_clone(const struct request_sock *req,
> 	new_ctx->conn = NULL;
> 	new_ctx->conn_finished = 1;
> 	new_ctx->icsk_af_ops = old_ctx->icsk_af_ops;
> -	new_ctx->tcp_sk_data_ready = old_ctx->tcp_sk_data_ready;
> +	new_ctx->tcp_data_ready = old_ctx->tcp_data_ready;
> +	new_ctx->tcp_state_change = old_ctx->tcp_state_change;
> +	new_ctx->tcp_write_space = old_ctx->tcp_write_space;
> 	new_ctx->mp_capable = 1;
> 	new_ctx->fourth_ack = 1;
> 	new_ctx->remote_key = subflow_req->remote_key;
> -- 
> 2.21.0
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org
>

--
Mat Martineau
Intel

[MPTCP] Re: [RFC PATCH 3/4] Squash-to: "mptcp: Implement MPTCP receive path"

[Paolo Abeni]

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

On Fri, 2019-12-20 at 14:06 -0800, Mat Martineau wrote:
> On Fri, 20 Dec 2019, Paolo Abeni wrote:
> 
> > deal with fallback: we must reset sk callback to the TCP one.
> > Use the cached ptr in the old ctx.
> 
> Hi Paolo -
> 
> Is this required if subflow_data_ready(), subflow_state_change(), and 
> subflow_write_space() only call through to the normal TCP functions when 
> ctx->conn is NULL? 

Uhm... almost. The actuall condition is somewhat more complex, but we
can indeed replace the indirect call with condition + direct call,
except that 'sock_def_wakeup()' - tcp -> sk_state_change - is declared
as static in net/core/sock.c. 

We can't direct-call that function and the current mptcp code duplicate
it - which is not so nice BTW.

Quick greeping inside the '/net' subtree shows that the preferred
solution when overriding sock callback is storing the old one and use
indirect calls.

I guess that is due to pre-spectre implementations and to avoid
inferring internals of the overridden sock status.

Likely MPTCP already assumes TCP specific semantic for the underlying
sock in several places, so we could possibly avoid the indirect calls,
but we would need to make visible 'sock_def_wakeup()'. Do we need an
additional pre-req patch for that?

In this RFC I vilely and intentionally opted for the quickest/dumbest
solution to avoid the above tricky questions ;)
 
Perhpas we should really opt for the direct-call solution - exporting
sock_def_wakeup() in "mptcp: Implement MPTCP receive path"???

Any suggestions more than welcome!

Cheers,

Paolo