| Message ID | a6838ca891ccff2c2407d9232ccd2a46fa3f8989.1684501922.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | TCP splice improvements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 10 this patch: 10 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 10 this patch: 10 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 28 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, 2023-05-19 at 14:33 +0100, Pavel Begunkov wrote: > Even when tcp_splice_read() reads all it was asked for, for blocking > sockets it'll release and immediately regrab the socket lock, loop > around and break on the while check. > > Check tss.len right after we adjust it, and return if we're done. > That saves us one release_sock(); lock_sock(); pair per successful > blocking splice read. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > net/ipv4/tcp.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 4d6392c16b7a..bf7627f37e69 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -789,13 +789,15 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, > */ > if (unlikely(*ppos)) > return -ESPIPE; > + if (unlikely(!tss.len)) > + return 0; > > ret = spliced = 0; > > lock_sock(sk); > > timeo = sock_rcvtimeo(sk, sock->file->f_flags & O_NONBLOCK); > - while (tss.len) { > + while (true) { > ret = __tcp_splice_read(sk, &tss); > if (ret < 0) > break; > @@ -835,10 +837,10 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, > } > continue; > } > - tss.len -= ret; > spliced += ret; > + tss.len -= ret; The patch LGTM. The only minor thing that I note is that the above chunk is not needed. Perhaps avoiding unneeded delta could be worthy. Cheers, Paolo
On 5/23/23 14:52, Paolo Abeni wrote: > On Fri, 2023-05-19 at 14:33 +0100, Pavel Begunkov wrote: >> Even when tcp_splice_read() reads all it was asked for, for blocking >> sockets it'll release and immediately regrab the socket lock, loop >> around and break on the while check. >> >> Check tss.len right after we adjust it, and return if we're done. >> That saves us one release_sock(); lock_sock(); pair per successful >> blocking splice read. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> net/ipv4/tcp.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >> index 4d6392c16b7a..bf7627f37e69 100644 >> --- a/net/ipv4/tcp.c >> +++ b/net/ipv4/tcp.c >> @@ -789,13 +789,15 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, >> */ >> if (unlikely(*ppos)) >> return -ESPIPE; >> + if (unlikely(!tss.len)) >> + return 0; >> >> ret = spliced = 0; >> >> lock_sock(sk); >> >> timeo = sock_rcvtimeo(sk, sock->file->f_flags & O_NONBLOCK); >> - while (tss.len) { >> + while (true) { >> ret = __tcp_splice_read(sk, &tss); >> if (ret < 0) >> break; >> @@ -835,10 +837,10 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, >> } >> continue; >> } >> - tss.len -= ret; >> spliced += ret; >> + tss.len -= ret; > > The patch LGTM. The only minor thing that I note is that the above > chunk is not needed. Perhaps avoiding unneeded delta could be worthy. It keeps it closer to the tss.len test, so I'd leave it for that reason, but on the other hand the compiler should be perfectly able to optimise it regardless (i.e. sub;cmp;jcc; vs sub;jcc;). I don't have a hard feeling on that, can change if you want.
On 5/24/23 13:51, Pavel Begunkov wrote: > On 5/23/23 14:52, Paolo Abeni wrote: >> On Fri, 2023-05-19 at 14:33 +0100, Pavel Begunkov wrote: >>> Even when tcp_splice_read() reads all it was asked for, for blocking >>> sockets it'll release and immediately regrab the socket lock, loop >>> around and break on the while check. >>> >>> Check tss.len right after we adjust it, and return if we're done. >>> That saves us one release_sock(); lock_sock(); pair per successful >>> blocking splice read. >>> >>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>> --- >>> net/ipv4/tcp.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >>> index 4d6392c16b7a..bf7627f37e69 100644 >>> --- a/net/ipv4/tcp.c >>> +++ b/net/ipv4/tcp.c >>> @@ -789,13 +789,15 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, >>> */ >>> if (unlikely(*ppos)) >>> return -ESPIPE; >>> + if (unlikely(!tss.len)) >>> + return 0; >>> ret = spliced = 0; >>> lock_sock(sk); >>> timeo = sock_rcvtimeo(sk, sock->file->f_flags & O_NONBLOCK); >>> - while (tss.len) { >>> + while (true) { >>> ret = __tcp_splice_read(sk, &tss); >>> if (ret < 0) >>> break; >>> @@ -835,10 +837,10 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, >>> } >>> continue; >>> } >>> - tss.len -= ret; >>> spliced += ret; >>> + tss.len -= ret; >> >> The patch LGTM. The only minor thing that I note is that the above >> chunk is not needed. Perhaps avoiding unneeded delta could be worthy. > > It keeps it closer to the tss.len test, so I'd leave it for that reason, > but on the other hand the compiler should be perfectly able to optimise it > regardless (i.e. sub;cmp;jcc; vs sub;jcc;). I don't have a hard feeling > on that, can change if you want. Is there anything I can do to help here? I think the patch is fine, but can amend the change per Paolo's suggestion if required.
On Mon, Jun 19, 2023 at 11:27 AM Pavel Begunkov <asml.silence@gmail.com> wrote: > > On 5/24/23 13:51, Pavel Begunkov wrote: > > On 5/23/23 14:52, Paolo Abeni wrote: > >> On Fri, 2023-05-19 at 14:33 +0100, Pavel Begunkov wrote: > >>> Even when tcp_splice_read() reads all it was asked for, for blocking > >>> sockets it'll release and immediately regrab the socket lock, loop > >>> around and break on the while check. > >>> > >>> Check tss.len right after we adjust it, and return if we're done. > >>> That saves us one release_sock(); lock_sock(); pair per successful > >>> blocking splice read. > >>> > >>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > >>> --- > >>> net/ipv4/tcp.c | 8 +++++--- > >>> 1 file changed, 5 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > >>> index 4d6392c16b7a..bf7627f37e69 100644 > >>> --- a/net/ipv4/tcp.c > >>> +++ b/net/ipv4/tcp.c > >>> @@ -789,13 +789,15 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, > >>> */ > >>> if (unlikely(*ppos)) > >>> return -ESPIPE; > >>> + if (unlikely(!tss.len)) > >>> + return 0; > >>> ret = spliced = 0; > >>> lock_sock(sk); > >>> timeo = sock_rcvtimeo(sk, sock->file->f_flags & O_NONBLOCK); > >>> - while (tss.len) { > >>> + while (true) { > >>> ret = __tcp_splice_read(sk, &tss); > >>> if (ret < 0) > >>> break; > >>> @@ -835,10 +837,10 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, > >>> } > >>> continue; > >>> } > >>> - tss.len -= ret; > >>> spliced += ret; > >>> + tss.len -= ret; > >> > >> The patch LGTM. The only minor thing that I note is that the above > >> chunk is not needed. Perhaps avoiding unneeded delta could be worthy. > > > > It keeps it closer to the tss.len test, so I'd leave it for that reason, > > but on the other hand the compiler should be perfectly able to optimise it > > regardless (i.e. sub;cmp;jcc; vs sub;jcc;). I don't have a hard feeling > > on that, can change if you want. > > Is there anything I can do to help here? I think the patch is > fine, but can amend the change per Paolo's suggestion if required. > We prefer seeing patches focusing on the change, instead of also doing arbitrary changes making future backports more likely to conflict. Thanks.
On 6/19/23 11:59, Eric Dumazet wrote: > On Mon, Jun 19, 2023 at 11:27 AM Pavel Begunkov <asml.silence@gmail.com> wrote: >> >> On 5/24/23 13:51, Pavel Begunkov wrote: >>> On 5/23/23 14:52, Paolo Abeni wrote: >>>> On Fri, 2023-05-19 at 14:33 +0100, Pavel Begunkov wrote: >>>>> Even when tcp_splice_read() reads all it was asked for, for blocking >>>>> sockets it'll release and immediately regrab the socket lock, loop >>>>> around and break on the while check. >>>>> >>>>> Check tss.len right after we adjust it, and return if we're done. >>>>> That saves us one release_sock(); lock_sock(); pair per successful >>>>> blocking splice read. >>>>> >>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>>> --- >>>>> net/ipv4/tcp.c | 8 +++++--- >>>>> 1 file changed, 5 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >>>>> index 4d6392c16b7a..bf7627f37e69 100644 >>>>> --- a/net/ipv4/tcp.c >>>>> +++ b/net/ipv4/tcp.c >>>>> @@ -789,13 +789,15 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, >>>>> */ >>>>> if (unlikely(*ppos)) >>>>> return -ESPIPE; >>>>> + if (unlikely(!tss.len)) >>>>> + return 0; >>>>> ret = spliced = 0; >>>>> lock_sock(sk); >>>>> timeo = sock_rcvtimeo(sk, sock->file->f_flags & O_NONBLOCK); >>>>> - while (tss.len) { >>>>> + while (true) { >>>>> ret = __tcp_splice_read(sk, &tss); >>>>> if (ret < 0) >>>>> break; >>>>> @@ -835,10 +837,10 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, >>>>> } >>>>> continue; >>>>> } >>>>> - tss.len -= ret; >>>>> spliced += ret; >>>>> + tss.len -= ret; >>>> >>>> The patch LGTM. The only minor thing that I note is that the above >>>> chunk is not needed. Perhaps avoiding unneeded delta could be worthy. >>> >>> It keeps it closer to the tss.len test, so I'd leave it for that reason, >>> but on the other hand the compiler should be perfectly able to optimise it >>> regardless (i.e. sub;cmp;jcc; vs sub;jcc;). I don't have a hard feeling >>> on that, can change if you want. >> >> Is there anything I can do to help here? I think the patch is >> fine, but can amend the change per Paolo's suggestion if required. >> > > We prefer seeing patches focusing on the change, instead of also doing > arbitrary changes > making future backports more likely to conflict. Thank you for taking a look! I cut it down and resent. I don't agree it's arbitrary, it's a clean up related to the change. I'm just trying to not make the death by a thousand cuts problem worse for networking, but I guess I'm worried for nothing.
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 4d6392c16b7a..bf7627f37e69 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -789,13 +789,15 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, */ if (unlikely(*ppos)) return -ESPIPE; + if (unlikely(!tss.len)) + return 0; ret = spliced = 0; lock_sock(sk); timeo = sock_rcvtimeo(sk, sock->file->f_flags & O_NONBLOCK); - while (tss.len) { + while (true) { ret = __tcp_splice_read(sk, &tss); if (ret < 0) break; @@ -835,10 +837,10 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos, } continue; } - tss.len -= ret; spliced += ret; + tss.len -= ret; - if (!timeo) + if (!tss.len || !timeo) break; release_sock(sk); lock_sock(sk);
Even when tcp_splice_read() reads all it was asked for, for blocking sockets it'll release and immediately regrab the socket lock, loop around and break on the while check. Check tss.len right after we adjust it, and return if we're done. That saves us one release_sock(); lock_sock(); pair per successful blocking splice read. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- net/ipv4/tcp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)