Re: [PATCH 04/11] sched_ext: Fix processs_ddsp_deferred_locals() by unifying DTL_INVALID handling

David Vernet
2024-09-01T15:35:52Z

[-- Attachment #1.1: Type: text/plain, Size: 950 bytes --]

On Sat, Aug 31, 2024 at 10:03:58PM -1000, Tejun Heo wrote:
> Hello,
>
> On Sat, Aug 31, 2024 at 07:56:39PM -0500, David Vernet wrote:
> ...
> > Sorry, should have been more clear: the testcase dispatched all tasks to
> > the wrong CPU, which is why it's a kworker in the print output below. I
> > believe that ksoftiqrd hit the same path as well and just wasn't printed
> > in the output because it lost the race to scx_bpf_error(). Let me know
> > if you want the testcase to repro and I can send it, or send a separate
> > patch to add it to selftests.
>
> Yeah, please share the repro.

See the attached patch. You can run the test as follows after rebuilding
selftests:

./runner -t ddsp_local_on_invalid

You may have to run the test a few times to see the repro. Worth noting
is that the repro doesn't seem to hit if we don't explicitly set the
fallback target to 0 in ddsp_local_on_invalid_enqueue().

Thanks,
David

[-- Attachment #1.2: 0001-scx-Add-test-validating-direct-dispatch-with-LOCAL_O.patch --]
[-- Type: text/x-patch, Size: 4635 bytes --]

From 5e1a850b7db989429b94ea5d9cdf786faf3bcd4f Mon Sep 17 00:00:00 2001
From: David Vernet <void@manifault.com>
Date: Sat, 31 Aug 2024 19:16:22 -0500
Subject: [PATCH] scx: Add test validating direct dispatch with LOCAL_ON

SCX_DSQ_LOCAL_ON | cpu can now be invoked on the direct dispatch path.
Let's ensure we're gracefully handling it being dispatched to a CPU
that it's not allowed to run on.

Signed-off-by: David Vernet <void@manifault.com>
---
tools/testing/selftests/sched_ext/Makefile | 1 +
.../sched_ext/ddsp_local_on_invalid.bpf.c | 42 ++++++++++++++
.../sched_ext/ddsp_local_on_invalid.c | 58 +++++++++++++++++++
3 files changed, 101 insertions(+)
create mode 100644 tools/testing/selftests/sched_ext/ddsp_local_on_invalid.bpf.c
create mode 100644 tools/testing/selftests/sched_ext/ddsp_local_on_invalid.c

diff --git a/tools/testing/selftests/sched_ext/Makefile b/tools/testing/selftests/sched_ext/Makefile
index 0754a2c110a1..4823a67e6854 100644
--- a/tools/testing/selftests/sched_ext/Makefile
+++ b/tools/testing/selftests/sched_ext/Makefile
@@ -165,6 +165,7 @@ auto-test-targets := \
enq_last_no_enq_fails \
enq_select_cpu_fails \
ddsp_bogus_dsq_fail \
+ ddsp_local_on_invalid \
ddsp_vtimelocal_fail \
dsp_local_on \
exit \
diff --git a/tools/testing/selftests/sched_ext/ddsp_local_on_invalid.bpf.c b/tools/testing/selftests/sched_ext/ddsp_local_on_invalid.bpf.c
new file mode 100644
index 000000000000..e4512d7cc4b5
--- /dev/null
+++ b/tools/testing/selftests/sched_ext/ddsp_local_on_invalid.bpf.c
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024 Meta Platforms, Inc. and affiliates.
+ * Copyright (c) 2024 David Vernet <dvernet@meta.com>
+ */
+#include <scx/common.bpf.h>
+
+char _license[] SEC("license") = "GPL";
+const volatile s32 nr_cpus;
+
+UEI_DEFINE(uei);
+
+s32 BPF_STRUCT_OPS(ddsp_local_on_invalid_select_cpu, struct task_struct *p,
+ s32 prev_cpu, u64 wake_flags)
+{
+ return prev_cpu;
+}
+
+void BPF_STRUCT_OPS(ddsp_local_on_invalid_enqueue, struct task_struct *p,
+ u64 enq_flags)
+{
+ int target = bpf_cpumask_first_zero(p->cpus_ptr);
+
+ if (target >= nr_cpus)
+ target = 0;
+
+ scx_bpf_dispatch(p, SCX_DSQ_LOCAL_ON | target, SCX_SLICE_DFL, enq_flags);
+}
+
+void BPF_STRUCT_OPS(ddsp_local_on_invalid_exit, struct scx_exit_info *ei)
+{
+ UEI_RECORD(uei, ei);
+}
+
+SEC(".struct_ops.link")
+struct sched_ext_ops ddsp_local_on_invalid_ops = {
+ .select_cpu = ddsp_local_on_invalid_select_cpu,
+ .enqueue = ddsp_local_on_invalid_enqueue,
+ .exit = ddsp_local_on_invalid_exit,
+ .name = "ddsp_local_on_invalid",
+ .timeout_ms = 2000U,
+};
diff --git a/tools/testing/selftests/sched_ext/ddsp_local_on_invalid.c b/tools/testing/selftests/sched_ext/ddsp_local_on_invalid.c
new file mode 100644
index 000000000000..7bc49df06ee0
--- /dev/null
+++ b/tools/testing/selftests/sched_ext/ddsp_local_on_invalid.c
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024 Meta Platforms, Inc. and affiliates.
+ * Copyright (c) 2024 David Vernet <dvernet@meta.com>
+ */
+#include <bpf/bpf.h>
+#include <scx/common.h>
+#include <unistd.h>
+#include "ddsp_local_on_invalid.bpf.skel.h"
+#include "scx_test.h"
+
+static enum scx_test_status setup(void **ctx)
+{
+ struct ddsp_local_on_invalid *skel;
+
+ skel = ddsp_local_on_invalid__open();
+ SCX_FAIL_IF(!skel, "Failed to open");
+
+ skel->rodata->nr_cpus = libbpf_num_possible_cpus();
+ SCX_FAIL_IF(ddsp_local_on_invalid__load(skel), "Failed to load skel");
+ *ctx = skel;
+
+ return SCX_TEST_PASS;
+}
+
+static enum scx_test_status run(void *ctx)
+{
+ struct ddsp_local_on_invalid *skel = ctx;
+ struct bpf_link *link;
+
+ link = bpf_map__attach_struct_ops(skel->maps.ddsp_local_on_invalid_ops);
+ SCX_FAIL_IF(!link, "Failed to attach struct_ops");
+
+ /* Just sleeping is fine, plenty of scheduling events happening */
+ sleep(1);
+
+ SCX_EQ(skel->data->uei.kind, EXIT_KIND(SCX_EXIT_ERROR));
+ bpf_link__destroy(link);
+
+ return SCX_TEST_PASS;
+}
+
+static void cleanup(void *ctx)
+{
+ struct ddsp_local_on_invalid *skel = ctx;
+
+ ddsp_local_on_invalid__destroy(skel);
+}
+
+struct scx_test ddsp_local_on_invalid = {
+ .name = "ddsp_local_on_invalid",
+ .description = "Verify we can gracefully handle direct dispatch "
+ "of tasks to an invalid local DSQ from osp.dispatch()",
+ .setup = setup,
+ .run = run,
+ .cleanup = cleanup,
+};
+REGISTER_SCX_TEST(&ddsp_local_on_invalid)
--
2.45.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]