[MPTCP] Re: [RFC 06/14] sendmsg: block until mptcp sk is writeable

[MPTCP] Re: [RFC 06/14] sendmsg: block until mptcp sk is writeable

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Thu, 2019-11-14 at 18:32 +0100, Florian Westphal wrote:
> > This disables transmit of new data until the peer has acked
> > enough mptcp data to get below the wspace write threshold (more than
> > half of wspace upperlimit is available again).
> > 
> > Also have poll not report EPOLLOUT in this case, its not relevant if a
> > subflow is writeable.
> > 
> > The latter is a temporary workaround that is needed because mptcp_poll
> > walks the subflows and calls __tcp_poll on each of them.
> > Because subflow ssk is usually writable, we will have to undo-that
> > if the mptcp sndbuf is exhausted.  This won't be needed anymore once
> > __tcp_poll is removed, I am working on this.
> > 
> > Signed-off-by: Florian Westphal <fw(a)strlen.de>
> > ---
> >  net/mptcp/protocol.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 2144e80b8704..83be407e1dd6 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -406,6 +406,18 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >  		return ret;
> >  	}
> >  
> > +	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
> > +
> > +	mptcp_clean_una(sk);
> > +
> > +	while (!sk_stream_memory_free(sk)) {
> > +		ret = sk_stream_wait_memory(sk, &timeo);
> > +		if (ret)
> > +			goto out;
> > +
> > +		mptcp_clean_una(sk);
> > +	}
> > +
> 
> Can we move the above loop to the non fallback case only ? e.g. after
> the below !mptcp_subflow_get(msk) checks?
> 
> If so, we could have a single loop checking for:
> 
> !sk_stream_memory_free(sk) || !mptcp_subflow_get_send()
> 
> (together with the next patch)

It would be easy to do if I remove

       if (!msg_data_left(msg)) {
	       pr_debug("empty send");
	       ret = sock_sendmsg(ssk->sk_socket, msg);

any idea why this is there in the first place?

[MPTCP] Re: [RFC 06/14] sendmsg: block until mptcp sk is writeable

[Mat Martineau]

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

On Mon, 18 Nov 2019, Paolo Abeni wrote:

> On Mon, 2019-11-18 at 13:11 +0100, Florian Westphal wrote:
>> Paolo Abeni <pabeni(a)redhat.com> wrote:
>>> On Thu, 2019-11-14 at 18:32 +0100, Florian Westphal wrote:
>>>> This disables transmit of new data until the peer has acked
>>>> enough mptcp data to get below the wspace write threshold (more than
>>>> half of wspace upperlimit is available again).
>>>>
>>>> Also have poll not report EPOLLOUT in this case, its not relevant if a
>>>> subflow is writeable.
>>>>
>>>> The latter is a temporary workaround that is needed because mptcp_poll
>>>> walks the subflows and calls __tcp_poll on each of them.
>>>> Because subflow ssk is usually writable, we will have to undo-that
>>>> if the mptcp sndbuf is exhausted.  This won't be needed anymore once
>>>> __tcp_poll is removed, I am working on this.
>>>>
>>>> Signed-off-by: Florian Westphal <fw(a)strlen.de>
>>>> ---
>>>>  net/mptcp/protocol.c | 18 ++++++++++++++++--
>>>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>>> index 2144e80b8704..83be407e1dd6 100644
>>>> --- a/net/mptcp/protocol.c
>>>> +++ b/net/mptcp/protocol.c
>>>> @@ -406,6 +406,18 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>>>>  		return ret;
>>>>  	}
>>>>
>>>> +	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
>>>> +
>>>> +	mptcp_clean_una(sk);
>>>> +
>>>> +	while (!sk_stream_memory_free(sk)) {
>>>> +		ret = sk_stream_wait_memory(sk, &timeo);
>>>> +		if (ret)
>>>> +			goto out;
>>>> +
>>>> +		mptcp_clean_una(sk);
>>>> +	}
>>>> +
>>>
>>> Can we move the above loop to the non fallback case only ? e.g. after
>>> the below !mptcp_subflow_get(msk) checks?
>>>
>>> If so, we could have a single loop checking for:
>>>
>>> !sk_stream_memory_free(sk) || !mptcp_subflow_get_send()
>>>
>>> (together with the next patch)
>>
>> It would be easy to do if I remove
>>
>>        if (!msg_data_left(msg)) {
>> 	       pr_debug("empty send");
>> 	       ret = sock_sendmsg(ssk->sk_socket, msg);
>>
>> any idea why this is there in the first place?
>
> Uhmm... that looks like a left-over from the initial implementation ?!?
> possibly trying to deal with fastopen?!?
>
> I think it can be dropped. @Peter / @Mat do you know better?
>

Paolo's right, that's a leftover. You're welcome to drop it.

--
Mat Martineau
Intel

[MPTCP] Re: [RFC 06/14] sendmsg: block until mptcp sk is writeable

[Paolo Abeni]

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

On Mon, 2019-11-18 at 13:11 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Thu, 2019-11-14 at 18:32 +0100, Florian Westphal wrote:
> > > This disables transmit of new data until the peer has acked
> > > enough mptcp data to get below the wspace write threshold (more than
> > > half of wspace upperlimit is available again).
> > > 
> > > Also have poll not report EPOLLOUT in this case, its not relevant if a
> > > subflow is writeable.
> > > 
> > > The latter is a temporary workaround that is needed because mptcp_poll
> > > walks the subflows and calls __tcp_poll on each of them.
> > > Because subflow ssk is usually writable, we will have to undo-that
> > > if the mptcp sndbuf is exhausted.  This won't be needed anymore once
> > > __tcp_poll is removed, I am working on this.
> > > 
> > > Signed-off-by: Florian Westphal <fw(a)strlen.de>
> > > ---
> > >  net/mptcp/protocol.c | 18 ++++++++++++++++--
> > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index 2144e80b8704..83be407e1dd6 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -406,6 +406,18 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> > >  		return ret;
> > >  	}
> > >  
> > > +	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
> > > +
> > > +	mptcp_clean_una(sk);
> > > +
> > > +	while (!sk_stream_memory_free(sk)) {
> > > +		ret = sk_stream_wait_memory(sk, &timeo);
> > > +		if (ret)
> > > +			goto out;
> > > +
> > > +		mptcp_clean_una(sk);
> > > +	}
> > > +
> > 
> > Can we move the above loop to the non fallback case only ? e.g. after
> > the below !mptcp_subflow_get(msk) checks?
> > 
> > If so, we could have a single loop checking for:
> > 
> > !sk_stream_memory_free(sk) || !mptcp_subflow_get_send()
> > 
> > (together with the next patch)
> 
> It would be easy to do if I remove
> 
>        if (!msg_data_left(msg)) {
> 	       pr_debug("empty send");
> 	       ret = sock_sendmsg(ssk->sk_socket, msg);
> 
> any idea why this is there in the first place?

Uhmm... that looks like a left-over from the initial implementation ?!?
possibly trying to deal with fastopen?!? 

I think it can be dropped. @Peter / @Mat do you know better?

/P

[MPTCP] Re: [RFC 06/14] sendmsg: block until mptcp sk is writeable

[Paolo Abeni]

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

On Mon, 2019-11-18 at 12:29 +0100, Paolo Abeni wrote:
> Can we move the above loop to the non fallback case only ? e.g. after
> the below !mptcp_subflow_get(msk) checks?
> 
> If so, we could have a single loop checking for:
> 
> !sk_stream_memory_free(sk) || !mptcp_subflow_get_send()
> 
> (together with the next patch)

Dumb me! I meant "with patch 9/14" - where a similar loop is added.

/P

[MPTCP] Re: [RFC 06/14] sendmsg: block until mptcp sk is writeable

[Paolo Abeni]

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

On Thu, 2019-11-14 at 18:32 +0100, Florian Westphal wrote:
> This disables transmit of new data until the peer has acked
> enough mptcp data to get below the wspace write threshold (more than
> half of wspace upperlimit is available again).
> 
> Also have poll not report EPOLLOUT in this case, its not relevant if a
> subflow is writeable.
> 
> The latter is a temporary workaround that is needed because mptcp_poll
> walks the subflows and calls __tcp_poll on each of them.
> Because subflow ssk is usually writable, we will have to undo-that
> if the mptcp sndbuf is exhausted.  This won't be needed anymore once
> __tcp_poll is removed, I am working on this.
> 
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/protocol.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 2144e80b8704..83be407e1dd6 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -406,6 +406,18 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  		return ret;
>  	}
>  
> +	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
> +
> +	mptcp_clean_una(sk);
> +
> +	while (!sk_stream_memory_free(sk)) {
> +		ret = sk_stream_wait_memory(sk, &timeo);
> +		if (ret)
> +			goto out;
> +
> +		mptcp_clean_una(sk);
> +	}
> +

Can we move the above loop to the non fallback case only ? e.g. after
the below !mptcp_subflow_get(msk) checks?

If so, we could have a single loop checking for:

!sk_stream_memory_free(sk) || !mptcp_subflow_get_send()

(together with the next patch)

Cheers,

Paolo