Skip to content

selftest/bpf: make test_tc_tunnel and test_tc_edt more robust to CI load - #13294

Closed
kernel-patches-daemon-bpf[bot] wants to merge 2 commits into
bpf_basefrom
series/1145882=>bpf
Closed

selftest/bpf: make test_tc_tunnel and test_tc_edt more robust to CI load#13294
kernel-patches-daemon-bpf[bot] wants to merge 2 commits into
bpf_basefrom
series/1145882=>bpf

Conversation

@kernel-patches-daemon-bpf

Copy link
Copy Markdown

Pull request for series with
subject: selftest/bpf: make test_tc_tunnel and test_tc_edt more robust to CI load
version: 5
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1145882

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: a13307e
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1145882
version: 5

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 4114dc4c134fae09172bde960e21e7e52fb7c72f
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

> bpf/selftests: fix comment style in network_helpers.c
>
> BPF subsystem requires multi-line comments to have the opening /* start
> on its own line.
>
> Update multi-line comments in network_helpers.c to follow this
> requirement.
>
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

This isn't a bug, but would 'selftests/bpf:' be the expected subject
prefix here?  The rest of the directory uses that convention (351
occurrences of 'selftests/bpf:' versus 0 of 'bpf/selftests:' in the
recent history), and the following patch in this series uses
'selftests/bpf:'.



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf/selftests: fix comment style in network_helpers.c
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31779510003

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 3cabab8543d3aca3281496bdf0c99c438bb2ce49
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

selftests/bpf: allocate a larger timeout for connection

This patch addresses sporadic test failures in CI by setting a larger
timeout specifically for the connect() step, then restoring the original
test-configured timeout for data I/O operations.

> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index d311db786880..ed4aeee1555d 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c

[ ... ]

> @@ -291,6 +293,12 @@ int client_socket(int family, int type,
>  	return -1;
>  }
>
> +static int connect_timeout_ms(const struct network_helper_opts *opts)
> +{
> +	/* Enforce a minimum connect timeout value */
> +	return MAX(opts->timeout_ms, CONNECT_MIN_TIMEOUT_MS);
> +}
> +
>  int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t addrlen,
>  		    const struct network_helper_opts *opts)
>  {
> @@ -305,13 +313,35 @@ int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t add
>  		return -1;
>  	}
>
> +	/*
> +	 * Override timeout configuration with a larger value for the
> +	 * connection
> +	 */
> +	if (settimeo(fd, connect_timeout_ms(opts))) {
> +		log_err("Failed to set connect timeout");
> +		goto close;
> +	}
> +
>  	if (connect(fd, (const struct sockaddr *)addr, addrlen)) {
> -		log_err("Failed to connect to server");
> -		save_errno_close(fd);
> -		return -1;
> +		log_err("Failed to connect");
> +		goto close;
> +	}
> +
> +	/*
> +	 * If the timeout configured by the test is different from the
> +	 * connect timeout, restore it
> +	 */
> +	if (opts->timeout_ms != CONNECT_MIN_TIMEOUT_MS &&
> +	    settimeo(fd, opts->timeout_ms)) {
> +		log_err("Failed to set timeout for connected socket");
> +		goto close;
                     ^^^^

Does this condition match what the comment describes? The value applied
to the socket before connect() is connect_timeout_ms(opts), which returns
MAX(opts->timeout_ms, CONNECT_MIN_TIMEOUT_MS). The guard tests
opts->timeout_ms != CONNECT_MIN_TIMEOUT_MS, so the two expressions only
match when opts->timeout_ms <= CONNECT_MIN_TIMEOUT_MS.

When a test passes a timeout larger than the minimum, the guard fires even
though the socket already has the right value. For example,
prog_tests/xdp_synproxy.c calls connect_to_fd(server_fd, 10000).
connect_timeout_ms() returns 10000, settimeo() applies 10000, then the guard
(10000 != 5000) is true and settimeo() re-applies the same 10000 value the
socket already has.

Would opts->timeout_ms != connect_timeout_ms(opts) express the stated
intent? That guard would be false when opts->timeout_ms == 10000 (nothing
to restore), and true when opts->timeout_ms < CONNECT_MIN_TIMEOUT_MS
(restore the smaller value the test asked for).

> +	}
> +
>  	return fd;
> +
> +close:
> +	save_errno_close(fd);
> +	return -1;
>  }



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: selftests/bpf: allocate a larger timeout for connection
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31779510003

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: a13307e
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1145882
version: 5

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: a13307e
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1145882
version: 5

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: a13307e
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1145882
version: 5

BPF subsystem requires multi-line comments to have the opening /* start
on its own line.

Update multi-line comments in network_helpers.c to follow this
requirement.

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: a13307e
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1145882
version: 5

Some tests, like tc_tunnel or tc_edt, sporadically fail in CI with the
following logs:

  (network_helpers.c:309: errno: Operation now in progress) \
    Failed to connect to server
  send_and_test_data:FAIL:connect to server unexpected error: -115

This is due to SO_RCVTIMEO and SO_SNDTIMEO being set on the client
socket (see settimeo() in client_socket()), allowing connect() to return
an error and to set errno to EINPROGRESS instead of ETIMEDOUT.
Increasing the timeout value for those tests is likely not a good
solution (and it has already been done by commit 2790db2
("selftests/bpf: Improve tc_tunnel test reliability")): some tests
expect some data transfer to fail, and so the timeout value would
increase overall test execution duration again (not only the connection,
but any socket operation).

Another solution is to allocate a timeout budget specific to the
connection: we can apply a larger timeout only for connections, and once
the connection is established, set back the timeout configured through
opts->timeout_ms; this would allow connection to succeed under heavy CI
load, while keeping timeout reasonable for the rest of the test traffic.

Set a larger SO_SNDTIMEO/SO_RCVTIMEO for the connection step, and reset
it back to the timeout configured by the test once the connection has
succeeded.

Fixes: 99126ab ("bpf: selftests: A few improvements to network_helpers.c")
Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev>
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=1145882 irrelevant now. Closing PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant