[MPTCP] Re: [PATCH mptcp-next 3/7] mptcp: avoid blocking in tcp_sendpages due to skb alloc

[MPTCP] Re: [PATCH mptcp-next 3/7] mptcp: avoid blocking in tcp_sendpages due to skb alloc

[Paolo Abeni]

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

On Wed, 2020-05-06 at 15:02 +0200, Florian Westphal wrote:
> If memory is tight, tcp_sendpages may fail because it can't
> allocate the skb head.
> 
> Now that we pass DONTWAIT to tcp_sendpages it won't block anymore on the
> subflow socket when this happens.
> 
> We can avoid such allocation failure in tcp code by preallocating an skb
> skb when we pick the subflow to use.

The sk_tx_skb_cache usage caused some TCP regression upstream:

commit 0b7d7f6b22084a3156f267c85303908a8f4c9a08
Author: Eric Dumazet <edumazet(a)google.com>
Date:   Fri Jun 14 16:22:20 2019 -0700

    tcp: add tcp_tx_skb_cache sysctl

but that one was not very clear and I guess does not matter for MPTCP -
lacking an established baseline.

So all good ;)

> @@ -656,9 +677,13 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
>  static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>  {
>  	struct mptcp_subflow_context *subflow;
> +	struct sock *sk = (struct sock *)msk;
>  	struct sock *backup = NULL;
>  
> -	sock_owned_by_me((const struct sock *)msk);
> +	sock_owned_by_me(sk);
> +
> +	if (!mptcp_sendmsg_alloc_skb(sk))
> +		return NULL;
>  
>  	mptcp_for_each_subflow(msk, subflow) {
>  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);

Don't we need the same for mptcp_subflow_get_retrans() ?

Thanks!

/P

[MPTCP] Re: [PATCH mptcp-next 3/7] mptcp: avoid blocking in tcp_sendpages due to skb alloc

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Wed, 2020-05-06 at 15:02 +0200, Florian Westphal wrote:
> > If memory is tight, tcp_sendpages may fail because it can't
> > allocate the skb head.
> > 
> > Now that we pass DONTWAIT to tcp_sendpages it won't block anymore on the
> > subflow socket when this happens.
> > 
> > We can avoid such allocation failure in tcp code by preallocating an skb
> > skb when we pick the subflow to use.
> 
> The sk_tx_skb_cache usage caused some TCP regression upstream:
> 
> commit 0b7d7f6b22084a3156f267c85303908a8f4c9a08
> Author: Eric Dumazet <edumazet(a)google.com>
> Date:   Fri Jun 14 16:22:20 2019 -0700
> 
>     tcp: add tcp_tx_skb_cache sysctl
> 
> but that one was not very clear and I guess does not matter for MPTCP -
> lacking an established baseline.
> 
> So all good ;)
> 
> > @@ -656,9 +677,13 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
> >  static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> >  {
> >  	struct mptcp_subflow_context *subflow;
> > +	struct sock *sk = (struct sock *)msk;
> >  	struct sock *backup = NULL;
> >  
> > -	sock_owned_by_me((const struct sock *)msk);
> > +	sock_owned_by_me(sk);
> > +
> > +	if (!mptcp_sendmsg_alloc_skb(sk))
> > +		return NULL;
> >  
> >  	mptcp_for_each_subflow(msk, subflow) {
> >  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> 
> Don't we need the same for mptcp_subflow_get_retrans() ?

Its optional.  If allocation doesn't fail there is no difference.

If it fails then as-is, mptcp_sendmsg_frag will return -EAGAIN and
no packet is sent.

If I'd add mptcp_sendmsg_alloc_skb() call to the worker, then it
would bail our right before (and would not sleep either).

So the end result is the same: no retransmission takes place.

Let me know if you prefer explicit mptcp_sendmsg_alloc_skb()
for retransmit case too.

Thanks!

[MPTCP] Re: [PATCH mptcp-next 3/7] mptcp: avoid blocking in tcp_sendpages due to skb alloc

[Paolo Abeni]

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

On Thu, 2020-05-07 at 13:44 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Wed, 2020-05-06 at 15:02 +0200, Florian Westphal wrote:
> > > If memory is tight, tcp_sendpages may fail because it can't
> > > allocate the skb head.
> > > 
> > > Now that we pass DONTWAIT to tcp_sendpages it won't block anymore on the
> > > subflow socket when this happens.
> > > 
> > > We can avoid such allocation failure in tcp code by preallocating an skb
> > > skb when we pick the subflow to use.
> > 
> > The sk_tx_skb_cache usage caused some TCP regression upstream:
> > 
> > commit 0b7d7f6b22084a3156f267c85303908a8f4c9a08
> > Author: Eric Dumazet <edumazet(a)google.com>
> > Date:   Fri Jun 14 16:22:20 2019 -0700
> > 
> >     tcp: add tcp_tx_skb_cache sysctl
> > 
> > but that one was not very clear and I guess does not matter for MPTCP -
> > lacking an established baseline.
> > 
> > So all good ;)
> > 
> > > @@ -656,9 +677,13 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
> > >  static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> > >  {
> > >  	struct mptcp_subflow_context *subflow;
> > > +	struct sock *sk = (struct sock *)msk;
> > >  	struct sock *backup = NULL;
> > >  
> > > -	sock_owned_by_me((const struct sock *)msk);
> > > +	sock_owned_by_me(sk);
> > > +
> > > +	if (!mptcp_sendmsg_alloc_skb(sk))
> > > +		return NULL;
> > >  
> > >  	mptcp_for_each_subflow(msk, subflow) {
> > >  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > 
> > Don't we need the same for mptcp_subflow_get_retrans() ?
> 
> Its optional.  If allocation doesn't fail there is no difference.
> 
> If it fails then as-is, mptcp_sendmsg_frag will return -EAGAIN and
> no packet is sent.
> 
> If I'd add mptcp_sendmsg_alloc_skb() call to the worker, then it
> would bail our right before (and would not sleep either).
> 
> So the end result is the same: no retransmission takes place.
> 
> Let me know if you prefer explicit mptcp_sendmsg_alloc_skb()
> for retransmit case too.

Ok, I missed/forgot that the retransmission must be non blocking.

I now see that retransmissions are fine as is!

Still on this, don't we need to check mptcp_sendmsg_alloc_skb() inside
the sendmsg() loop, too? Elsewhere we could fail on large send if we
complete the first skb, keep looping in sendmsg() and the next
iteration would block. Am I missing something?

Thanks!

Paolo

[MPTCP] Re: [PATCH mptcp-next 3/7] mptcp: avoid blocking in tcp_sendpages due to skb alloc

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> Ok, I missed/forgot that the retransmission must be non blocking.
> 
> I now see that retransmissions are fine as is!
> 
> Still on this, don't we need to check mptcp_sendmsg_alloc_skb() inside
> the sendmsg() loop, too? Elsewhere we could fail on large send if we
> complete the first skb, keep looping in sendmsg() and the next
> iteration would block. Am I missing something?

You mean:

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 780d873b6c19..66a67b11d01b 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -814,6 +814,7 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
                if (!tx_ok)
                        break;
                if (!sk_stream_memory_free(ssk) ||
+                   !mptcp_sendmsg_alloc_skb(ssk) ||
                    !mptcp_page_frag_refill(ssk, pfrag) ||
                    !mptcp_ext_cache_refill(msk)) {
                        tcp_push(ssk, msg->msg_flags, mss_now,

?

Yes, that could be added.  Its not a huge deal since mptcp_sendmsg_frag() would
yield -EAGAIN if do_tcp_sendpages would block, and that will then re-start for
the 'blocking io' case (i.e., ssk is released and 'goto restart' fetches next
ssk with posssible wait_for_memory).

[MPTCP] Re: [PATCH mptcp-next 3/7] mptcp: avoid blocking in tcp_sendpages due to skb alloc

[Paolo Abeni]

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

On Thu, 2020-05-07 at 16:33 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > Ok, I missed/forgot that the retransmission must be non blocking.
> > 
> > I now see that retransmissions are fine as is!
> > 
> > Still on this, don't we need to check mptcp_sendmsg_alloc_skb() inside
> > the sendmsg() loop, too? Elsewhere we could fail on large send if we
> > complete the first skb, keep looping in sendmsg() and the next
> > iteration would block. Am I missing something?
> 
> You mean:
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 780d873b6c19..66a67b11d01b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -814,6 +814,7 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>                 if (!tx_ok)
>                         break;
>                 if (!sk_stream_memory_free(ssk) ||
> +                   !mptcp_sendmsg_alloc_skb(ssk) ||
>                     !mptcp_page_frag_refill(ssk, pfrag) ||
>                     !mptcp_ext_cache_refill(msk)) {
>                         tcp_push(ssk, msg->msg_flags, mss_now,
> 
> ?

Yep exactly!

> Yes, that could be added.  Its not a huge deal since mptcp_sendmsg_frag() would
> yield -EAGAIN if do_tcp_sendpages would block, and that will then re-start for
> the 'blocking io' case (i.e., ssk is released and 'goto restart' fetches next
> ssk with posssible wait_for_memory).

Again, I missed some pieces of the puzzle ;) Yep, we are good even
without that. The lack of simmetry WRT the other allocation is a bit
confusing to me, but it's really a subjective matter.

I'm good with or without the above chunk

Thanks for the clarifications!

Paolo

(ack to the whole series!)