On Sep 15, 2020, at 9:35 AM, Nicolas Rybowski nicolas.rybowski@tessares.net wrote:
Hi Song,
Thanks for the feedback !
On Mon, Sep 14, 2020 at 8:07 PM Song Liu song@kernel.org wrote:
On Fri, Sep 11, 2020 at 8:02 AM Nicolas Rybowski nicolas.rybowski@tessares.net wrote:
This patch adds a base for MPTCP specific tests.
It is currently limited to the is_mptcp field in case of plain TCP connection because for the moment there is no easy way to get the subflow sk from a msk in userspace. This implies that we cannot lookup the sk_storage attached to the subflow sk in the sockops program.
Acked-by: Matthieu Baerts matthieu.baerts@tessares.net Signed-off-by: Nicolas Rybowski nicolas.rybowski@tessares.net
Acked-by: Song Liu songliubraving@fb.com
With some nitpicks below.
Notes: v1 -> v2:
- new patch: mandatory selftests (Alexei)
[...]
int timeout_ms);diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c new file mode 100644 index 000000000000..0e65d64868e9 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <test_progs.h> +#include "cgroup_helpers.h" +#include "network_helpers.h"
+struct mptcp_storage {
__u32 invoked;__u32 is_mptcp;+};
+static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp) +{
int err = 0, cfd = client_fd;struct mptcp_storage val;/* Currently there is no easy way to get back the subflow sk from the MPTCP* sk, thus we cannot access here the sk_storage associated to the subflow* sk. Also, there is no sk_storage associated with the MPTCP sk since it* does not trigger sockops events.* We silently pass this situation at the moment.*/if (is_mptcp == 1)return 0;if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {perror("Failed to read socket storage");Maybe simplify this with CHECK(), which contains a customized error message? Same for some other calls.
The whole logic here is strongly inspired from prog_tests/tcp_rtt.c where CHECK_FAIL is used. Also the CHECK macro will print a PASS message on successful map lookup, which is not expected at this point of the tests. I think it would be more interesting to leave it as it is to keep a cohesion between TCP and MPTCP selftests. What do you think?
I guess CHECK_FAIL makes sense when we don't need the PASS message. Let's keep this part as-is then.
Thanks, Song