On Sun, Feb 16, 2025 at 11:12:15PM +0100, Stefano Brivio wrote:
> In commit e5eefe77435a ("tcp: Refactor to use events instead of
> states, split out spliced implementation"), this:
> 
> 			if (!bitmap_isset(rcvlowat_set, conn - ts) &&
> 			    readlen > (long)c->tcp.pipe_size / 10) {
> 
> (note the !) became:
> 
> 			if (conn->flags & lowat_set_flag &&
> 			    readlen > (long)c->tcp.pipe_size / 10) {
> 
> in the new tcp_splice_sock_handler().
> 
> We want to check, there, if we should set SO_RCVLOWAT, only if we
> haven't set it already.
> 
> But, instead, we're checking if it's already set before we set it, so
> we'll never set it, of course.
> 
> Fix the check and re-enable the functionality, which should give us
> improved CPU utilisation in non-interactive cases where we are not
> transferring at full pipe capacity.
> 
> Fixes: e5eefe77435a ("tcp: Refactor to use events instead of states, split out spliced implementation")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Ouch.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

At least insofar as this clearly corrects towards the intended
behaviour.  Given that we inadvertently bee using RCVLOWAT for so
long, I am a bit worried that this might expose deadlocks or stalls.
But, I guess we debug that when we come to it.

> ---
>  tcp_splice.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 8a39a6f..5d845c9 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -556,7 +556,7 @@ eintr:
>  			if (readlen >= (long)c->tcp.pipe_size * 10 / 100)
>  				continue;
>  
> -			if (conn->flags & lowat_set_flag &&
> +			if (!(conn->flags & lowat_set_flag) &&
>  			    readlen > (long)c->tcp.pipe_size / 10) {
>  				int lowat = c->tcp.pipe_size / 4;
>  

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson