[MPTCP] Re: [RFC 13/14] sendmsg: don't restart mptcp_sendmsg_frag

[MPTCP] Re: [RFC 13/14] sendmsg: don't restart mptcp_sendmsg_frag

[Paolo Abeni]

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

On Thu, 2019-11-14 at 18:32 +0100, Florian Westphal wrote:
> This function calls do_tcp_sendpages which already has such a loop.
> 
> When tcp sendbuffer runs out of space and non-blocking io is used,
> do_tcp_sendpages will return early because it can't sleep.
> No -EAGAIN is returned, as some data was sent.
> 
> When mptcp_sendmsg_frag is called again, next call will either return
> -EAGAIN immediately or it will only send a few more bytes.

If I understand correctly, the goal here is setting the appropriate
return value when overall sendmsg() spooled a few bytes and than would
block, right? currently we can return erroneously  -EAGAIN instead of
the number of written bytes on some scenarios.

I think that there is a side effect with this change: before a
blocking, successful, write(<large val>) would always return <large
val>,

After this patch it will return min(<large val>, <max skb size>), as
mptcp_sendmsg_frag() is limited to a single skb.

Do we want to preserve the old behavior?

Cheers,

Paolo

[MPTCP] Re: [RFC 13/14] sendmsg: don't restart mptcp_sendmsg_frag

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> I have no objections on this change.
> 
> Cheers,
> 
> Paolo
> 
> p.s. we will likely have to touch sendmsg_frag() again to fix the issue
> reported by Matt.

Ok, I will submit a v2 *without* this change.  We
can then probably better figure out how to handle this.

(We can also keep requirements for future .sendpage in mind).

[MPTCP] Re: [RFC 13/14] sendmsg: don't restart mptcp_sendmsg_frag

[Paolo Abeni]

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

On Mon, 2019-11-18 at 13:17 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Thu, 2019-11-14 at 18:32 +0100, Florian Westphal wrote:
> > > This function calls do_tcp_sendpages which already has such a loop.
> > > 
> > > When tcp sendbuffer runs out of space and non-blocking io is used,
> > > do_tcp_sendpages will return early because it can't sleep.
> > > No -EAGAIN is returned, as some data was sent.
> > > 
> > > When mptcp_sendmsg_frag is called again, next call will either return
> > > -EAGAIN immediately or it will only send a few more bytes.
> > 
> > If I understand correctly, the goal here is setting the appropriate
> > return value when overall sendmsg() spooled a few bytes and than would
> > block, right? currently we can return erroneously  -EAGAIN instead of
> > the number of written bytes on some scenarios.
> 
> No, the goal is to remove useless code.  The other solution would be
> to teach mptcp_send_frag to be able to tell when it did hit EGAIN to
> exit the loop earlier, for that it would need to be able to return
> both errno and the number of queued bytes.  I didn't like adding
> another int *err argument.

I see.

I have no objections on this change.

Cheers,

Paolo

p.s. we will likely have to touch sendmsg_frag() again to fix the issue
reported by Matt.

[MPTCP] Re: [RFC 13/14] sendmsg: don't restart mptcp_sendmsg_frag

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Thu, 2019-11-14 at 18:32 +0100, Florian Westphal wrote:
> > This function calls do_tcp_sendpages which already has such a loop.
> > 
> > When tcp sendbuffer runs out of space and non-blocking io is used,
> > do_tcp_sendpages will return early because it can't sleep.
> > No -EAGAIN is returned, as some data was sent.
> > 
> > When mptcp_sendmsg_frag is called again, next call will either return
> > -EAGAIN immediately or it will only send a few more bytes.
> 
> If I understand correctly, the goal here is setting the appropriate
> return value when overall sendmsg() spooled a few bytes and than would
> block, right? currently we can return erroneously  -EAGAIN instead of
> the number of written bytes on some scenarios.

No, the goal is to remove useless code.  The other solution would be
to teach mptcp_send_frag to be able to tell when it did hit EGAIN to
exit the loop earlier, for that it would need to be able to return
both errno and the number of queued bytes.  I didn't like adding
another int *err argument.

> I think that there is a side effect with this change: before a
> blocking, successful, write(<large val>) would always return <large
> val>,

Yes.

> After this patch it will return min(<large val>, <max skb size>), as
> mptcp_sendmsg_frag() is limited to a single skb.
> 
> Do we want to preserve the old behavior?

TCP doesn't guarantee that it consumes entire buffer either, why would
mptcp try to?