[PATCH net-next 0/2] TCP splice improvements

[PATCH net-next 0/2] TCP splice improvements

[Pavel Begunkov]

The main part is in Patch 1, which optimises locking for successful
blocking TCP splice read, following with a clean up in Patch 2.

Pavel Begunkov (2):
  net/tcp: optimise locking for blocking splice
  net/tcp: optimise non error splice paths

 net/ipv4/tcp.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
2.40.0

[PATCH net-next 1/2] net/tcp: optimise locking for blocking splice

[Pavel Begunkov]

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;
 
-		if (!timeo)
+		if (!tss.len || !timeo)
 			break;
 		release_sock(sk);
 		lock_sock(sk);
-- 
2.40.0

Re: [PATCH net-next 1/2] net/tcp: optimise locking for blocking splice

[Paolo Abeni]

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

Re: [PATCH net-next 1/2] net/tcp: optimise locking for blocking splice

[Pavel Begunkov]

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.

-- 
Pavel Begunkov

Re: [PATCH net-next 1/2] net/tcp: optimise locking for blocking splice

[Pavel Begunkov]

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.

-- 
Pavel Begunkov

Re: [PATCH net-next 1/2] net/tcp: optimise locking for blocking splice

[Eric Dumazet]

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.

Re: [PATCH net-next 1/2] net/tcp: optimise locking for blocking splice

[Pavel Begunkov]

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.

-- 
Pavel Begunkov

[PATCH net-next 2/2] net/tcp: optimise non error splice paths

[Pavel Begunkov]

Move post __tcp_splice_read() error checking inside the "nothing to
read" block. That removes an extra if from the path where it has
successfully spliced some bytes.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 net/ipv4/tcp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index bf7627f37e69..0139b2c70ed4 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -799,9 +799,9 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
 	timeo = sock_rcvtimeo(sk, sock->file->f_flags & O_NONBLOCK);
 	while (true) {
 		ret = __tcp_splice_read(sk, &tss);
-		if (ret < 0)
-			break;
-		else if (!ret) {
+		if (ret <= 0) {
+			if (unlikely(ret < 0))
+				break;
 			if (spliced)
 				break;
 			if (sock_flag(sk, SOCK_DONE))
-- 
2.40.0