net: Don't save mid batch datagram processing error for next recvmmsg call
Subsystems (3)
Hi,
I think this addresses the problems reported by David Laight and
others, where errors saved on a per socket area could be delivered to a different thread, so I just followed David Laight's suggestion and stopped saving it, we'll return it only if it happens for the first datagram, else we return less entries than asked for.
Steven, IIRC you was the one that suggested using this
mechanism, no? Do you have anything against this move?
- Arnaldo
Quoted 1 line
From
5cf1a5c682c8bb0add9adfb5167a63f269136523Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <[email protected]>
Date: Mon, 21 Jul 2014 12:39:56 -0300
Subject: [PATCH 2/2] net: Don't save mid batch datagram processing error for
next recvmmsg call
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When the call to the underlying per protocol recvmsg fails for some reason and we already have successfully processed at least one datagram in the batch, we were saving the error for the next recvmmsg (or recvmsg) call, so the user would eventually get notified of what happened.
This was found to be fraught with problems, as, for instance, errors could be notified for a different thread than a faulty one providing invalid buffers.
So just return the number of datagrams successfully received.
--- net/socket.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)
net/socket.c +7 -22
@@ -2436,28 +2436,13 @@ out_put: timeout->tv_nsec = (timeout_hz %HZ ) * (NSEC_PER_SEC /HZ ); } -if (err ==0 ) -return datagrams; - -if (datagrams !=0 ) { -/* -* We may return less entries than requested (vlen) if the -* sock is non block and there aren't enough datagrams... -*/ -if (err != -EAGAIN ) { -/* -* ... or if recvmsg returns an error after we -* received some datagrams, where we record the -* error to return on the next call or if the -* app asks about it using getsockopt(SO_ERROR). -*/ - sock->sk->sk_err = -err; - } - -return datagrams; - } - -return err; +/* +* We may return less entries than requested (vlen) if the +* sock is non block and there aren't enough datagrams or +* if some problem happened after successfully receiving one +* datagram. +*/ +return datagrams ?: err; } SYSCALL_DEFINE5(recvmmsg, int, fd, struct mmsghdr __user *, mmsg,
Signature · 1 line
1.9.3
David Miller
maintainerFrom: Arnaldo Carvalho de Melo <[email protected]>
Date: Mon, 21 Jul 2014 16:30:42 -0300
Quoted 22 lines
From
5cf1a5c682c8bb0add9adfb5167a63f269136523Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <[email protected]>
Date: Mon, 21 Jul 2014 12:39:56 -0300
Subject: [PATCH 2/2] net: Don't save mid batch datagram processing error for
next recvmmsg call
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bitWhen the call to the underlying per protocol recvmsg fails for some reason and we already have successfully processed at least one datagram in the batch, we were saving the error for the next recvmmsg (or recvmsg) call, so the user would eventually get notified of what happened.
This was found to be fraught with problems, as, for instance, errors could be notified for a different thread than a faulty one providing invalid buffers.
So just return the number of datagrams successfully received.
Reported-by David Laight <[email protected]>
...
Quoted 1 line
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
This is absolutely correct, any time we have a partial success we must indicate success and the amount that was properly transferred, leaving the error signalling for a subsequent retry (if any).
From: Arnaldo Carvalho de Melo
Quoted 8 lines
I think this addresses the problems reported by David Laight and
others, where errors saved on a per socket area could be delivered to a different thread, so I just followed David Laight's suggestion and stopped saving it, we'll return it only if it happens for the first datagram, else we return less entries than asked for.Steven, IIRC you was the one that suggested using this
mechanism, no? Do you have anything against this move?
+ return datagrams ?: err;
Inline patches, don't attach them.
Don't use non-C constructs.
David
Em Tue, Jul 22, 2014 at 08:33:27AM +0000, David Laight escreveu:
Quoted 12 lines
From: Arnaldo Carvalho de Melo
Quoted 8 lines
I think this addresses the problems reported by David Laight and
others, where errors saved on a per socket area could be delivered to a different thread, so I just followed David Laight's suggestion and stopped saving it, we'll return it only if it happens for the first datagram, else we return less entries than asked for.Steven, IIRC you was the one that suggested using this
mechanism, no? Do you have anything against this move?net/socket.c +1
-0+return datagrams ?: err;
Quoted 1 line
Inline patches, don't attach them.
Oh well, ok, will do next time.
Quoted 1 line
Don't use non-C constructs.
We use it in many places, one could say the kernel isn't written in C anyway:
[acme@zoo linux]$ find . -name "*.[ch]" | xargs grep '?:' | cut -d'/' -f -2 | sort | uniq -c | sort -nr
189 ./drivers
45 ./arch
33 ./crypto
30 ./fs
25 ./net
23 ./kernel
22 ./tools
9 ./sound
8 ./include
5 ./security
5 ./mm
3 ./block
2 ./lib
1 ./scripts
1 ./ipc[acme@zoo linux]$
Other than that, are you ok with the change?
- Arnaldo