[PATCH] [doc] msg_zerocopy.rst: clarify the TCP shutdown scenario

[PATCH] [doc] msg_zerocopy.rst: clarify the TCP shutdown scenario

[Frederik Deweerdt]

Hi folks,

Based on my understanding, retransmissions of zero copied buffers can
happen after `close(2)`, the patch below amends the docs to suggest how
notifications should be handled in that case.

Explicitly mention that applications shouldn't be calling `close(2)` on
a TCP socket without draining the error queue.
---
 Documentation/networking/msg_zerocopy.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/networking/msg_zerocopy.rst b/Documentation/networking/msg_zerocopy.rst
index 15920db8d35d..cb44fc1f3e3e 100644
--- a/Documentation/networking/msg_zerocopy.rst
+++ b/Documentation/networking/msg_zerocopy.rst
@@ -144,6 +144,10 @@ the socket. A socket that has an error queued would normally block
 other operations until the error is read. Zerocopy notifications have
 a zero error code, however, to not block send and recv calls.
 
+For protocols like TCP, where retransmissions can occur after the
+application is done with a given connection, applications should signal
+the close to the peer via shutdown(2), and keep polling the error queue
+until all transmissions have completed.
 
 Notification Batching
 ~~~~~~~~~~~~~~~~~~~~~
-- 
2.36.1

Re: [PATCH] [doc] msg_zerocopy.rst: clarify the TCP shutdown scenario

[Willem de Bruijn]

On Tue, May 31, 2022 at 10:48 PM Frederik Deweerdt
<frederik.deweerdt@gmail.com> wrote:
>
> Hi folks,
>
> Based on my understanding, retransmissions of zero copied buffers can
> happen after `close(2)`, the patch below amends the docs to suggest how
> notifications should be handled in that case.

Not just retransmissions. The first transmission similarly may be queued.

>
> Explicitly mention that applications shouldn't be calling `close(2)` on
> a TCP socket without draining the error queue.
> ---
>  Documentation/networking/msg_zerocopy.rst | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/networking/msg_zerocopy.rst b/Documentation/networking/msg_zerocopy.rst
> index 15920db8d35d..cb44fc1f3e3e 100644
> --- a/Documentation/networking/msg_zerocopy.rst
> +++ b/Documentation/networking/msg_zerocopy.rst
> @@ -144,6 +144,10 @@ the socket. A socket that has an error queued would normally block
>  other operations until the error is read. Zerocopy notifications have
>  a zero error code, however, to not block send and recv calls.
>
> +For protocols like TCP, where retransmissions can occur after the
> +application is done with a given connection, applications should signal
> +the close to the peer via shutdown(2), and keep polling the error queue
> +until all transmissions have completed.

A socket must not be closed until all completion notifications have
been received.

Calling shutdown is an optional step. It may be sufficient to simply
delay close.

Re: [PATCH] [doc] msg_zerocopy.rst: clarify the TCP shutdown scenario

[Frederik Deweerdt]

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

Hi Willem,

On Wed, Jun 01, 2022 at 09:24:32AM -0400, Willem de Bruijn wrote:
> On Tue, May 31, 2022 at 10:48 PM Frederik Deweerdt
> <frederik.deweerdt@gmail.com> wrote:
> >
> > Hi folks,
> >
> > Based on my understanding, retransmissions of zero copied buffers can
> > happen after `close(2)`, the patch below amends the docs to suggest how
> > notifications should be handled in that case.
> 
> Not just retransmissions. The first transmission similarly may be queued.
> 
> >
[...]
> > @@ -144,6 +144,10 @@ the socket. A socket that has an error queued would normally block
> >  other operations until the error is read. Zerocopy notifications have
> >  a zero error code, however, to not block send and recv calls.
> >
> > +For protocols like TCP, where retransmissions can occur after the
> > +application is done with a given connection, applications should signal
> > +the close to the peer via shutdown(2), and keep polling the error queue
> > +until all transmissions have completed.
> 
> A socket must not be closed until all completion notifications have
> been received.
> 
> Calling shutdown is an optional step. It may be sufficient to simply
> delay close.

Thank you for the feedback, that helps!

What do you think of the attached patch?

Frederik

Signed-off-by: Frederik Deweerdt <frederik.deweerdt@gmail.com>

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1155 bytes --]

commit 3218d973b68bc6d9f88d9e2374f3ada3df5ee7ff
Author: Frederik Deweerdt <frederik.deweerdt@gmail.com>
Date:   Tue May 31 18:23:54 2022 -0700

    [doc] msg_zerocopy.rst: clarify the TCP shutdown scenario
    
    Explicitly mention that applications shouldn't be calling `close(2)` on
    a TCP socket without draining the error queue.

diff --git a/Documentation/networking/msg_zerocopy.rst b/Documentation/networking/msg_zerocopy.rst
index 15920db8d35d..9373631d0a82 100644
--- a/Documentation/networking/msg_zerocopy.rst
+++ b/Documentation/networking/msg_zerocopy.rst
@@ -144,6 +144,11 @@ the socket. A socket that has an error queued would normally block
 other operations until the error is read. Zerocopy notifications have
 a zero error code, however, to not block send and recv calls.
 
+For protocols like TCP, transmissions can occur after the application
+has called close(2). In cases where it's undesirable to delay calling
+close(2) until all notifications have been processed, the application
+can use shutdown(2), and keep polling the error queue until all
+transmissions have completed.
 
 Notification Batching
 ~~~~~~~~~~~~~~~~~~~~~

Re: [PATCH] [doc] msg_zerocopy.rst: clarify the TCP shutdown scenario

[Willem de Bruijn]

On Wed, Jun 1, 2022 at 7:03 PM Frederik Deweerdt
<frederik.deweerdt@gmail.com> wrote:
>
> Hi Willem,
>
> On Wed, Jun 01, 2022 at 09:24:32AM -0400, Willem de Bruijn wrote:
> > On Tue, May 31, 2022 at 10:48 PM Frederik Deweerdt
> > <frederik.deweerdt@gmail.com> wrote:
> > >
> > > Hi folks,
> > >
> > > Based on my understanding, retransmissions of zero copied buffers can
> > > happen after `close(2)`, the patch below amends the docs to suggest how
> > > notifications should be handled in that case.
> >
> > Not just retransmissions. The first transmission similarly may be queued.
> >
> > >
> [...]
> > > @@ -144,6 +144,10 @@ the socket. A socket that has an error queued would normally block
> > >  other operations until the error is read. Zerocopy notifications have
> > >  a zero error code, however, to not block send and recv calls.
> > >
> > > +For protocols like TCP, where retransmissions can occur after the
> > > +application is done with a given connection, applications should signal
> > > +the close to the peer via shutdown(2), and keep polling the error queue
> > > +until all transmissions have completed.
> >
> > A socket must not be closed until all completion notifications have
> > been received.
> >
> > Calling shutdown is an optional step. It may be sufficient to simply
> > delay close.
>
> Thank you for the feedback, that helps!
>
> What do you think of the attached patch?

Please always share patches inline.

> +++ b/Documentation/networking/msg_zerocopy.rst
> @@ -144,6 +144,11 @@ the socket. A socket that has an error queued would normally block
> other operations until the error is read. Zerocopy notifications have
> a zero error code, however, to not block send and recv calls.
>
> +For protocols like TCP, transmissions can occur after the application
> +has called close(2). In cases where it's undesirable to delay calling
> +close(2) until all notifications have been processed, the application
> +can use shutdown(2), and keep polling the error queue until all
> +transmissions have completed.

Sorry to nitpick, but calling close(2) is still delayed. You meant where it's
undesirable to delay closing the TCP connection?

Shutdown can be used then. But that is not unique to msg_zerocopy?
I don't feel strongly either way. Perhaps it would be helpful to describe
what makes it is undesirable to wait with calling close.

Btw, packets leaving the host after a socket is closed is not unique to
the TCP protocol, but an effect of queuing, such as in the qdisc layer.

RE: [PATCH] [doc] msg_zerocopy.rst: clarify the TCP shutdown scenario

[David Laight]

From: Frederik Deweerdt <frederik.deweerdt@gmail.com>
> Sent: 02 June 2022 00:01
> >
> > A socket must not be closed until all completion notifications have
> > been received.
> >
> > Calling shutdown is an optional step. It may be sufficient to simply
> > delay close.

What happens if the process gets killed - eg by SIGSEGV?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] [doc] msg_zerocopy.rst: clarify the TCP shutdown scenario

[Willem de Bruijn]

On Fri, Jun 3, 2022 at 9:18 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Frederik Deweerdt <frederik.deweerdt@gmail.com>
> > Sent: 02 June 2022 00:01
> > >
> > > A socket must not be closed until all completion notifications have
> > > been received.
> > >
> > > Calling shutdown is an optional step. It may be sufficient to simply
> > > delay close.
>
> What happens if the process gets killed - eg by SIGSEGV?

The skb frags hold independent references on the pages. Once skbs are
freed on transmit completion or socket purge the pages are released if
there are no other page references.

Otherwise there is nothing zerocopy specific about closing TCP
connections when a process crashes.

RE: [PATCH] [doc] msg_zerocopy.rst: clarify the TCP shutdown scenario

[David Laight]

From: Willem de Bruijn
> Sent: 03 June 2022 14:30
> 
> On Fri, Jun 3, 2022 at 9:18 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Frederik Deweerdt <frederik.deweerdt@gmail.com>
> > > Sent: 02 June 2022 00:01
> > > >
> > > > A socket must not be closed until all completion notifications have
> > > > been received.
> > > >
> > > > Calling shutdown is an optional step. It may be sufficient to simply
> > > > delay close.
> >
> > What happens if the process gets killed - eg by SIGSEGV?
> 
> The skb frags hold independent references on the pages. Once skbs are
> freed on transmit completion or socket purge the pages are released if
> there are no other page references.
> 
> Otherwise there is nothing zerocopy specific about closing TCP
> connections when a process crashes.

So the proposed text for the documentation is just wrong.

Something that says that retransmissions can re-read the
pages after the socket is closed would be more relevant.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)