public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 bpf-next 1/2] bpf: add bpf_task_get_cgroup kfunc
@ 2024-03-16 16:22 Jose Fernandez
  2024-03-16 16:22 ` [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup Jose Fernandez
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Jose Fernandez @ 2024-03-16 16:22 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa
  Cc: bpf, linux-kernel, linux-kselftest, Jose Fernandez,
	Tycho Andersen

This patch enhances the BPF helpers by adding a kfunc to retrieve the
cgroup v2 of a task, addressing a previous limitation where only
bpf_task_get_cgroup1 was available for cgroup v1. The new kfunc is
particularly useful for scenarios where obtaining the cgroup ID of a
task other than the "current" one is necessary, which the existing
bpf_get_current_cgroup_id helper cannot accommodate. A specific use
case at Netflix involved the sched_switch tracepoint, where we had to
get the cgroup IDs of both the prev and next tasks.

The bpf_task_get_cgroup kfunc acquires and returns a reference to a
task's default cgroup, ensuring thread-safe access by correctly
implementing RCU read locking and unlocking. It leverages the existing
cgroup.h helper, and cgroup_tryget to safely acquire a reference to it.

Signed-off-by: Jose Fernandez <josef@netflix.com>
Reviewed-by: Tycho Andersen <tycho@tycho.pizza>
---
V1 -> V2: Return a pointer to the cgroup instead of the cgroup ID

 kernel/bpf/helpers.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index a89587859571..bbd19d5eedb6 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2266,6 +2266,31 @@ bpf_task_get_cgroup1(struct task_struct *task, int hierarchy_id)
 		return NULL;
 	return cgrp;
 }
+
+/**
+ * bpf_task_get_cgroup - Acquire a reference to the default cgroup of a task.
+ * @task: The target task
+ *
+ * This function returns the task's default cgroup, primarily
+ * designed for use with cgroup v2. In cgroup v1, the concept of default
+ * cgroup varies by subsystem, and while this function will work with
+ * cgroup v1, it's recommended to use bpf_task_get_cgroup1 instead.
+ * A cgroup returned by this kfunc which is not subsequently stored in a
+ * map, must be released by calling bpf_cgroup_release().
+ *
+ * Return: On success, the cgroup is returned. On failure, NULL is returned.
+ */
+__bpf_kfunc struct cgroup *bpf_task_get_cgroup(struct task_struct *task)
+{
+	struct cgroup *cgrp;
+
+	rcu_read_lock();
+	cgrp = task_dfl_cgroup(task);
+	if (!cgroup_tryget(cgrp))
+		cgrp = NULL;
+	rcu_read_unlock();
+	return cgrp;
+}
 #endif /* CONFIG_CGROUPS */
 
 /**
@@ -2573,6 +2598,7 @@ BTF_ID_FLAGS(func, bpf_cgroup_ancestor, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_cgroup_from_id, KF_ACQUIRE | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_task_under_cgroup, KF_RCU)
 BTF_ID_FLAGS(func, bpf_task_get_cgroup1, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_task_get_cgroup, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
 #endif
 BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_throw)

base-commit: 4c8644f86c854c214aaabbcc24a27fa4c7e6a951
-- 
2.40.1


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

* [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup
  2024-03-16 16:22 [PATCH V2 bpf-next 1/2] bpf: add bpf_task_get_cgroup kfunc Jose Fernandez
@ 2024-03-16 16:22 ` Jose Fernandez
  2024-03-18  3:10   ` Ratheesh Kannoth
  2024-03-18  9:58   ` Jiri Olsa
  2024-03-18 15:54 ` [PATCH V2 bpf-next 1/2] bpf: add bpf_task_get_cgroup kfunc Yonghong Song
  2024-03-18 16:07 ` Stanislav Fomichev
  2 siblings, 2 replies; 8+ messages in thread
From: Jose Fernandez @ 2024-03-16 16:22 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa
  Cc: bpf, linux-kernel, linux-kselftest, Jose Fernandez,
	Tycho Andersen

This patch adds a selftest for the `bpf_task_get_cgroup` kfunc. The test
focuses on the use case of obtaining the cgroup ID of the previous task
in a `sched_switch` tracepoint.

The selftest involves creating a test cgroup, attaching a BPF program
that utilizes the `bpf_task_get_cgroup` during a `sched_switch`
tracepoint, and validating that the obtained cgroup ID for the previous
task matches the expected cgroup ID.

Signed-off-by: Jose Fernandez <josef@netflix.com>
Reviewed-by: Tycho Andersen <tycho@tycho.pizza>
---
V1 -> V2: Refactor test to work with a cgroup pointer instead of the ID

 .../bpf/prog_tests/task_get_cgroup.c          | 58 +++++++++++++++++++
 .../bpf/progs/test_task_get_cgroup.c          | 37 ++++++++++++
 2 files changed, 95 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_task_get_cgroup.c

diff --git a/tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c b/tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c
new file mode 100644
index 000000000000..67ed65d0c461
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2024 Netflix, Inc.
+
+#include <test_progs.h>
+#include <cgroup_helpers.h>
+#include "test_task_get_cgroup.skel.h"
+#include <unistd.h>
+
+#define TEST_CGROUP "/test-task-get-cgroup/"
+
+void test_task_get_cgroup(void)
+{
+	struct test_task_get_cgroup *skel;
+	int err, fd;
+	pid_t pid;
+	__u64 cgroup_id, expected_cgroup_id;
+	const struct timespec req = {
+		.tv_sec = 1,
+		.tv_nsec = 0,
+	};
+
+	fd = test__join_cgroup(TEST_CGROUP);
+	if (!ASSERT_OK(fd < 0, "test_join_cgroup_TEST_CGROUP"))
+		return;
+
+	skel = test_task_get_cgroup__open();
+	if (!ASSERT_OK_PTR(skel, "test_task_get_cgroup__open"))
+		goto cleanup;
+
+	err = test_task_get_cgroup__load(skel);
+	if (!ASSERT_OK(err, "test_task_get_cgroup__load"))
+		goto cleanup;
+
+	err = test_task_get_cgroup__attach(skel);
+	if (!ASSERT_OK(err, "test_task_get_cgroup__attach"))
+		goto cleanup;
+
+	pid = getpid();
+	expected_cgroup_id = get_cgroup_id(TEST_CGROUP);
+	if (!ASSERT_GT(expected_cgroup_id, 0, "get_cgroup_id"))
+		goto cleanup;
+
+	/* Trigger nanosleep to enter the sched_switch tracepoint */
+	/* The previous task should be this process */
+	syscall(__NR_nanosleep, &req, NULL);
+
+	err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.pid_to_cgid_map), &pid,
+				  &cgroup_id);
+
+	if (!ASSERT_OK(err, "bpf_map_lookup_elem"))
+		goto cleanup;
+
+	ASSERT_EQ(cgroup_id, expected_cgroup_id, "cgroup_id");
+
+cleanup:
+	test_task_get_cgroup__destroy(skel);
+	close(fd);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c b/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c
new file mode 100644
index 000000000000..580f8f0657d5
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2024 Netflix, Inc.
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+struct cgroup *bpf_task_get_cgroup(struct task_struct *task) __ksym;
+void bpf_cgroup_release(struct cgroup *cgrp) __ksym;
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__uint(max_entries, 4096);
+	__type(key, __u32);
+	__type(value, __u64);
+} pid_to_cgid_map SEC(".maps");
+
+SEC("tp_btf/sched_switch")
+int BPF_PROG(sched_switch, bool preempt, struct task_struct *prev,
+	     struct task_struct *next)
+{
+	struct cgroup *cgrp;
+	u64 cgroup_id;
+	u32 pid;
+
+	cgrp = bpf_task_get_cgroup(prev);
+	if (cgrp == NULL)
+		return 0;
+	cgroup_id = cgrp->kn->id;
+	pid = prev->pid;
+	bpf_map_update_elem(&pid_to_cgid_map, &pid, &cgroup_id, BPF_ANY);
+
+	bpf_cgroup_release(cgrp);
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.40.1


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

* Re: [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup
  2024-03-16 16:22 ` [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup Jose Fernandez
@ 2024-03-18  3:10   ` Ratheesh Kannoth
  2024-03-18  3:15     ` Alexei Starovoitov
  2024-03-18  9:58   ` Jiri Olsa
  1 sibling, 1 reply; 8+ messages in thread
From: Ratheesh Kannoth @ 2024-03-18  3:10 UTC (permalink / raw)
  To: Jose Fernandez
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
	bpf, linux-kernel, linux-kselftest, Tycho Andersen

On 2024-03-16 at 21:52:41, Jose Fernandez (josef@netflix.com) wrote:
> This patch adds a selftest for the `bpf_task_get_cgroup` kfunc. The test
> focuses on the use case of obtaining the cgroup ID of the previous task
> in a `sched_switch` tracepoint.
>
> The selftest involves creating a test cgroup, attaching a BPF program
> that utilizes the `bpf_task_get_cgroup` during a `sched_switch`
> tracepoint, and validating that the obtained cgroup ID for the previous
> task matches the expected cgroup ID.
>
> Signed-off-by: Jose Fernandez <josef@netflix.com>
> Reviewed-by: Tycho Andersen <tycho@tycho.pizza>
> ---
> V1 -> V2: Refactor test to work with a cgroup pointer instead of the ID
>
>  .../bpf/prog_tests/task_get_cgroup.c          | 58 +++++++++++++++++++
>  .../bpf/progs/test_task_get_cgroup.c          | 37 ++++++++++++
>  2 files changed, 95 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_task_get_cgroup.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c b/tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c
> new file mode 100644
> index 000000000000..67ed65d0c461
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c
> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2024 Netflix, Inc.
> +
> +#include <test_progs.h>
> +#include <cgroup_helpers.h>
> +#include "test_task_get_cgroup.skel.h"
> +#include <unistd.h>
> +
> +#define TEST_CGROUP "/test-task-get-cgroup/"
> +
> +void test_task_get_cgroup(void)
> +{
> +	struct test_task_get_cgroup *skel;
> +	int err, fd;
> +	pid_t pid;
> +	__u64 cgroup_id, expected_cgroup_id;
> +	const struct timespec req = {
> +		.tv_sec = 1,
> +		.tv_nsec = 0,
> +	};
Reverse Xmas tree.

> +
> +	fd = test__join_cgroup(TEST_CGROUP);
> +	if (!ASSERT_OK(fd < 0, "test_join_cgroup_TEST_CGROUP"))
> +		return;
> +
> +	skel = test_task_get_cgroup__open();
> +	if (!ASSERT_OK_PTR(skel, "test_task_get_cgroup__open"))
> +		goto cleanup;
> +
> +	err = test_task_get_cgroup__load(skel);
> +	if (!ASSERT_OK(err, "test_task_get_cgroup__load"))
> +		goto cleanup;
> +
> +	err = test_task_get_cgroup__attach(skel);
> +	if (!ASSERT_OK(err, "test_task_get_cgroup__attach"))
> +		goto cleanup;
> +
> +	pid = getpid();
> +	expected_cgroup_id = get_cgroup_id(TEST_CGROUP);
> +	if (!ASSERT_GT(expected_cgroup_id, 0, "get_cgroup_id"))
> +		goto cleanup;
> +
> +	/* Trigger nanosleep to enter the sched_switch tracepoint */
> +	/* The previous task should be this process */
> +	syscall(__NR_nanosleep, &req, NULL);
> +
> +	err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.pid_to_cgid_map), &pid,
> +				  &cgroup_id);
> +
> +	if (!ASSERT_OK(err, "bpf_map_lookup_elem"))
> +		goto cleanup;
> +
> +	ASSERT_EQ(cgroup_id, expected_cgroup_id, "cgroup_id");
> +
> +cleanup:
> +	test_task_get_cgroup__destroy(skel);
> +	close(fd);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c b/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c
> new file mode 100644
> index 000000000000..580f8f0657d5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c
> @@ -0,0 +1,37 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2024 Netflix, Inc.
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +struct cgroup *bpf_task_get_cgroup(struct task_struct *task) __ksym;
> +void bpf_cgroup_release(struct cgroup *cgrp) __ksym;
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_HASH);
> +	__uint(max_entries, 4096);
> +	__type(key, __u32);
> +	__type(value, __u64);
> +} pid_to_cgid_map SEC(".maps");
> +
> +SEC("tp_btf/sched_switch")
> +int BPF_PROG(sched_switch, bool preempt, struct task_struct *prev,
> +	     struct task_struct *next)
> +{
> +	struct cgroup *cgrp;
> +	u64 cgroup_id;
> +	u32 pid;
> +
> +	cgrp = bpf_task_get_cgroup(prev);
> +	if (cgrp == NULL)
> +		return 0;
> +	cgroup_id = cgrp->kn->id;
> +	pid = prev->pid;
> +	bpf_map_update_elem(&pid_to_cgid_map, &pid, &cgroup_id, BPF_ANY);
> +
> +	bpf_cgroup_release(cgrp);
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.40.1
>

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

* Re: [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup
  2024-03-18  3:10   ` Ratheesh Kannoth
@ 2024-03-18  3:15     ` Alexei Starovoitov
  0 siblings, 0 replies; 8+ messages in thread
From: Alexei Starovoitov @ 2024-03-18  3:15 UTC (permalink / raw)
  To: Ratheesh Kannoth
  Cc: Jose Fernandez, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo, Jiri Olsa, bpf, LKML,
	open list:KERNEL SELFTEST FRAMEWORK, Tycho Andersen

On Sun, Mar 17, 2024 at 8:10 PM Ratheesh Kannoth <rkannoth@marvell.com> wrote:
>
> On 2024-03-16 at 21:52:41, Jose Fernandez (josef@netflix.com) wrote:
> > This patch adds a selftest for the `bpf_task_get_cgroup` kfunc. The test
> > focuses on the use case of obtaining the cgroup ID of the previous task
> > in a `sched_switch` tracepoint.
> >
> > The selftest involves creating a test cgroup, attaching a BPF program
> > that utilizes the `bpf_task_get_cgroup` during a `sched_switch`
> > tracepoint, and validating that the obtained cgroup ID for the previous
> > task matches the expected cgroup ID.
> >
> > Signed-off-by: Jose Fernandez <josef@netflix.com>
> > Reviewed-by: Tycho Andersen <tycho@tycho.pizza>
> > ---
> > V1 -> V2: Refactor test to work with a cgroup pointer instead of the ID
> >
> >  .../bpf/prog_tests/task_get_cgroup.c          | 58 +++++++++++++++++++
> >  .../bpf/progs/test_task_get_cgroup.c          | 37 ++++++++++++
> >  2 files changed, 95 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_task_get_cgroup.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c b/tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c
> > new file mode 100644
> > index 000000000000..67ed65d0c461
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/task_get_cgroup.c
> > @@ -0,0 +1,58 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright 2024 Netflix, Inc.
> > +
> > +#include <test_progs.h>
> > +#include <cgroup_helpers.h>
> > +#include "test_task_get_cgroup.skel.h"
> > +#include <unistd.h>
> > +
> > +#define TEST_CGROUP "/test-task-get-cgroup/"
> > +
> > +void test_task_get_cgroup(void)
> > +{
> > +     struct test_task_get_cgroup *skel;
> > +     int err, fd;
> > +     pid_t pid;
> > +     __u64 cgroup_id, expected_cgroup_id;
> > +     const struct timespec req = {
> > +             .tv_sec = 1,
> > +             .tv_nsec = 0,
> > +     };
> Reverse Xmas tree.

NO. We don't do it in bpf trees.

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

* Re: [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup
  2024-03-16 16:22 ` [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup Jose Fernandez
  2024-03-18  3:10   ` Ratheesh Kannoth
@ 2024-03-18  9:58   ` Jiri Olsa
  2024-03-19  0:30     ` Jose Fernandez
  1 sibling, 1 reply; 8+ messages in thread
From: Jiri Olsa @ 2024-03-18  9:58 UTC (permalink / raw)
  To: Jose Fernandez
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf,
	linux-kernel, linux-kselftest, Tycho Andersen

On Sat, Mar 16, 2024 at 10:22:41AM -0600, Jose Fernandez wrote:

SNIP

> +void test_task_get_cgroup(void)
> +{
> +	struct test_task_get_cgroup *skel;
> +	int err, fd;
> +	pid_t pid;
> +	__u64 cgroup_id, expected_cgroup_id;
> +	const struct timespec req = {
> +		.tv_sec = 1,
> +		.tv_nsec = 0,
> +	};
> +
> +	fd = test__join_cgroup(TEST_CGROUP);
> +	if (!ASSERT_OK(fd < 0, "test_join_cgroup_TEST_CGROUP"))
> +		return;
> +
> +	skel = test_task_get_cgroup__open();
> +	if (!ASSERT_OK_PTR(skel, "test_task_get_cgroup__open"))
> +		goto cleanup;
> +
> +	err = test_task_get_cgroup__load(skel);
> +	if (!ASSERT_OK(err, "test_task_get_cgroup__load"))
> +		goto cleanup;

nit, you could call test_task_get_cgroup__open_and_load

> +
> +	err = test_task_get_cgroup__attach(skel);
> +	if (!ASSERT_OK(err, "test_task_get_cgroup__attach"))
> +		goto cleanup;
> +
> +	pid = getpid();
> +	expected_cgroup_id = get_cgroup_id(TEST_CGROUP);
> +	if (!ASSERT_GT(expected_cgroup_id, 0, "get_cgroup_id"))
> +		goto cleanup;
> +
> +	/* Trigger nanosleep to enter the sched_switch tracepoint */
> +	/* The previous task should be this process */
> +	syscall(__NR_nanosleep, &req, NULL);

would smaller sleep do? also we have our own usleep (in test_progs.c)
that calls nanosleep

> +
> +	err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.pid_to_cgid_map), &pid,
> +				  &cgroup_id);
> +
> +	if (!ASSERT_OK(err, "bpf_map_lookup_elem"))
> +		goto cleanup;
> +
> +	ASSERT_EQ(cgroup_id, expected_cgroup_id, "cgroup_id");
> +
> +cleanup:
> +	test_task_get_cgroup__destroy(skel);
> +	close(fd);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c b/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c
> new file mode 100644
> index 000000000000..580f8f0657d5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c
> @@ -0,0 +1,37 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2024 Netflix, Inc.
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +struct cgroup *bpf_task_get_cgroup(struct task_struct *task) __ksym;
> +void bpf_cgroup_release(struct cgroup *cgrp) __ksym;
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_HASH);
> +	__uint(max_entries, 4096);
> +	__type(key, __u32);
> +	__type(value, __u64);
> +} pid_to_cgid_map SEC(".maps");
> +
> +SEC("tp_btf/sched_switch")
> +int BPF_PROG(sched_switch, bool preempt, struct task_struct *prev,
> +	     struct task_struct *next)
> +{
> +	struct cgroup *cgrp;
> +	u64 cgroup_id;
> +	u32 pid;
> +

could you filter for your pid in here like we do in other places,
(eg in progs/kprobe_multi.c)

in which case you won't need hash map, but just a single value
to store the cgroup id to

jirka

> +	cgrp = bpf_task_get_cgroup(prev);
> +	if (cgrp == NULL)
> +		return 0;
> +	cgroup_id = cgrp->kn->id;
> +	pid = prev->pid;
> +	bpf_map_update_elem(&pid_to_cgid_map, &pid, &cgroup_id, BPF_ANY);
> +
> +	bpf_cgroup_release(cgrp);
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> -- 
> 2.40.1
> 

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

* Re: [PATCH V2 bpf-next 1/2] bpf: add bpf_task_get_cgroup kfunc
  2024-03-16 16:22 [PATCH V2 bpf-next 1/2] bpf: add bpf_task_get_cgroup kfunc Jose Fernandez
  2024-03-16 16:22 ` [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup Jose Fernandez
@ 2024-03-18 15:54 ` Yonghong Song
  2024-03-18 16:07 ` Stanislav Fomichev
  2 siblings, 0 replies; 8+ messages in thread
From: Yonghong Song @ 2024-03-18 15:54 UTC (permalink / raw)
  To: Jose Fernandez, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa
  Cc: bpf, linux-kernel, linux-kselftest, Tycho Andersen


On 3/16/24 9:22 AM, Jose Fernandez wrote:
> This patch enhances the BPF helpers by adding a kfunc to retrieve the
> cgroup v2 of a task, addressing a previous limitation where only
> bpf_task_get_cgroup1 was available for cgroup v1. The new kfunc is
> particularly useful for scenarios where obtaining the cgroup ID of a
> task other than the "current" one is necessary, which the existing
> bpf_get_current_cgroup_id helper cannot accommodate. A specific use
> case at Netflix involved the sched_switch tracepoint, where we had to
> get the cgroup IDs of both the prev and next tasks.
>
> The bpf_task_get_cgroup kfunc acquires and returns a reference to a
> task's default cgroup, ensuring thread-safe access by correctly
> implementing RCU read locking and unlocking. It leverages the existing
> cgroup.h helper, and cgroup_tryget to safely acquire a reference to it.
>
> Signed-off-by: Jose Fernandez <josef@netflix.com>
> Reviewed-by: Tycho Andersen <tycho@tycho.pizza>

Acked-by: Yonghong Song <yonghong.song@linux.dev>


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

* Re: [PATCH V2 bpf-next 1/2] bpf: add bpf_task_get_cgroup kfunc
  2024-03-16 16:22 [PATCH V2 bpf-next 1/2] bpf: add bpf_task_get_cgroup kfunc Jose Fernandez
  2024-03-16 16:22 ` [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup Jose Fernandez
  2024-03-18 15:54 ` [PATCH V2 bpf-next 1/2] bpf: add bpf_task_get_cgroup kfunc Yonghong Song
@ 2024-03-18 16:07 ` Stanislav Fomichev
  2 siblings, 0 replies; 8+ messages in thread
From: Stanislav Fomichev @ 2024-03-18 16:07 UTC (permalink / raw)
  To: Jose Fernandez
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel,
	linux-kselftest, Tycho Andersen

On 03/16, Jose Fernandez wrote:
> This patch enhances the BPF helpers by adding a kfunc to retrieve the
> cgroup v2 of a task, addressing a previous limitation where only
> bpf_task_get_cgroup1 was available for cgroup v1. The new kfunc is
> particularly useful for scenarios where obtaining the cgroup ID of a
> task other than the "current" one is necessary, which the existing
> bpf_get_current_cgroup_id helper cannot accommodate. A specific use
> case at Netflix involved the sched_switch tracepoint, where we had to
> get the cgroup IDs of both the prev and next tasks.
> 
> The bpf_task_get_cgroup kfunc acquires and returns a reference to a
> task's default cgroup, ensuring thread-safe access by correctly
> implementing RCU read locking and unlocking. It leverages the existing
> cgroup.h helper, and cgroup_tryget to safely acquire a reference to it.
> 
> Signed-off-by: Jose Fernandez <josef@netflix.com>
> Reviewed-by: Tycho Andersen <tycho@tycho.pizza>

Acked-by: Stanislav Fomichev <sdf@google.com>

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

* Re: [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup
  2024-03-18  9:58   ` Jiri Olsa
@ 2024-03-19  0:30     ` Jose Fernandez
  0 siblings, 0 replies; 8+ messages in thread
From: Jose Fernandez @ 2024-03-19  0:30 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf,
	linux-kernel, linux-kselftest, Tycho Andersen

On 24/03/18 10:58AM, Jiri Olsa wrote:
> On Sat, Mar 16, 2024 at 10:22:41AM -0600, Jose Fernandez wrote:
> 
> SNIP
> 
> > +void test_task_get_cgroup(void)
> > +{
> > +	struct test_task_get_cgroup *skel;
> > +	int err, fd;
> > +	pid_t pid;
> > +	__u64 cgroup_id, expected_cgroup_id;
> > +	const struct timespec req = {
> > +		.tv_sec = 1,
> > +		.tv_nsec = 0,
> > +	};
> > +
> > +	fd = test__join_cgroup(TEST_CGROUP);
> > +	if (!ASSERT_OK(fd < 0, "test_join_cgroup_TEST_CGROUP"))
> > +		return;
> > +
> > +	skel = test_task_get_cgroup__open();
> > +	if (!ASSERT_OK_PTR(skel, "test_task_get_cgroup__open"))
> > +		goto cleanup;
> > +
> > +	err = test_task_get_cgroup__load(skel);
> > +	if (!ASSERT_OK(err, "test_task_get_cgroup__load"))
> > +		goto cleanup;
> 
> nit, you could call test_task_get_cgroup__open_and_load

I'll rename.

> > +
> > +	err = test_task_get_cgroup__attach(skel);
> > +	if (!ASSERT_OK(err, "test_task_get_cgroup__attach"))
> > +		goto cleanup;
> > +
> > +	pid = getpid();
> > +	expected_cgroup_id = get_cgroup_id(TEST_CGROUP);
> > +	if (!ASSERT_GT(expected_cgroup_id, 0, "get_cgroup_id"))
> > +		goto cleanup;
> > +
> > +	/* Trigger nanosleep to enter the sched_switch tracepoint */
> > +	/* The previous task should be this process */
> > +	syscall(__NR_nanosleep, &req, NULL);
> 
> would smaller sleep do? also we have our own usleep (in test_progs.c)
> that calls nanosleep

Yes a smaller sleep should be fine.
I'll reduce the sleep and use the usleep helper.

> > +
> > +	err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.pid_to_cgid_map), &pid,
> > +				  &cgroup_id);
> > +
> > +	if (!ASSERT_OK(err, "bpf_map_lookup_elem"))
> > +		goto cleanup;
> > +
> > +	ASSERT_EQ(cgroup_id, expected_cgroup_id, "cgroup_id");
> > +
> > +cleanup:
> > +	test_task_get_cgroup__destroy(skel);
> > +	close(fd);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c b/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c
> > new file mode 100644
> > index 000000000000..580f8f0657d5
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_task_get_cgroup.c
> > @@ -0,0 +1,37 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright 2024 Netflix, Inc.
> > +
> > +#include "vmlinux.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +struct cgroup *bpf_task_get_cgroup(struct task_struct *task) __ksym;
> > +void bpf_cgroup_release(struct cgroup *cgrp) __ksym;
> > +
> > +struct {
> > +	__uint(type, BPF_MAP_TYPE_HASH);
> > +	__uint(max_entries, 4096);
> > +	__type(key, __u32);
> > +	__type(value, __u64);
> > +} pid_to_cgid_map SEC(".maps");
> > +
> > +SEC("tp_btf/sched_switch")
> > +int BPF_PROG(sched_switch, bool preempt, struct task_struct *prev,
> > +	     struct task_struct *next)
> > +{
> > +	struct cgroup *cgrp;
> > +	u64 cgroup_id;
> > +	u32 pid;
> > +
> 
> could you filter for your pid in here like we do in other places,
> (eg in progs/kprobe_multi.c)
> 
> in which case you won't need hash map, but just a single value
> to store the cgroup id to
> 
> jirka

I'll apply this suggestion as well and include it in V3. 
Thanks for the feedback.

> 
> > +	cgrp = bpf_task_get_cgroup(prev);
> > +	if (cgrp == NULL)
> > +		return 0;
> > +	cgroup_id = cgrp->kn->id;
> > +	pid = prev->pid;
> > +	bpf_map_update_elem(&pid_to_cgid_map, &pid, &cgroup_id, BPF_ANY);
> > +
> > +	bpf_cgroup_release(cgrp);
> > +	return 0;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
> > -- 
> > 2.40.1
> > 

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

end of thread, other threads:[~2024-03-19  0:30 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-16 16:22 [PATCH V2 bpf-next 1/2] bpf: add bpf_task_get_cgroup kfunc Jose Fernandez
2024-03-16 16:22 ` [PATCH V2 bpf-next 2/2] selftests/bpf: add selftest for bpf_task_get_cgroup Jose Fernandez
2024-03-18  3:10   ` Ratheesh Kannoth
2024-03-18  3:15     ` Alexei Starovoitov
2024-03-18  9:58   ` Jiri Olsa
2024-03-19  0:30     ` Jose Fernandez
2024-03-18 15:54 ` [PATCH V2 bpf-next 1/2] bpf: add bpf_task_get_cgroup kfunc Yonghong Song
2024-03-18 16:07 ` Stanislav Fomichev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox