diff mbox series

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

Message ID 20200507143348.GO32392@breakpoint.cc
State Superseded, archived
Delegated to: Paolo Abeni
Headers show
Series Re: [PATCH mptcp-next 3/7] mptcp: avoid blocking in tcp_sendpages due to skb alloc | expand

Commit Message

Florian Westphal May 7, 2020, 2:33 p.m. UTC 
Paolo Abeni <pabeni@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:


?

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).

Comments

Paolo Abeni May 7, 2020, 2:38 p.m. UTC | #1 
On Thu, 2020-05-07 at 16:33 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni@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!)
diff mbox series

Patch

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,