[MPTCP] Re: [RFC PATCH 01/12] mptcp: msk is writable according to msk write space

[MPTCP] Re: [RFC PATCH 01/12] mptcp: msk is writable according to msk write space

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> Currently, when checking for the 'msk is writable' condition,
> we look at the individual subflows write space. That works
> well while we sends data via a single subflow, but will not
> as soon as we will enable concurrent xmit on multiple subflows.
> 
> Let's use the msk write space instead.

Why do we need MPTCP_SEND_SPACE bit after this change?

> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
>  net/mptcp/protocol.c | 42 ++++++++++++++++++++++++------------------
>  net/mptcp/subflow.c  |  3 +--
>  2 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3b9ae98c67bb..d51b1d95dfb8 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -612,8 +612,15 @@ static void mptcp_clean_una(struct sock *sk)
>  		sk_mem_reclaim_partial(sk);
>  
>  		/* Only wake up writers if a subflow is ready */
> -		if (test_bit(MPTCP_SEND_SPACE, &msk->flags))
> +		if (sk_stream_is_writeable(sk)) {
> +			set_bit(MPTCP_SEND_SPACE, &mptcp_sk(sk)->flags);

mptcp sk has enough wmem, so MPTCP_SEND_SPACE is set.

> @@ -799,21 +806,27 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
>  	return ret;
>  }
>  
> -static void mptcp_nospace(struct mptcp_sock *msk, struct socket *sock)
> +static void mptcp_nospace(struct sock *sk, struct sock *ssk)
>  {
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct socket *sock;
> +
>  	clear_bit(MPTCP_SEND_SPACE, &msk->flags);

This clears it, helper is called when msk wmem occupancy is too large.

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 7e838bc6e364..bd2201d62ae6 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -926,8 +926,7 @@ static void subflow_write_space(struct sock *sk)
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>  	struct sock *parent = subflow->conn;
>  
> -	sk_stream_write_space(sk);
> -	if (sk_stream_is_writeable(sk)) {
> +	if (sk_stream_is_writeable(parent)) {
>  		set_bit(MPTCP_SEND_SPACE, &mptcp_sk(parent)->flags);

here, its set again when wmem occupancy of msk is low enough again.

So, AFAICS we can just remove it completely.

If not, perhaps add a comment as to what its signalling vs. 'msk is
writeable/not writeable'.

Otherwise looks like a good cleanup to me.

[MPTCP] Re: [RFC PATCH 01/12] mptcp: msk is writable according to msk write space

[Paolo Abeni]

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

On Sun, 2020-08-02 at 00:23 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > Currently, when checking for the 'msk is writable' condition,
> > we look at the individual subflows write space. That works
> > well while we sends data via a single subflow, but will not
> > as soon as we will enable concurrent xmit on multiple subflows.
> > 
> > Let's use the msk write space instead.
> 
> Why do we need MPTCP_SEND_SPACE bit after this change?

I'm testing without it, I see the following:

the msk is writable, but all the subflows have sk_stream_wspace() < 0.

The above can happen because we account the full truesize to ssk, while
only the page frag size to the msk.

Note that checking:

	sk_stream_wspace(sk) > 0 && sk_stream_is_writeable(parent)

in subflow_write_space() is not enough, we also need mptcp_poll()
returning a mask with EPOLLOUT cleared when all subflows have no write
space.

Overall I think that keeping the flag around is the simplest option.

Cheers,

Paolo