[MPTCP] Re: [PATCH] mptcp: propagate sendbuf setting to subflow on fallback

[MPTCP] Re: [PATCH] mptcp: propagate sendbuf setting to subflow on fallback

[Mat Martineau]

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


On Thu, 12 Mar 2020, Florian Westphal wrote:

> For normal mptcp subflows we do not need to propagate the sndbuf size
> because all skbs to be transmitted via mptcp get charged to the mptcp
> send buffer, i.e. mptcp sndbuf will limit the amount of data handed to
> the subflow sockets.
>
> However, when we enter fallback mode, the mptcp xmit function is cut
> short and no mptcp-level socket accouting takes place.
> Therefore for the sndbuf limit to have any effect we need to set it
> on the fallback socket.
>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
> I'll pass this to netdev@ unless there are objections.
>
> net/mptcp/protocol.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 8d6ab825bdab..ddfd02ab6e31 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -57,15 +57,27 @@ static bool __mptcp_needs_tcp_fallback(const struct mptcp_sock *msk)
> 	return msk->first && !sk_is_mptcp(msk->first);
> }
>
> +static void sync_sndbuf(const struct sock *msk, struct sock *ssk)
> +{
> +	if (unlikely((msk->sk_userlocks & SOCK_SNDBUF_LOCK) &&
> +	     ssk->sk_sndbuf != msk->sk_sndbuf)) {
> +		ssk->sk_userlocks |= SOCK_SNDBUF_LOCK;
> +		WRITE_ONCE(ssk->sk_sndbuf, msk->sk_sndbuf);
> +	}
> +}
> +
> static struct socket *__mptcp_tcp_fallback(struct mptcp_sock *msk)
> {
> -	sock_owned_by_me((const struct sock *)msk);
> +	struct sock *sk = (struct sock *)msk;
> +
> +	sock_owned_by_me(sk);
>
> 	if (likely(!__mptcp_needs_tcp_fallback(msk)))
> 		return NULL;
>
> 	if (msk->subflow) {
> -		release_sock((struct sock *)msk);
> +		sync_sndbuf(sk, msk->subflow->sk);
> +		release_sock(sk);

__mptcp_tcp_fallback() is used on several system calls to determine if a 
fallback subflow is in use, but the commit message implies the sync is to 
happen upon fallback.

This placement of sync_sndbuf() will sync the value in a few cases, 
including when mptcp_sendmsg() is called (which is a good time to sync) 
and mptcp_recvmsg().

It will also sync the value in setsockopt()/getsockopt() - but, 
counterintuitively, not with SOL_SOCKET options like SO_SNDBUF.

Is the intent to sync once at fallback time, or to resync on send/recv & 
sockopt changes? It looks like we set tcp_sk(sk)->is_mptcp = 0 in several 
places to implement fallback, so there isn't a dedicated function for 
things to do at fallback time.

When fallback has already happened, does it make sense to pass through 
SO_SNDBUF changes to a fallback subflow (which would unfortunately involve 
net core changes)?

--
Mat Martineau
Intel