netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v3 0/3] selftests/bpf: new MPTCP subflow subtest
@ 2024-07-03 18:57 Matthieu Baerts (NGI0)
  2024-07-03 18:57 ` [PATCH bpf-next v3 1/3] selftests/bpf: Add mptcp subflow example Matthieu Baerts (NGI0)
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-07-03 18:57 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, Andrii Nakryiko,
	Eduard Zingerman, Mykola Lysenko, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, David S. Miller, Jakub Kicinski,
	Jesper Dangaard Brouer
  Cc: linux-kernel, netdev, bpf, linux-kselftest,
	Matthieu Baerts (NGI0), Nicolas Rybowski, Geliang Tang

In this series from Geliang, modifying MPTCP BPF selftests, we have:

- A new MPTCP subflow BPF program setting socket options per subflow: it
  looks better to have this old test program in the BPF selftests to
  track regressions and to serve as example.

  Note: Nicolas is no longer working for Tessares, but he did this work
  while working for them, and his email address is no longer available.

- A new symlink to MPTCP's pm_nl_ctl tool is added in BPF selftests, to
  be able to use it instead of 'ip mptcp' which is not supported by the
  BPF CI running IPRoute 5.5.0.

- A new MPTCP BPF subtest validating the new BPF program added in the
  first patch.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v3:
- Sorry for the delay between v2 and v3, this series was conflicting
  with the "add netns helpers", but it looks like it is on hold:
  https://lore.kernel.org/cover.1715821541.git.tanggeliang@kylinos.cn
- Patch 1/3 includes "bpf_tracing_net.h", introduced in between.
- New patch 2/3: "selftests/bpf: Add mptcp pm_nl_ctl link".
- Patch 3/3: use the tool introduced in patch 2/3 + SYS_NOFAIL() helper.
- Link to v2: https://lore.kernel.org/r/20240509-upstream-bpf-next-20240506-mptcp-subflow-test-v2-0-4048c2948665@kernel.org

Changes in v2:
- Previous patches 1/4 and 2/4 have been dropped from this series:
  - 1/4: "selftests/bpf: Handle SIGINT when creating netns":
    - A new version, more generic and no longer specific to MPTCP BPF
      selftest will be sent later, as part of a new series. (Alexei)
  - 2/4: "selftests/bpf: Add RUN_MPTCP_TEST macro":
    - Removed, not to hide helper functions in macros. (Alexei)
- The commit message of patch 1/2 has been clarified to avoid some
  possible confusions spot by Alexei.
- Link to v1: https://lore.kernel.org/r/20240507-upstream-bpf-next-20240506-mptcp-subflow-test-v1-0-e2bcbdf49857@kernel.org

---
Geliang Tang (2):
      selftests/bpf: Add mptcp pm_nl_ctl link
      selftests/bpf: Add mptcp subflow subtest

Nicolas Rybowski (1):
      selftests/bpf: Add mptcp subflow example

 MAINTAINERS                                       |   1 +
 tools/testing/selftests/bpf/Makefile              |   3 +-
 tools/testing/selftests/bpf/mptcp_pm_nl_ctl.c     |   1 +
 tools/testing/selftests/bpf/prog_tests/mptcp.c    | 104 ++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/mptcp_subflow.c |  59 ++++++++++++
 5 files changed, 167 insertions(+), 1 deletion(-)
---
base-commit: fd8db07705c55a995c42b1e71afc42faad675b0b
change-id: 20240506-upstream-bpf-next-20240506-mptcp-subflow-test-faef6654bfa3

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v3 1/3] selftests/bpf: Add mptcp subflow example
  2024-07-03 18:57 [PATCH bpf-next v3 0/3] selftests/bpf: new MPTCP subflow subtest Matthieu Baerts (NGI0)
@ 2024-07-03 18:57 ` Matthieu Baerts (NGI0)
  2024-07-03 18:57 ` [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link Matthieu Baerts (NGI0)
  2024-07-03 18:57 ` [PATCH bpf-next v3 3/3] selftests/bpf: Add mptcp subflow subtest Matthieu Baerts (NGI0)
  2 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-07-03 18:57 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, Andrii Nakryiko,
	Eduard Zingerman, Mykola Lysenko, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, David S. Miller, Jakub Kicinski,
	Jesper Dangaard Brouer
  Cc: linux-kernel, netdev, bpf, linux-kselftest,
	Matthieu Baerts (NGI0), Nicolas Rybowski, Geliang Tang

From: Nicolas Rybowski <nicolas.rybowski@tessares.net>

Move Nicolas' patch into bpf selftests directory. This example adds a
different mark (SO_MARK) on each subflow, and changes the TCP CC only on
the first subflow.

From the userspace, an application can do a setsockopt() on an MPTCP
socket, and typically the same value will be propagated to all subflows
(paths). If someone wants to have different values per subflow, the
recommended way is to use BPF. So it is good to add such example here,
and make sure there is no regressions.

This example shows how it is possible to:

    Identify the parent msk of an MPTCP subflow.
    Put different sockopt for each subflow of a same MPTCP connection.

Here especially, two different behaviours are implemented:

    A socket mark (SOL_SOCKET SO_MARK) is put on each subflow of a same
    MPTCP connection. The order of creation of the current subflow defines
    its mark. The TCP CC algorithm of the very first subflow of an MPTCP
    connection is set to "reno".

This is just to show it is possible to identify an MPTCP connection, and
set socket options, from different SOL levels, per subflow. "reno" has
been picked because it is built-in and usually not set as default one.
It is easy to verify with 'ss' that these modifications have been
applied correctly. That's what the next patch is going to do.

Nicolas' code comes from:

    commit 4d120186e4d6 ("bpf:examples: update mptcp_set_mark_kern.c")

from the MPTCP repo https://github.com/multipath-tcp/mptcp_net-next (the
"scripts" branch), and it has been adapted by Geliang.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/76
Co-developed-by: Geliang Tang <tanggeliang@kylinos.cn>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
Signed-off-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
 - v1 -> v2:
   - The commit message has been updated: why setting multiple socket
     options, why reno, the verification is done in a later patch
     (different author). (Alexei)
 - v2 -> v3:
   - Only #include "bpf_tracing_net.h", linked to:
     https://lore.kernel.org/20240509175026.3423614-1-martin.lau@linux.dev
---
 tools/testing/selftests/bpf/progs/mptcp_subflow.c | 59 +++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
new file mode 100644
index 000000000000..bc572e1d6df8
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020, Tessares SA. */
+/* Copyright (c) 2024, Kylin Software */
+
+/* vmlinux.h, bpf_helpers.h and other 'define' */
+#include "bpf_tracing_net.h"
+
+char _license[] SEC("license") = "GPL";
+
+char cc[TCP_CA_NAME_MAX] = "reno";
+
+/* Associate a subflow counter to each token */
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__uint(key_size, sizeof(__u32));
+	__uint(value_size, sizeof(__u32));
+	__uint(max_entries, 100);
+} mptcp_sf SEC(".maps");
+
+SEC("sockops")
+int mptcp_subflow(struct bpf_sock_ops *skops)
+{
+	__u32 init = 1, key, mark, *cnt;
+	struct mptcp_sock *msk;
+	struct bpf_sock *sk;
+	int err;
+
+	if (skops->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
+		return 1;
+
+	sk = skops->sk;
+	if (!sk)
+		return 1;
+
+	msk = bpf_skc_to_mptcp_sock(sk);
+	if (!msk)
+		return 1;
+
+	key = msk->token;
+	cnt = bpf_map_lookup_elem(&mptcp_sf, &key);
+	if (cnt) {
+		/* A new subflow is added to an existing MPTCP connection */
+		__sync_fetch_and_add(cnt, 1);
+		mark = *cnt;
+	} else {
+		/* A new MPTCP connection is just initiated and this is its primary subflow */
+		bpf_map_update_elem(&mptcp_sf, &key, &init, BPF_ANY);
+		mark = init;
+	}
+
+	/* Set the mark of the subflow's socket based on appearance order */
+	err = bpf_setsockopt(skops, SOL_SOCKET, SO_MARK, &mark, sizeof(mark));
+	if (err < 0)
+		return 1;
+	if (mark == 1)
+		err = bpf_setsockopt(skops, SOL_TCP, TCP_CONGESTION, cc, TCP_CA_NAME_MAX);
+
+	return 1;
+}

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link
  2024-07-03 18:57 [PATCH bpf-next v3 0/3] selftests/bpf: new MPTCP subflow subtest Matthieu Baerts (NGI0)
  2024-07-03 18:57 ` [PATCH bpf-next v3 1/3] selftests/bpf: Add mptcp subflow example Matthieu Baerts (NGI0)
@ 2024-07-03 18:57 ` Matthieu Baerts (NGI0)
  2024-07-04 10:48   ` Matthieu Baerts
  2024-07-03 18:57 ` [PATCH bpf-next v3 3/3] selftests/bpf: Add mptcp subflow subtest Matthieu Baerts (NGI0)
  2 siblings, 1 reply; 10+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-07-03 18:57 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, Andrii Nakryiko,
	Eduard Zingerman, Mykola Lysenko, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, David S. Miller, Jakub Kicinski,
	Jesper Dangaard Brouer
  Cc: linux-kernel, netdev, bpf, linux-kselftest,
	Matthieu Baerts (NGI0), Geliang Tang

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a symlink to MPTCP's pm_nl_ctl tool into bpf selftests,
and updates Makefile to compile it.

This is useful to run MPTCP BPF selftests on systems with an old version
of IPRoute2. This tool can be used as an alternative to 'ip mptcp'.

MAINTAINERS needs to be updated since a new file is added in a non
covered place.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 MAINTAINERS                                   | 1 +
 tools/testing/selftests/bpf/Makefile          | 3 ++-
 tools/testing/selftests/bpf/mptcp_pm_nl_ctl.c | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index cd3277a98cfe..4ea5db496698 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15756,6 +15756,7 @@ F:	include/trace/events/mptcp.h
 F:	include/uapi/linux/mptcp*.h
 F:	net/mptcp/
 F:	tools/testing/selftests/bpf/*/*mptcp*.c
+F:	tools/testing/selftests/bpf/*mptcp*.c
 F:	tools/testing/selftests/net/mptcp/
 
 NETWORKING [TCP]
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index e0b3887b3d2d..204269d0b5b8 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \
 	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
 	test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
 	xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
-	xdp_features bpf_test_no_cfi.ko
+	xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl
 
 TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi
 
@@ -645,6 +645,7 @@ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko	\
 		       $(OUTPUT)/xdp_synproxy				\
 		       $(OUTPUT)/sign-file				\
 		       $(OUTPUT)/uprobe_multi				\
+		       $(OUTPUT)/mptcp_pm_nl_ctl			\
 		       ima_setup.sh 					\
 		       verify_sig_setup.sh				\
 		       $(wildcard progs/btf_dump_test_case_*.c)		\
diff --git a/tools/testing/selftests/bpf/mptcp_pm_nl_ctl.c b/tools/testing/selftests/bpf/mptcp_pm_nl_ctl.c
new file mode 120000
index 000000000000..5a08c255b278
--- /dev/null
+++ b/tools/testing/selftests/bpf/mptcp_pm_nl_ctl.c
@@ -0,0 +1 @@
+../net/mptcp/pm_nl_ctl.c
\ No newline at end of file

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v3 3/3] selftests/bpf: Add mptcp subflow subtest
  2024-07-03 18:57 [PATCH bpf-next v3 0/3] selftests/bpf: new MPTCP subflow subtest Matthieu Baerts (NGI0)
  2024-07-03 18:57 ` [PATCH bpf-next v3 1/3] selftests/bpf: Add mptcp subflow example Matthieu Baerts (NGI0)
  2024-07-03 18:57 ` [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link Matthieu Baerts (NGI0)
@ 2024-07-03 18:57 ` Matthieu Baerts (NGI0)
  2 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-07-03 18:57 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, Andrii Nakryiko,
	Eduard Zingerman, Mykola Lysenko, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	Shuah Khan, David S. Miller, Jakub Kicinski,
	Jesper Dangaard Brouer
  Cc: linux-kernel, netdev, bpf, linux-kselftest,
	Matthieu Baerts (NGI0), Geliang Tang

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a subtest named test_subflow to load and verify the newly
added mptcp subflow example in test_mptcp. Add a helper endpoint_init()
to add a new subflow endpoint. Add another helper ss_search() to verify the
fwmark and congestion values set by mptcp_subflow prog using setsockopts.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/76
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
 - v2 -> v3:
   - Use './mptcp_pm_nl_ctl' instead of 'ip mptcp', not supported by the
     BPF CI running IPRoute 5.5.0.
   - Use SYS_NOFAIL() in _ss_search() instead of calling system()
---
 tools/testing/selftests/bpf/prog_tests/mptcp.c | 104 +++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index d2ca32fa3b21..975427b3c66e 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -9,8 +9,12 @@
 #include "network_helpers.h"
 #include "mptcp_sock.skel.h"
 #include "mptcpify.skel.h"
+#include "mptcp_subflow.skel.h"
 
 #define NS_TEST "mptcp_ns"
+#define ADDR_1	"10.0.1.1"
+#define ADDR_2	"10.0.1.2"
+#define PORT_1	10001
 
 #ifndef IPPROTO_MPTCP
 #define IPPROTO_MPTCP 262
@@ -335,10 +339,110 @@ static void test_mptcpify(void)
 	close(cgroup_fd);
 }
 
+static int endpoint_init(char *flags)
+{
+	SYS(fail, "ip -net %s link add veth1 type veth peer name veth2", NS_TEST);
+	SYS(fail, "ip -net %s addr add %s/24 dev veth1", NS_TEST, ADDR_1);
+	SYS(fail, "ip -net %s link set dev veth1 up", NS_TEST);
+	SYS(fail, "ip -net %s addr add %s/24 dev veth2", NS_TEST, ADDR_2);
+	SYS(fail, "ip -net %s link set dev veth2 up", NS_TEST);
+	/* It would be better to use  "ip -net %s mptcp endpoint add %s %s",
+	 * but the BPF CI is using an old version of IPRoute (5.5.0).
+	 */
+	SYS(fail, "ip netns exec %s ./mptcp_pm_nl_ctl add %s flags %s", NS_TEST, ADDR_2, flags);
+
+	return 0;
+fail:
+	return -1;
+}
+
+static int _ss_search(char *src, char *dst, char *port, char *keyword)
+{
+	return SYS_NOFAIL("ip netns exec %s ss -enita src %s dst %s %s %d | grep -q '%s'",
+			  NS_TEST, src, dst, port, PORT_1, keyword);
+}
+
+static int ss_search(char *src, char *keyword)
+{
+	return _ss_search(src, ADDR_1, "dport", keyword);
+}
+
+static void run_subflow(char *new)
+{
+	int server_fd, client_fd, err;
+	char cc[TCP_CA_NAME_MAX];
+	socklen_t len = sizeof(cc);
+
+	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
+	if (!ASSERT_GE(server_fd, 0, "start_mptcp_server"))
+		return;
+
+	client_fd = connect_to_fd(server_fd, 0);
+	if (!ASSERT_GE(client_fd, 0, "connect to fd"))
+		goto fail;
+
+	err = getsockopt(server_fd, SOL_TCP, TCP_CONGESTION, cc, &len);
+	if (!ASSERT_OK(err, "getsockopt(srv_fd, TCP_CONGESTION)"))
+		goto fail;
+
+	send_byte(client_fd);
+
+	ASSERT_OK(ss_search(ADDR_1, "fwmark:0x1"), "ss_search fwmark:0x1");
+	ASSERT_OK(ss_search(ADDR_2, "fwmark:0x2"), "ss_search fwmark:0x2");
+	ASSERT_OK(ss_search(ADDR_1, new), "ss_search new cc");
+	ASSERT_OK(ss_search(ADDR_2, cc), "ss_search default cc");
+
+	close(client_fd);
+fail:
+	close(server_fd);
+}
+
+static void test_subflow(void)
+{
+	int cgroup_fd, prog_fd, err;
+	struct mptcp_subflow *skel;
+	struct nstoken *nstoken;
+
+	cgroup_fd = test__join_cgroup("/mptcp_subflow");
+	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup: mptcp_subflow"))
+		return;
+
+	skel = mptcp_subflow__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open_load: mptcp_subflow"))
+		goto close_cgroup;
+
+	err = mptcp_subflow__attach(skel);
+	if (!ASSERT_OK(err, "skel_attach: mptcp_subflow"))
+		goto skel_destroy;
+
+	prog_fd = bpf_program__fd(skel->progs.mptcp_subflow);
+	err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SOCK_OPS, 0);
+	if (!ASSERT_OK(err, "prog_attach"))
+		goto skel_destroy;
+
+	nstoken = create_netns();
+	if (!ASSERT_OK_PTR(nstoken, "create_netns: mptcp_subflow"))
+		goto skel_destroy;
+
+	if (!ASSERT_OK(endpoint_init("subflow"), "endpoint_init"))
+		goto close_netns;
+
+	run_subflow(skel->data->cc);
+
+close_netns:
+	cleanup_netns(nstoken);
+skel_destroy:
+	mptcp_subflow__destroy(skel);
+close_cgroup:
+	close(cgroup_fd);
+}
+
 void test_mptcp(void)
 {
 	if (test__start_subtest("base"))
 		test_base();
 	if (test__start_subtest("mptcpify"))
 		test_mptcpify();
+	if (test__start_subtest("subflow"))
+		test_subflow();
 }

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link
  2024-07-03 18:57 ` [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link Matthieu Baerts (NGI0)
@ 2024-07-04 10:48   ` Matthieu Baerts
  2024-07-05 23:10     ` Martin KaFai Lau
  0 siblings, 1 reply; 10+ messages in thread
From: Matthieu Baerts @ 2024-07-04 10:48 UTC (permalink / raw)
  To: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, Jiri Olsa, David S. Miller, Jakub Kicinski,
	Jesper Dangaard Brouer
  Cc: linux-kernel, netdev, bpf, linux-kselftest, Geliang Tang, mptcp,
	Mat Martineau, Geliang Tang, Shuah Khan

Hello,

@BPF people: this new tool doesn't compile on the BPF CI [1]. Can I have
a hand to find the best way to fix this please? (see below)

[1] https://github.com/kernel-patches/bpf/pull/7296

On 03/07/2024 20:57, Matthieu Baerts (NGI0) wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a symlink to MPTCP's pm_nl_ctl tool into bpf selftests,
> and updates Makefile to compile it.
> 
> This is useful to run MPTCP BPF selftests on systems with an old version
> of IPRoute2. This tool can be used as an alternative to 'ip mptcp'.

(...)

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index e0b3887b3d2d..204269d0b5b8 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \
>  	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
>  	test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
>  	xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
> -	xdp_features bpf_test_no_cfi.ko
> +	xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl
On the BPF CI, we have such errors:

   mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file not found
     20 | #include "linux/mptcp.h"
        |          ^~~~~~~~~~~~~~~

On my side, I don't have any issue, because the compiler uses the
mptcp.h file from the system: /usr/include/linux/mptcp.h

I suppose that's not OK on the BPF CI, as it looks like it doesn't have
this file there, probably because it still uses Ubuntu 20.04 as base,
which doesn't include this file in the linux-libc-dev package.

When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the other
programs from that list -- is compiled (V=1), I see that the following
"-I" options are given:

  -I${PWD}/tools/testing/selftests/bpf
  -I${BUILD}//tools/include
  -I${BUILD}/include/generated
  -I${PWD}/tools/lib
  -I${PWD}/tools/include
  -I${PWD}/tools/include/uapi
  -I${BUILD}/

It will then not look at -I${PWD}/usr/include or the directory generated
with:

  make headers_install INSTALL_HDR_PATH=(...)

I guess that's why people have duplicated files in 'tools/include/uapi',
but I also understood from Jakub that it is not a good idea to continue
to do so.

What would be the best solution to avoid a copy? A symlink still looks
like a workaround.

In the other selftests, KHDR_INCLUDES is used to be able to include the
path containing the UAPI headers. So if someone built the headers in a
seperated directory -- INSTALL_HDR_PATH=(...) -- KHDR_INCLUDES can be
overridden to look there, instead of ${KERNEL_SRC}/usr/include. Would it
be OK to do that? Would it work for the CI without extra changes? Or do
you still prefer a copy/symlink to 'tools/include/uapi' instead?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link
  2024-07-04 10:48   ` Matthieu Baerts
@ 2024-07-05 23:10     ` Martin KaFai Lau
  2024-07-06  0:25       ` Matthieu Baerts
  0 siblings, 1 reply; 10+ messages in thread
From: Martin KaFai Lau @ 2024-07-05 23:10 UTC (permalink / raw)
  To: Matthieu Baerts
  Cc: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	linux-kernel, netdev, bpf, linux-kselftest, Geliang Tang, mptcp,
	Mat Martineau, Geliang Tang, Shuah Khan

On 7/4/24 3:48 AM, Matthieu Baerts wrote:
>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> index e0b3887b3d2d..204269d0b5b8 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \
>>   	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
>>   	test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
>>   	xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
>> -	xdp_features bpf_test_no_cfi.ko
>> +	xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl
> On the BPF CI, we have such errors:
> 
>     mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file not found
>       20 | #include "linux/mptcp.h"
>          |          ^~~~~~~~~~~~~~~
> 
> On my side, I don't have any issue, because the compiler uses the
> mptcp.h file from the system: /usr/include/linux/mptcp.h
> 
> I suppose that's not OK on the BPF CI, as it looks like it doesn't have
> this file there, probably because it still uses Ubuntu 20.04 as base,
> which doesn't include this file in the linux-libc-dev package.
> 
> When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the other
> programs from that list -- is compiled (V=1), I see that the following
> "-I" options are given:
> 
>    -I${PWD}/tools/testing/selftests/bpf
>    -I${BUILD}//tools/include
>    -I${BUILD}/include/generated
>    -I${PWD}/tools/lib
>    -I${PWD}/tools/include
>    -I${PWD}/tools/include/uapi
>    -I${BUILD}/
> 
> It will then not look at -I${PWD}/usr/include or the directory generated
> with:
> 
>    make headers_install INSTALL_HDR_PATH=(...)

It sounds like the tools/testing/selftests/net/mptcp/Makefile is looking at this 
include path, so it works?

iiu the bpf/Makefile correctly, it has the bpftool "make" compiled and installed 
at tools/testing/selftests/bpf/tools/sbin/. May be directly compile the 
pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"?

> 
> I guess that's why people have duplicated files in 'tools/include/uapi',
> but I also understood from Jakub that it is not a good idea to continue
> to do so.
> 
> What would be the best solution to avoid a copy? A symlink still looks
> like a workaround.
> 
> In the other selftests, KHDR_INCLUDES is used to be able to include the
> path containing the UAPI headers. So if someone built the headers in a

Meaning KHDR_INCLUDES should be used and -I${PWD}/tools/include/uapi can be 
retired? I haven't looked into the details. I quickly tried but it fails in my 
environment.

> seperated directory -- INSTALL_HDR_PATH=(...) -- KHDR_INCLUDES can be
> overridden to look there, instead of ${KERNEL_SRC}/usr/include. Would it
> be OK to do that? Would it work for the CI without extra changes? Or do
> you still prefer a copy/symlink to 'tools/include/uapi' instead?

> 
> Cheers,
> Matt


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link
  2024-07-05 23:10     ` Martin KaFai Lau
@ 2024-07-06  0:25       ` Matthieu Baerts
  2024-07-24  7:42         ` Geliang Tang
  0 siblings, 1 reply; 10+ messages in thread
From: Matthieu Baerts @ 2024-07-06  0:25 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	linux-kernel, netdev, bpf, linux-kselftest, Geliang Tang, mptcp,
	Mat Martineau, Geliang Tang, Shuah Khan

Hi Martin,

Thank you for your reply!

On 06/07/2024 01:10, Martin KaFai Lau wrote:
> On 7/4/24 3:48 AM, Matthieu Baerts wrote:
>>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/
>>> selftests/bpf/Makefile
>>> index e0b3887b3d2d..204269d0b5b8 100644
>>> --- a/tools/testing/selftests/bpf/Makefile
>>> +++ b/tools/testing/selftests/bpf/Makefile
>>> @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \
>>>       flow_dissector_load test_flow_dissector
>>> test_tcp_check_syncookie_user \
>>>       test_lirc_mode2_user xdping test_cpp runqslower bench
>>> bpf_testmod.ko \
>>>       xskxceiver xdp_redirect_multi xdp_synproxy veristat
>>> xdp_hw_metadata \
>>> -    xdp_features bpf_test_no_cfi.ko
>>> +    xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl
>> On the BPF CI, we have such errors:
>>
>>     mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file not found
>>       20 | #include "linux/mptcp.h"
>>          |          ^~~~~~~~~~~~~~~
>>
>> On my side, I don't have any issue, because the compiler uses the
>> mptcp.h file from the system: /usr/include/linux/mptcp.h
>>
>> I suppose that's not OK on the BPF CI, as it looks like it doesn't have
>> this file there, probably because it still uses Ubuntu 20.04 as base,
>> which doesn't include this file in the linux-libc-dev package.
>>
>> When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the other
>> programs from that list -- is compiled (V=1), I see that the following
>> "-I" options are given:
>>
>>    -I${PWD}/tools/testing/selftests/bpf
>>    -I${BUILD}//tools/include
>>    -I${BUILD}/include/generated
>>    -I${PWD}/tools/lib
>>    -I${PWD}/tools/include
>>    -I${PWD}/tools/include/uapi
>>    -I${BUILD}/
>>
>> It will then not look at -I${PWD}/usr/include or the directory generated
>> with:
>>
>>    make headers_install INSTALL_HDR_PATH=(...)
> 
> It sounds like the tools/testing/selftests/net/mptcp/Makefile is looking
> at this include path, so it works?

Yes it does work.

> iiu the bpf/Makefile correctly, it has the bpftool "make" compiled and
> installed at tools/testing/selftests/bpf/tools/sbin/. May be directly
> compile the pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"?

That could be an alternative, I didn't know it would be OK to add such
dependence, good idea.

>> I guess that's why people have duplicated files in 'tools/include/uapi',
>> but I also understood from Jakub that it is not a good idea to continue
>> to do so.
>>
>> What would be the best solution to avoid a copy? A symlink still looks
>> like a workaround.
>>
>> In the other selftests, KHDR_INCLUDES is used to be able to include the
>> path containing the UAPI headers. So if someone built the headers in a
> 
> Meaning KHDR_INCLUDES should be used and -I${PWD}/tools/include/uapi can
> be retired?

That's the idea, yes, for "userspace programs". I mean: for BPF programs
requiring vmlinux.h (BPF_CFLAGS), I guess you will still need the bpf.h
file from tools/include/uapi, no?

> I haven't looked into the details. I quickly tried but it
> fails in my environment.

Do you not have issues because some files have something like:

  #include <uapi/linux/(...).h>

On my side, I had a working version using this patch:

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 7c5827d20c2e..112f14d40852 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -37,7 +37,7 @@ CFLAGS += -g $(OPT_FLAGS) -rdynamic            \
>           -Wall -Werror -fno-omit-frame-pointer                  \
>           $(GENFLAGS) $(SAN_CFLAGS) $(LIBELF_CFLAGS)             \
>           -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)   \
> -         -I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT)
> +         -I$(TOOLSINCDIR) $(KHDR_INCLUDES) -I$(OUTPUT)
>  LDFLAGS += $(SAN_LDFLAGS)
>  LDLIBS += $(LIBELF_LIBS) -lz -lrt -lpthread
>  

But only after having removed these extra 'uapi/':

  $ git grep -l '<uapi/' -- tools/testing/selftests/bpf | \
    xargs sed -i 's|#include <uapi/|#include <|g'

Is it not OK for you like that?

Note that I built the selftests using KHDR_INCLUDES=-I$INSTALL_HDR_PATH.

>> seperated directory -- INSTALL_HDR_PATH=(...) -- KHDR_INCLUDES can be
>> overridden to look there, instead of ${KERNEL_SRC}/usr/include. Would it
>> be OK to do that? Would it work for the CI without extra changes? Or do
>> you still prefer a copy/symlink to 'tools/include/uapi' instead?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link
  2024-07-06  0:25       ` Matthieu Baerts
@ 2024-07-24  7:42         ` Geliang Tang
  2024-07-24  8:24           ` Matthieu Baerts
  0 siblings, 1 reply; 10+ messages in thread
From: Geliang Tang @ 2024-07-24  7:42 UTC (permalink / raw)
  To: Matthieu Baerts, Martin KaFai Lau
  Cc: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer, netdev,
	bpf, linux-kselftest, Geliang Tang, mptcp, Mat Martineau,
	Shuah Khan

Hi Matt,

On Sat, 2024-07-06 at 02:25 +0200, Matthieu Baerts wrote:
> Hi Martin,
> 
> Thank you for your reply!
> 
> On 06/07/2024 01:10, Martin KaFai Lau wrote:
> > On 7/4/24 3:48 AM, Matthieu Baerts wrote:
> > > > diff --git a/tools/testing/selftests/bpf/Makefile
> > > > b/tools/testing/
> > > > selftests/bpf/Makefile
> > > > index e0b3887b3d2d..204269d0b5b8 100644
> > > > --- a/tools/testing/selftests/bpf/Makefile
> > > > +++ b/tools/testing/selftests/bpf/Makefile
> > > > @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED =
> > > > test_skb_cgroup_id_user \
> > > >       flow_dissector_load test_flow_dissector
> > > > test_tcp_check_syncookie_user \
> > > >       test_lirc_mode2_user xdping test_cpp runqslower bench
> > > > bpf_testmod.ko \
> > > >       xskxceiver xdp_redirect_multi xdp_synproxy veristat
> > > > xdp_hw_metadata \
> > > > -    xdp_features bpf_test_no_cfi.ko
> > > > +    xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl
> > > On the BPF CI, we have such errors:
> > > 
> > >     mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file
> > > not found
> > >       20 | #include "linux/mptcp.h"
> > >          |          ^~~~~~~~~~~~~~~
> > > 
> > > On my side, I don't have any issue, because the compiler uses the
> > > mptcp.h file from the system: /usr/include/linux/mptcp.h
> > > 
> > > I suppose that's not OK on the BPF CI, as it looks like it
> > > doesn't have
> > > this file there, probably because it still uses Ubuntu 20.04 as
> > > base,
> > > which doesn't include this file in the linux-libc-dev package.
> > > 
> > > When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the
> > > other
> > > programs from that list -- is compiled (V=1), I see that the
> > > following
> > > "-I" options are given:
> > > 
> > >    -I${PWD}/tools/testing/selftests/bpf
> > >    -I${BUILD}//tools/include
> > >    -I${BUILD}/include/generated
> > >    -I${PWD}/tools/lib
> > >    -I${PWD}/tools/include
> > >    -I${PWD}/tools/include/uapi
> > >    -I${BUILD}/
> > > 
> > > It will then not look at -I${PWD}/usr/include or the directory
> > > generated
> > > with:
> > > 
> > >    make headers_install INSTALL_HDR_PATH=(...)
> > 
> > It sounds like the tools/testing/selftests/net/mptcp/Makefile is
> > looking
> > at this include path, so it works?
> 
> Yes it does work.
> 
> > iiu the bpf/Makefile correctly, it has the bpftool "make" compiled
> > and
> > installed at tools/testing/selftests/bpf/tools/sbin/. May be
> > directly
> > compile the pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"?
> 
> That could be an alternative, I didn't know it would be OK to add
> such
> dependence, good idea.
> 
> > > I guess that's why people have duplicated files in
> > > 'tools/include/uapi',
> > > but I also understood from Jakub that it is not a good idea to
> > > continue
> > > to do so.
> > > 
> > > What would be the best solution to avoid a copy? A symlink still
> > > looks
> > > like a workaround.
> > > 
> > > In the other selftests, KHDR_INCLUDES is used to be able to
> > > include the
> > > path containing the UAPI headers. So if someone built the headers
> > > in a
> > 
> > Meaning KHDR_INCLUDES should be used and -
> > I${PWD}/tools/include/uapi can
> > be retired?
> 
> That's the idea, yes, for "userspace programs". I mean: for BPF
> programs
> requiring vmlinux.h (BPF_CFLAGS), I guess you will still need the
> bpf.h
> file from tools/include/uapi, no?
> 
> > I haven't looked into the details. I quickly tried but it
> > fails in my environment.
> 
> Do you not have issues because some files have something like:
> 
>   #include <uapi/linux/(...).h>
> 
> On my side, I had a working version using this patch:
> 
> > diff --git a/tools/testing/selftests/bpf/Makefile
> > b/tools/testing/selftests/bpf/Makefile
> > index 7c5827d20c2e..112f14d40852 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -37,7 +37,7 @@ CFLAGS += -g $(OPT_FLAGS) -rdynamic            \
> >           -Wall -Werror -fno-omit-frame-pointer                  \
> >           $(GENFLAGS) $(SAN_CFLAGS) $(LIBELF_CFLAGS)             \
> >           -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)   \
> > -         -I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT)
> > +         -I$(TOOLSINCDIR) $(KHDR_INCLUDES) -I$(OUTPUT)
> >  LDFLAGS += $(SAN_LDFLAGS)
> >  LDLIBS += $(LIBELF_LIBS) -lz -lrt -lpthread
> >  
> 
> But only after having removed these extra 'uapi/':
> 
>   $ git grep -l '<uapi/' -- tools/testing/selftests/bpf | \
>     xargs sed -i 's|#include <uapi/|#include <|g'
> 
> Is it not OK for you like that?
> 
> Note that I built the selftests using KHDR_INCLUDES=-
> I$INSTALL_HDR_PATH.

Do you need me to do anything here? This patchset seems to have been
waiting for several months.

Another option is to roll back to v2, not add this mptcp_pm_nl_ctl
tool, and continue to use "ip mptcp". I remember mentioning in the
comments of v2 that BPF CI systems will also be upgraded to new Ubuntu
system and iproute2 in the future. At this time now we can make a check
for "ip mptcp" and only run this test on systems that support "ip
mptcp", and skip the test with test__skip() for systems that do not
support it, so that CI can also pass.

WDYT?

Thanks,
-Geliang

> 
> > > seperated directory -- INSTALL_HDR_PATH=(...) -- KHDR_INCLUDES
> > > can be
> > > overridden to look there, instead of ${KERNEL_SRC}/usr/include.
> > > Would it
> > > be OK to do that? Would it work for the CI without extra changes?
> > > Or do
> > > you still prefer a copy/symlink to 'tools/include/uapi' instead?
> 
> Cheers,
> Matt


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link
  2024-07-24  7:42         ` Geliang Tang
@ 2024-07-24  8:24           ` Matthieu Baerts
  2024-07-24 19:47             ` Martin KaFai Lau
  0 siblings, 1 reply; 10+ messages in thread
From: Matthieu Baerts @ 2024-07-24  8:24 UTC (permalink / raw)
  To: Geliang Tang, Martin KaFai Lau
  Cc: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer, netdev,
	bpf, linux-kselftest, Geliang Tang, mptcp, Mat Martineau,
	Shuah Khan

Hi Geliang,

Thank you for your reply!

On 24/07/2024 09:42, Geliang Tang wrote:
> Hi Matt,
> 
> On Sat, 2024-07-06 at 02:25 +0200, Matthieu Baerts wrote:
>> Hi Martin,
>>
>> Thank you for your reply!
>>
>> On 06/07/2024 01:10, Martin KaFai Lau wrote:
>>> On 7/4/24 3:48 AM, Matthieu Baerts wrote:
>>>>> diff --git a/tools/testing/selftests/bpf/Makefile
>>>>> b/tools/testing/
>>>>> selftests/bpf/Makefile
>>>>> index e0b3887b3d2d..204269d0b5b8 100644
>>>>> --- a/tools/testing/selftests/bpf/Makefile
>>>>> +++ b/tools/testing/selftests/bpf/Makefile
>>>>> @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED =
>>>>> test_skb_cgroup_id_user \
>>>>>       flow_dissector_load test_flow_dissector
>>>>> test_tcp_check_syncookie_user \
>>>>>       test_lirc_mode2_user xdping test_cpp runqslower bench
>>>>> bpf_testmod.ko \
>>>>>       xskxceiver xdp_redirect_multi xdp_synproxy veristat
>>>>> xdp_hw_metadata \
>>>>> -    xdp_features bpf_test_no_cfi.ko
>>>>> +    xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl
>>>> On the BPF CI, we have such errors:
>>>>
>>>>     mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file
>>>> not found
>>>>       20 | #include "linux/mptcp.h"
>>>>          |          ^~~~~~~~~~~~~~~
>>>>
>>>> On my side, I don't have any issue, because the compiler uses the
>>>> mptcp.h file from the system: /usr/include/linux/mptcp.h
>>>>
>>>> I suppose that's not OK on the BPF CI, as it looks like it
>>>> doesn't have
>>>> this file there, probably because it still uses Ubuntu 20.04 as
>>>> base,
>>>> which doesn't include this file in the linux-libc-dev package.
>>>>
>>>> When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the
>>>> other
>>>> programs from that list -- is compiled (V=1), I see that the
>>>> following
>>>> "-I" options are given:
>>>>
>>>>    -I${PWD}/tools/testing/selftests/bpf
>>>>    -I${BUILD}//tools/include
>>>>    -I${BUILD}/include/generated
>>>>    -I${PWD}/tools/lib
>>>>    -I${PWD}/tools/include
>>>>    -I${PWD}/tools/include/uapi
>>>>    -I${BUILD}/
>>>>
>>>> It will then not look at -I${PWD}/usr/include or the directory
>>>> generated
>>>> with:
>>>>
>>>>    make headers_install INSTALL_HDR_PATH=(...)
>>>
>>> It sounds like the tools/testing/selftests/net/mptcp/Makefile is
>>> looking
>>> at this include path, so it works?
>>
>> Yes it does work.
>>
>>> iiu the bpf/Makefile correctly, it has the bpftool "make" compiled
>>> and
>>> installed at tools/testing/selftests/bpf/tools/sbin/. May be
>>> directly
>>> compile the pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"?
>>
>> That could be an alternative, I didn't know it would be OK to add
>> such
>> dependence, good idea.
>>
>>>> I guess that's why people have duplicated files in
>>>> 'tools/include/uapi',
>>>> but I also understood from Jakub that it is not a good idea to
>>>> continue
>>>> to do so.
>>>>
>>>> What would be the best solution to avoid a copy? A symlink still
>>>> looks
>>>> like a workaround.
>>>>
>>>> In the other selftests, KHDR_INCLUDES is used to be able to
>>>> include the
>>>> path containing the UAPI headers. So if someone built the headers
>>>> in a
>>>
>>> Meaning KHDR_INCLUDES should be used and -
>>> I${PWD}/tools/include/uapi can
>>> be retired?
>>
>> That's the idea, yes, for "userspace programs". I mean: for BPF
>> programs
>> requiring vmlinux.h (BPF_CFLAGS), I guess you will still need the
>> bpf.h
>> file from tools/include/uapi, no?
>>
>>> I haven't looked into the details. I quickly tried but it
>>> fails in my environment.
>>
>> Do you not have issues because some files have something like:
>>
>>   #include <uapi/linux/(...).h>
>>
>> On my side, I had a working version using this patch:
>>
>>> diff --git a/tools/testing/selftests/bpf/Makefile
>>> b/tools/testing/selftests/bpf/Makefile
>>> index 7c5827d20c2e..112f14d40852 100644
>>> --- a/tools/testing/selftests/bpf/Makefile
>>> +++ b/tools/testing/selftests/bpf/Makefile
>>> @@ -37,7 +37,7 @@ CFLAGS += -g $(OPT_FLAGS) -rdynamic            \
>>>           -Wall -Werror -fno-omit-frame-pointer                  \
>>>           $(GENFLAGS) $(SAN_CFLAGS) $(LIBELF_CFLAGS)             \
>>>           -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)   \
>>> -         -I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT)
>>> +         -I$(TOOLSINCDIR) $(KHDR_INCLUDES) -I$(OUTPUT)
>>>  LDFLAGS += $(SAN_LDFLAGS)
>>>  LDLIBS += $(LIBELF_LIBS) -lz -lrt -lpthread
>>>  
>>
>> But only after having removed these extra 'uapi/':
>>
>>   $ git grep -l '<uapi/' -- tools/testing/selftests/bpf | \
>>     xargs sed -i 's|#include <uapi/|#include <|g'
>>
>> Is it not OK for you like that?
>>
>> Note that I built the selftests using KHDR_INCLUDES=-
>> I$INSTALL_HDR_PATH.
> 
> Do you need me to do anything here? This patchset seems to have been
> waiting for several months.

Sorry, I was not sure my suggestion here above was OK for Martin and
other BPF maintainers. Maybe sending a proper patch implementing these
modifications would be easier?

> Another option is to roll back to v2, not add this mptcp_pm_nl_ctl
> tool, and continue to use "ip mptcp". I remember mentioning in the
> comments of v2 that BPF CI systems will also be upgraded to new Ubuntu
> system and iproute2 in the future. At this time now we can make a check
> for "ip mptcp" and only run this test on systems that support "ip
> mptcp", and skip the test with test__skip() for systems that do not
> support it, so that CI can also pass.

I think it would be good to get rid of some duplicated header files, but
well, that's not directly linked to the new test we want to add, it is
more something to ease the maintenance of these duplicated files.

Then yes, it might be good to skip the test if "ip mptcp" is not supported.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link
  2024-07-24  8:24           ` Matthieu Baerts
@ 2024-07-24 19:47             ` Martin KaFai Lau
  0 siblings, 0 replies; 10+ messages in thread
From: Martin KaFai Lau @ 2024-07-24 19:47 UTC (permalink / raw)
  To: Matthieu Baerts
  Cc: Geliang Tang, Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
	Alexei Starovoitov, Daniel Borkmann, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer, netdev,
	bpf, linux-kselftest, Geliang Tang, mptcp, Mat Martineau,
	Shuah Khan

On 7/24/24 1:24 AM, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for your reply!
> 
> On 24/07/2024 09:42, Geliang Tang wrote:
>> Hi Matt,
>>
>> On Sat, 2024-07-06 at 02:25 +0200, Matthieu Baerts wrote:
>>> Hi Martin,
>>>
>>> Thank you for your reply!
>>>
>>> On 06/07/2024 01:10, Martin KaFai Lau wrote:
>>>> On 7/4/24 3:48 AM, Matthieu Baerts wrote:
>>>>>> diff --git a/tools/testing/selftests/bpf/Makefile
>>>>>> b/tools/testing/
>>>>>> selftests/bpf/Makefile
>>>>>> index e0b3887b3d2d..204269d0b5b8 100644
>>>>>> --- a/tools/testing/selftests/bpf/Makefile
>>>>>> +++ b/tools/testing/selftests/bpf/Makefile
>>>>>> @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED =
>>>>>> test_skb_cgroup_id_user \
>>>>>>        flow_dissector_load test_flow_dissector
>>>>>> test_tcp_check_syncookie_user \
>>>>>>        test_lirc_mode2_user xdping test_cpp runqslower bench
>>>>>> bpf_testmod.ko \
>>>>>>        xskxceiver xdp_redirect_multi xdp_synproxy veristat
>>>>>> xdp_hw_metadata \
>>>>>> -    xdp_features bpf_test_no_cfi.ko
>>>>>> +    xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl
>>>>> On the BPF CI, we have such errors:
>>>>>
>>>>>      mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file
>>>>> not found
>>>>>        20 | #include "linux/mptcp.h"
>>>>>           |          ^~~~~~~~~~~~~~~
>>>>>
>>>>> On my side, I don't have any issue, because the compiler uses the
>>>>> mptcp.h file from the system: /usr/include/linux/mptcp.h
>>>>>
>>>>> I suppose that's not OK on the BPF CI, as it looks like it
>>>>> doesn't have
>>>>> this file there, probably because it still uses Ubuntu 20.04 as
>>>>> base,
>>>>> which doesn't include this file in the linux-libc-dev package.
>>>>>
>>>>> When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the
>>>>> other
>>>>> programs from that list -- is compiled (V=1), I see that the
>>>>> following
>>>>> "-I" options are given:
>>>>>
>>>>>     -I${PWD}/tools/testing/selftests/bpf
>>>>>     -I${BUILD}//tools/include
>>>>>     -I${BUILD}/include/generated
>>>>>     -I${PWD}/tools/lib
>>>>>     -I${PWD}/tools/include
>>>>>     -I${PWD}/tools/include/uapi
>>>>>     -I${BUILD}/
>>>>>
>>>>> It will then not look at -I${PWD}/usr/include or the directory
>>>>> generated
>>>>> with:
>>>>>
>>>>>     make headers_install INSTALL_HDR_PATH=(...)
>>>>
>>>> It sounds like the tools/testing/selftests/net/mptcp/Makefile is
>>>> looking
>>>> at this include path, so it works?
>>>
>>> Yes it does work.
>>>
>>>> iiu the bpf/Makefile correctly, it has the bpftool "make" compiled
>>>> and
>>>> installed at tools/testing/selftests/bpf/tools/sbin/. May be
>>>> directly
>>>> compile the pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"?
>>>
>>> That could be an alternative, I didn't know it would be OK to add
>>> such
>>> dependence, good idea.
>>>
>>>>> I guess that's why people have duplicated files in
>>>>> 'tools/include/uapi',
>>>>> but I also understood from Jakub that it is not a good idea to
>>>>> continue
>>>>> to do so.
>>>>>
>>>>> What would be the best solution to avoid a copy? A symlink still
>>>>> looks
>>>>> like a workaround.
>>>>>
>>>>> In the other selftests, KHDR_INCLUDES is used to be able to
>>>>> include the
>>>>> path containing the UAPI headers. So if someone built the headers
>>>>> in a
>>>>
>>>> Meaning KHDR_INCLUDES should be used and -
>>>> I${PWD}/tools/include/uapi can
>>>> be retired?
>>>
>>> That's the idea, yes, for "userspace programs". I mean: for BPF
>>> programs
>>> requiring vmlinux.h (BPF_CFLAGS), I guess you will still need the
>>> bpf.h
>>> file from tools/include/uapi, no?
>>>
>>>> I haven't looked into the details. I quickly tried but it
>>>> fails in my environment.
>>>
>>> Do you not have issues because some files have something like:
>>>
>>>    #include <uapi/linux/(...).h>
>>>
>>> On my side, I had a working version using this patch:
>>>
>>>> diff --git a/tools/testing/selftests/bpf/Makefile
>>>> b/tools/testing/selftests/bpf/Makefile
>>>> index 7c5827d20c2e..112f14d40852 100644
>>>> --- a/tools/testing/selftests/bpf/Makefile
>>>> +++ b/tools/testing/selftests/bpf/Makefile
>>>> @@ -37,7 +37,7 @@ CFLAGS += -g $(OPT_FLAGS) -rdynamic            \
>>>>            -Wall -Werror -fno-omit-frame-pointer                  \
>>>>            $(GENFLAGS) $(SAN_CFLAGS) $(LIBELF_CFLAGS)             \
>>>>            -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)   \
>>>> -         -I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT)
>>>> +         -I$(TOOLSINCDIR) $(KHDR_INCLUDES) -I$(OUTPUT)
>>>>   LDFLAGS += $(SAN_LDFLAGS)
>>>>   LDLIBS += $(LIBELF_LIBS) -lz -lrt -lpthread
>>>>   
>>>
>>> But only after having removed these extra 'uapi/':
>>>
>>>    $ git grep -l '<uapi/' -- tools/testing/selftests/bpf | \
>>>      xargs sed -i 's|#include <uapi/|#include <|g'
>>>
>>> Is it not OK for you like that?

I tried and it works for me with the above changes. The other $(APIDIR) usages 
in the Makefile can be replaced also?

Matt, do you want to post a patch and see how does it go with the bpf CI?

[ Sorry for the late reply. ]

>>>
>>> Note that I built the selftests using KHDR_INCLUDES=-
>>> I$INSTALL_HDR_PATH.


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-07-24 19:47 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-03 18:57 [PATCH bpf-next v3 0/3] selftests/bpf: new MPTCP subflow subtest Matthieu Baerts (NGI0)
2024-07-03 18:57 ` [PATCH bpf-next v3 1/3] selftests/bpf: Add mptcp subflow example Matthieu Baerts (NGI0)
2024-07-03 18:57 ` [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link Matthieu Baerts (NGI0)
2024-07-04 10:48   ` Matthieu Baerts
2024-07-05 23:10     ` Martin KaFai Lau
2024-07-06  0:25       ` Matthieu Baerts
2024-07-24  7:42         ` Geliang Tang
2024-07-24  8:24           ` Matthieu Baerts
2024-07-24 19:47             ` Martin KaFai Lau
2024-07-03 18:57 ` [PATCH bpf-next v3 3/3] selftests/bpf: Add mptcp subflow subtest Matthieu Baerts (NGI0)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).