[MPTCP] Re: [PATCH v2 mptcp-next 2/4] mptcp: re-enable sndbuf autotune

[MPTCP] Re: [PATCH v2 mptcp-next 2/4] mptcp: re-enable sndbuf autotune

[Paolo Abeni]

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

On Mon, 2021-01-11 at 17:01 -0800, Mat Martineau wrote:
> On Mon, 11 Jan 2021, Paolo Abeni wrote:
> 
> > After commit 6e628cd3a8f7 ("mptcp: use mptcp release_cb for
> > delayed tasks"), MPTCP never sets the flag bit SOCK_NOSPACE
> > on its subflow. As a side effect, autotune never takes place,
> > as it happens inside tcp_new_space(), which in turn is called
> > only when the mentioned bit is set.
> > 
> > Let's sendmsg() set the subflows NOSPACE bit when looking for
> > more memory. Additionally, cleanup the sndbuf propagation from
> > subflow into the msk, leveraging the subflow write space callback
> > and dropping a bunch of duplicate code.
> > 
> > This also makes the SNDBUF_LIMITED chrono relevant again
> > for MPTCP subflows.
> > 
> > Fixes: 6e628cd3a8f7 ("mptcp: use mptcp release_cb for delayed tasks")
> > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > ---
> > v1 -> v2:
> > - leverage the previous commit, so we can always simply set the nospace
> >   bit on the msk/parent socket
> > ---
> > net/mptcp/protocol.c | 57 +++++++++++++++++++-------------------------
> > net/mptcp/protocol.h | 20 ++++++++++++++++
> > net/mptcp/subflow.c  | 10 +++++++-
> > 3 files changed, 53 insertions(+), 34 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index ada69495a51b..a64b2f6fb17b 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1421,7 +1410,8 @@ static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk,
> > 		 send_info[1].ssk, send_info[1].ratio);
> > 
> > 	/* pick the best backup if no other subflow is active */
> > -	if (!nr_active)
> > +	msk->use_backup = !nr_active;
> > +	if (msk->use_backup)
> 
> This is the only place msk->use_backup is read. Is this a leftover from 
> v1?

Indeed! thanks for noticing! I will drop that in the next iteration.

Paolo

[MPTCP] Re: [PATCH v2 mptcp-next 2/4] mptcp: re-enable sndbuf autotune

[Mat Martineau]

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

On Mon, 11 Jan 2021, Paolo Abeni wrote:

> After commit 6e628cd3a8f7 ("mptcp: use mptcp release_cb for
> delayed tasks"), MPTCP never sets the flag bit SOCK_NOSPACE
> on its subflow. As a side effect, autotune never takes place,
> as it happens inside tcp_new_space(), which in turn is called
> only when the mentioned bit is set.
>
> Let's sendmsg() set the subflows NOSPACE bit when looking for
> more memory. Additionally, cleanup the sndbuf propagation from
> subflow into the msk, leveraging the subflow write space callback
> and dropping a bunch of duplicate code.
>
> This also makes the SNDBUF_LIMITED chrono relevant again
> for MPTCP subflows.
>
> Fixes: 6e628cd3a8f7 ("mptcp: use mptcp release_cb for delayed tasks")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> v1 -> v2:
> - leverage the previous commit, so we can always simply set the nospace
>   bit on the msk/parent socket
> ---
> net/mptcp/protocol.c | 57 +++++++++++++++++++-------------------------
> net/mptcp/protocol.h | 20 ++++++++++++++++
> net/mptcp/subflow.c  | 10 +++++++-
> 3 files changed, 53 insertions(+), 34 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index ada69495a51b..a64b2f6fb17b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

> @@ -1421,7 +1410,8 @@ static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk,
> 		 send_info[1].ssk, send_info[1].ratio);
>
> 	/* pick the best backup if no other subflow is active */
> -	if (!nr_active)
> +	msk->use_backup = !nr_active;
> +	if (msk->use_backup)

This is the only place msk->use_backup is read. Is this a leftover from 
v1?


--
Mat Martineau
Intel