Re: [PATCH] riscv: selftests: Add hwprobe binaries to .gitignore
On 4/26/24 12:58 AM, Charlie Jenkins wrote:
> The cbo and which-cpu hwprobe selftests leave their artifacts in the
> kernel tree and end up being tracked by git. Add the binaries to the
> hwprobe selftest .gitignore so this no longer happens.
I've been posted comments on patches adding new tests to please put
artifacts in the .gitignore. But still people forget or don't know about it
as .gitignore is specific to selftests and they aren't used-to it.
>
> Signed-off-by: Charlie Jenkins
> Fixes: a29e2a48afe3 ("RISC-V: selftests: Add CBO tests")
> Fixes: ef7d6abb2cf5 ("RISC-V: selftests: Add which-cpus hwprobe test")
Reviewed-by: Muhammad Usama Anjum
> ---
> tools/testing/selftests/riscv/hwprobe/.gitignore | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/testing/selftests/riscv/hwprobe/.gitignore
> b/tools/testing/selftests/riscv/hwprobe/.gitignore
> index 8113dc3bdd03..6e384e80ea1a 100644
> --- a/tools/testing/selftests/riscv/hwprobe/.gitignore
> +++ b/tools/testing/selftests/riscv/hwprobe/.gitignore
> @@ -1 +1,3 @@
> hwprobe
> +cbo
> +which-cpus
>
> ---
> base-commit: ed30a4a51bb196781c8058073ea720133a65596f
> change-id: 20240425-gitignore_hwprobe_artifacts-fb0f2cd3509c
--
BR,
Muhammad Usama Anjum
Re: [PATCH bpf-next v3 06/11] bpf: Fix compare error in function retval_range_within
On Thu, Apr 11, 2024 at 5:24 AM Xu Kuohai wrote:
>
> From: Xu Kuohai
>
> After checking lsm hook return range in verifier, the test case
> "test_progs -t test_lsm" failed, and the failure log says:
>
> libbpf: prog 'test_int_hook': BPF program load failed: Invalid argument
> libbpf: prog 'test_int_hook': -- BEGIN PROG LOAD LOG --
> 0: R1=ctx() R10=fp0
> ; int BPF_PROG(test_int_hook, struct vm_area_struct *vma, @ lsm.c:89
> 0: (79) r0 = *(u64 *)(r1 +24) ;
> R0_w=scalar(smin=smin32=-4095,smax=smax32=0) R1=ctx()
>
> [...]
>
> 24: (b4) w0 = -1 ; R0_w=0x
> ; int BPF_PROG(test_int_hook, struct vm_area_struct *vma, @ lsm.c:89
> 25: (95) exit
> At program exit the register R0 has smin=4294967295 smax=4294967295 should
> have been in [-4095, 0]
>
> It can be seen that instruction "w0 = -1" zero extended -1 to 64-bit
> register r0, setting both smin and smax values of r0 to 4294967295.
> This resulted in a false reject when r0 was checked with range [-4095, 0].
>
> Given bpf_retval_range is a 32-bit range, this patch fixes it by
> changing the compare between r0 and return range from 64-bit
> operation to 32-bit operation.
>
> Fixes: 8fa4ecd49b81 ("bpf: enforce exact retval range on subprog/callback
> exit")
> Signed-off-by: Xu Kuohai
> ---
> kernel/bpf/verifier.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 05c7c5f2bec0..5393d576c76f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9879,7 +9879,7 @@ static bool in_rbtree_lock_required_cb(struct
> bpf_verifier_env *env)
>
> static bool retval_range_within(struct bpf_retval_range range, const struct
> bpf_reg_state *reg)
> {
> - return range.minval <= reg->smin_value && reg->smax_value <=
> range.maxval;
> + return range.minval <= reg->s32_min_value && reg->s32_max_value <=
> range.maxval;
are all BPF programs treated as if they return int instead of long? If
not, we probably should have a bool flag in bpf_retval_range whether
comparison should be 32-bit or 64-bit?
> }
>
> static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
> --
> 2.30.2
>
Re: [PATCH] KVM: selftests: Avoid assuming "sudo" exists
On Mon, Apr 15, 2024, Brendan Jackman wrote:
> I ran into a failure running this test on a minimal rootfs.
>
> Can be fixed by just skipping the "sudo" in case we are already root.
>
> Signed-off-by: Brendan Jackman
> ---
> tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> index 7cbb409801eea..0e56822e8e0bf 100755
> --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> @@ -13,10 +13,21 @@ NX_HUGE_PAGES_RECOVERY_RATIO=$(cat
> /sys/module/kvm/parameters/nx_huge_pages_reco
> NX_HUGE_PAGES_RECOVERY_PERIOD=$(cat
> /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms)
> HUGE_PAGES=$(cat /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages)
>
> +# If we're already root, the host might not have sudo.
> +if [ $(whoami) == "root" ]; then
> + function maybe_sudo () {
Any objection to do_sudo instead of maybe_sudo? I can fixup when applying if
that works for you.
> + "$@"
> + }
> +else
> + function maybe_sudo () {
> + sudo "$@"
> + }
> +fi
> +
> set +e
>
> function sudo_echo () {
> - echo "$1" | sudo tee -a "$2" > /dev/null
> + echo "$1" | maybe_sudo tee -a "$2" > /dev/null
> }
>
> NXECUTABLE="$(dirname $0)/nx_huge_pages_test"
>
> ---
> base-commit: 2c71fdf02a95b3dd425b42f28fd47fb2b1d22702
> change-id: 20240415-kvm-selftests-no-sudo-1a55f831f882
>
> Best regards,
> --
> Brendan Jackman
>
Re: [PATCH 2/3] bpf: do not walk twice the hash map on free
Hi Benjamin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 52578f7f53ff8fe3a8f6f3bc8b5956615c07a16e]
url:
https://github.com/intel-lab-lkp/linux/commits/Benjamin-Tissoires/bpf-do-not-walk-twice-the-map-on-free/20240425-220322
base: 52578f7f53ff8fe3a8f6f3bc8b5956615c07a16e
patch link:
https://lore.kernel.org/r/20240425-bpf-next-v1-2-1d8330e6c643%40kernel.org
patch subject: [PATCH 2/3] bpf: do not walk twice the hash map on free
config: arc-randconfig-002-20240426
(https://download.01.org/0day-ci/archive/20240426/202404260653.ulrcgrp2-...@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20240426/202404260653.ulrcgrp2-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202404260653.ulrcgrp2-...@intel.com/
All warnings (new ones prefixed by >>):
>> kernel/bpf/hashtab.c:243:13: warning: 'htab_free_prealloced_wq' defined but
>> not used [-Wunused-function]
243 | static void htab_free_prealloced_wq(struct bpf_htab *htab)
| ^~~
vim +/htab_free_prealloced_wq +243 kernel/bpf/hashtab.c
68134668c17f31 Alexei Starovoitov 2021-07-14 242
246331e3f1eac9 Benjamin Tissoires 2024-04-20 @243 static void
htab_free_prealloced_wq(struct bpf_htab *htab)
246331e3f1eac9 Benjamin Tissoires 2024-04-20 244 {
246331e3f1eac9 Benjamin Tissoires 2024-04-20 245 u32 num_entries =
htab->map.max_entries;
246331e3f1eac9 Benjamin Tissoires 2024-04-20 246 int i;
246331e3f1eac9 Benjamin Tissoires 2024-04-20 247
246331e3f1eac9 Benjamin Tissoires 2024-04-20 248 if
(!btf_record_has_field(htab->map.record, BPF_WORKQUEUE))
246331e3f1eac9 Benjamin Tissoires 2024-04-20 249 return;
246331e3f1eac9 Benjamin Tissoires 2024-04-20 250 if
(htab_has_extra_elems(htab))
246331e3f1eac9 Benjamin Tissoires 2024-04-20 251 num_entries +=
num_possible_cpus();
246331e3f1eac9 Benjamin Tissoires 2024-04-20 252
246331e3f1eac9 Benjamin Tissoires 2024-04-20 253 for (i = 0; i <
num_entries; i++) {
246331e3f1eac9 Benjamin Tissoires 2024-04-20 254 struct
htab_elem *elem;
246331e3f1eac9 Benjamin Tissoires 2024-04-20 255
246331e3f1eac9 Benjamin Tissoires 2024-04-20 256 elem =
get_htab_elem(htab, i);
246331e3f1eac9 Benjamin Tissoires 2024-04-20 257
bpf_obj_free_workqueue(htab->map.record,
246331e3f1eac9 Benjamin Tissoires 2024-04-20 258
elem->key + round_up(htab->map.key_size, 8));
246331e3f1eac9 Benjamin Tissoires 2024-04-20 259 cond_resched();
246331e3f1eac9 Benjamin Tissoires 2024-04-20 260 }
246331e3f1eac9 Benjamin Tissoires 2024-04-20 261 }
246331e3f1eac9 Benjamin Tissoires 2024-04-20 262
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v4 0/9] Support Zve32[xf] and Zve64[xfd] Vector subextensions
Hello: This series was applied to riscv/linux.git (fixes) by Palmer Dabbelt : On Fri, 12 Apr 2024 14:48:56 +0800 you wrote: > The series composes of two parts. The first part provides a quick fix for > the issue on a recent thread[1]. The issue happens when a platform has > ununified vector register length across multiple cores. Specifically, > patch 1 adds a comment at a callsite of riscv_setup_vsize to clarify how > vlenb is observed by the system. Patch 2 fixes the issue by failing the > boot process of a secondary core if vlenb mismatches. > > [...] Here is the summary with links: - [v4,1/9] riscv: vector: add a comment when calling riscv_setup_vsize() (no matching commit) - [v4,2/9] riscv: smp: fail booting up smp if inconsistent vlen is detected (no matching commit) - [v4,3/9] riscv: cpufeature: call match_isa_ext() for single-letter extensions (no matching commit) - [v4,4/9] riscv: cpufeature: add zve32[xf] and zve64[xfd] isa detection (no matching commit) - [v4,5/9] dt-bindings: riscv: add Zve32[xf] Zve64[xfd] ISA extension description (no matching commit) - [v4,6/9] riscv: hwprobe: add zve Vector subextensions into hwprobe interface (no matching commit) - [v4,7/9] riscv: vector: adjust minimum Vector requirement to ZVE32X (no matching commit) - [v4,8/9] hwprobe: fix integer promotion in RISCV_HWPROBE_EXT macro https://git.kernel.org/riscv/c/5ea6764d9095 - [v4,9/9] selftest: run vector prctl test for ZVE32X (no matching commit) You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
[PATCH net-next v2 3/3] selftests: drv-net: validate the environment
Throw a slightly more helpful exception when env variables
are partially populated. Prior to this change we'd get
a dictionary key exception somewhere later on.
Reviewed-by: Willem de Bruijn
Signed-off-by: Jakub Kicinski
---
v2:
- check "both or none" for addresses
---
.../selftests/drivers/net/lib/py/env.py | 25 +++
1 file changed, 25 insertions(+)
diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py
b/tools/testing/selftests/drivers/net/lib/py/env.py
index 6f57bd5c0ed7..e2ab637e56dc 100644
--- a/tools/testing/selftests/drivers/net/lib/py/env.py
+++ b/tools/testing/selftests/drivers/net/lib/py/env.py
@@ -88,6 +88,7 @@ from .remote import Remote
self._ns_peer = None
if "NETIF" in self.env:
+self._check_env()
self.dev = ip("link show dev " + self.env['NETIF'], json=True)[0]
self.v4 = self.env.get("LOCAL_V4")
@@ -143,6 +144,30 @@ from .remote import Remote
ip(f"-6 addr add dev {self._ns_peer.nsims[0].ifname}
{self.nsim_v6_pfx}2/64 nodad", ns=self._netns)
ip(f" link set dev {self._ns_peer.nsims[0].ifname} up",
ns=self._netns)
+def _check_env(self):
+vars_needed = [
+["LOCAL_V4", "LOCAL_V6"],
+["REMOTE_V4", "REMOTE_V6"],
+["REMOTE_TYPE"],
+["REMOTE_ARGS"]
+]
+missing = []
+
+for choice in vars_needed:
+for entry in choice:
+if entry in self.env:
+break
+else:
+missing.append(choice)
+# Make sure v4 / v6 configs are symmetric
+if ("LOCAL_V6" in self.env) != ("REMOTE_V6" in self.env):
+missing.append(["LOCAL_V6", "REMOTE_V6"])
+if ("LOCAL_V4" in self.env) != ("REMOTE_V4" in self.env):
+missing.append(["LOCAL_V4", "REMOTE_V4"])
+if missing:
+raise Exception("Invalid environment, missing configuration:",
missing,
+"Please see
tools/testing/selftests/drivers/net/README.rst")
+
def __enter__(self):
return self
--
2.44.0
[PATCH net-next v2 1/3] selftests: drv-net: extend the README with more info and example
Add more info to the README. It's also now copied to GitHub for increased visibility: https://github.com/linux-netdev/nipa/wiki/Running-driver-tests Reviewed-by: Willem de Bruijn Signed-off-by: Jakub Kicinski --- .../testing/selftests/drivers/net/README.rst | 97 --- 1 file changed, 85 insertions(+), 12 deletions(-) diff --git a/tools/testing/selftests/drivers/net/README.rst b/tools/testing/selftests/drivers/net/README.rst index 0cbab33dad1f..3b6a29e6564b 100644 --- a/tools/testing/selftests/drivers/net/README.rst +++ b/tools/testing/selftests/drivers/net/README.rst @@ -1,18 +1,42 @@ -Running tests -= +.. SPDX-License-Identifier: GPL-2.0 -Tests are executed within kselftest framework like any other tests. -By default tests execute against software drivers such as netdevsim. -All tests must support running against a real device (SW-only tests -should instead be placed in net/ or drivers/net/netdevsim, HW-only -tests in drivers/net/hw). +Running driver tests + -Set appropriate variables to point the tests at a real device. +Networking driver tests are executed within kselftest framework like any +other tests. They support testing both real device drivers and emulated / +software drivers (latter mostly to test the core parts of the stack). + +SW mode +~~~ + +By default, when no extra parameters are set or exported, tests execute +against software drivers such as netdevsim. No extra preparation is required +the software devices are created and destroyed as part of the test. +In this mode the tests are indistinguishable from other selftests and +(for example) can be run under ``virtme-ng`` like the core networking selftests. + +HW mode +~~~ + +Executing tests against a real device requires external preparation. +The netdevice against which tests will be run must exist, be running +(in UP state) and be configured with an IP address. + +Refer to list of :ref:`Variables` later in this file to set up running +the tests against a real device. + +Both modes required +~~~ + +All tests in drivers/net must support running both against a software device +and a real device. SW-only tests should instead be placed in net/ or +drivers/net/netdevsim, HW-only tests in drivers/net/hw. Variables = -Variables can be set in the environment or by creating a net.config +The variables can be set in the environment or by creating a net.config file in the same directory as this README file. Example:: $ NETIF=eth0 ./some_test.sh @@ -23,9 +47,9 @@ Variables can be set in the environment or by creating a net.config # Variable set in a file NETIF=eth0 -Please note that the config parser is very simple, if there are -any non-alphanumeric characters in the value it needs to be in -double quotes. +Local test (which don't require endpoint for sending / receiving traffic) +need only the ``NETIF`` variable. Remaining variables define the endpoint +and communication method. NETIF ~ @@ -61,3 +85,52 @@ Arguments used to construct the communication channel. for netns - name of the "remote" namespace for ssh - name/address of the remote host + +Example +=== + +Build the selftests:: + + # make -C tools/testing/selftests/ TARGETS="drivers/net drivers/net/hw" + +"Install" the tests and copy them over to the target machine:: + + # make -C tools/testing/selftests/ TARGETS="drivers/net drivers/net/hw" \ + install INSTALL_PATH=/tmp/ksft-net-drv + + # rsync -ra --delete /tmp/ksft-net-drv root@192.168.1.1:/root/ + +On the target machine, running the tests will use netdevsim by default:: + + [/root] # ./ksft-net-drv/run_kselftest.sh -t drivers/net:ping.py + TAP version 13 + 1..1 + # timeout set to 45 + # selftests: drivers/net: ping.py + # KTAP version 1 + # 1..3 + # ok 1 ping.test_v4 + # ok 2 ping.test_v6 + # ok 3 ping.test_tcp + # # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0 + ok 1 selftests: drivers/net: ping.py + +Create a config with remote info:: + + [/root] # cat > ./ksft-net-drv/drivers/net/net.config <
[PATCH net-next v2 2/3] selftests: drv-net: reimplement the config parser
The shell lexer is not helping much, do very basic parsing
manually.
Reviewed-by: Willem de Bruijn
Signed-off-by: Jakub Kicinski
---
v2:
- use split() to split the line on =
---
.../selftests/drivers/net/lib/py/env.py | 26 ++-
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py
b/tools/testing/selftests/drivers/net/lib/py/env.py
index a3db1bb1afeb..6f57bd5c0ed7 100644
--- a/tools/testing/selftests/drivers/net/lib/py/env.py
+++ b/tools/testing/selftests/drivers/net/lib/py/env.py
@@ -1,7 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
import os
-import shlex
from pathlib import Path
from lib.py import KsftSkipEx
from lib.py import cmd, ip
@@ -16,17 +15,20 @@ from .remote import Remote
if not (src_dir / "net.config").exists():
return env
-lexer = shlex.shlex(open((src_dir / "net.config").as_posix(), 'r').read())
-k = None
-for token in lexer:
-if k is None:
-k = token
-env[k] = ""
-elif token == "=":
-pass
-else:
-env[k] = token
-k = None
+with open((src_dir / "net.config").as_posix(), 'r') as fp:
+for line in fp.readlines():
+full_file = line
+# Strip comments
+pos = line.find("#")
+if pos >= 0:
+line = line[:pos]
+line = line.strip()
+if not line:
+continue
+pair = line.split('=', maxsplit=1)
+if len(pair) != 2:
+raise Exception("Can't parse configuration line:", full_file)
+env[pair[0]] = pair[1]
return env
--
2.44.0
[PATCH net-next v2 0/3] selftests: drv-net: round some sharp edges
I had to explain how to run the driver tests twice already. Improve the README so we can just point to it. Improve the config validation. v2: - use split() in patch 2 - more validation in last patch v1: https://lore.kernel.org/r/20240424221444.4194069-1-k...@kernel.org/ Jakub Kicinski (3): selftests: drv-net: extend the README with more info and example selftests: drv-net: reimplement the config parser selftests: drv-net: validate the environment .../testing/selftests/drivers/net/README.rst | 97 --- .../selftests/drivers/net/lib/py/env.py | 51 +++--- 2 files changed, 124 insertions(+), 24 deletions(-) -- 2.44.0
Re: [PATCH net-next 0/4] selftests: drv-net: round some sharp edges
On Wed, 24 Apr 2024 21:45:49 -0400 Willem de Bruijn wrote: > Left two minor points comments inline, but nothing important. I'll respin, apply your suggestions and drop the unfortunate patch.
Re: [PATCH 0/2] tools/nolibc: implement strtol() and friends
On 2024-04-25 18:30:39+, Willy Tarreau wrote: > Hi again Thomas, > > On Thu, Apr 25, 2024 at 06:09:25PM +0200, Thomas Weißschuh wrote: > > I wanted to implement sscanf() for ksft_min_kernel_version() and this is > > a prerequisite for it. > > > > It's also useful on its own so it gets its own submission. > > > > Signed-off-by: Thomas Weißschuh > > Nice work, thank you. For the whole series, modulo my small comments on > 2/2: Thanks for those great catches! I addressed them and pushed the result to nolibc/next. > Acked-by: Willy Tarreau Thomas
[PATCH v2 4/4] selftests/resctrl: Enable MBA/MBA tests on AMD
Enable MBA/MBM tests if UMC (Unified Memory Controller) support is
available on the system. Tests will be skipped otherwise.
Update noncont_cat_run_test to check for vendor. AMD supports
non contiguous CBM masks but does not report it via CPUID.
Signed-off-by: Babu Moger
---
tools/testing/selftests/resctrl/cat_test.c | 2 +-
tools/testing/selftests/resctrl/mba_test.c | 1 -
tools/testing/selftests/resctrl/mbm_test.c | 1 -
3 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c
b/tools/testing/selftests/resctrl/cat_test.c
index 4cb991be8e31..b682eaf65bfd 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -314,7 +314,7 @@ static int noncont_cat_run_test(const struct resctrl_test
*test,
else
return -EINVAL;
- if (sparse_masks != ((ecx >> 3) & 1)) {
+ if ((get_vendor() == ARCH_INTEL) && sparse_masks != ((ecx >> 3) & 1)) {
ksft_print_msg("CPUID output doesn't match 'sparse_masks' file
content!\n");
return 1;
}
diff --git a/tools/testing/selftests/resctrl/mba_test.c
b/tools/testing/selftests/resctrl/mba_test.c
index 7946e32e85c8..353054724fa7 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -177,7 +177,6 @@ static bool mba_feature_check(const struct resctrl_test
*test)
struct resctrl_test mba_test = {
.name = "MBA",
.resource = "MB",
- .vendor_specific = ARCH_INTEL,
.feature_check = mba_feature_check,
.run_test = mba_run_test,
};
diff --git a/tools/testing/selftests/resctrl/mbm_test.c
b/tools/testing/selftests/resctrl/mbm_test.c
index d67ffa3ec63a..f2223315b5b8 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -147,7 +147,6 @@ static bool mbm_feature_check(const struct resctrl_test
*test)
struct resctrl_test mbm_test = {
.name = "MBM",
.resource = "MB",
- .vendor_specific = ARCH_INTEL,
.feature_check = mbm_feature_check,
.run_test = mbm_run_test,
};
--
2.34.1
[PATCH v2 3/4] selftests/resctrl: Add support for MBM and MBA tests on AMD
Add support to read UMC (Unified Memory Controller) perf events to compare
the numbers with QoS monitor for AMD.
Signed-off-by: Babu Moger
---
tools/testing/selftests/resctrl/resctrl_val.c | 67 ---
1 file changed, 59 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c
b/tools/testing/selftests/resctrl/resctrl_val.c
index e3b09128ec3d..d90d3196d7b5 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -11,6 +11,7 @@
#include "resctrl.h"
#define UNCORE_IMC "uncore_imc"
+#define AMD_UMC"amd_umc"
#define READ_FILE_NAME "events/cas_count_read"
#define WRITE_FILE_NAME"events/cas_count_write"
#define DYN_PMU_PATH "/sys/bus/event_source/devices"
@@ -146,6 +147,47 @@ static int open_perf_event(int i, int cpu_no, int j)
return 0;
}
+/* Get type and config (read and write) of an UMC counter */
+static int read_from_umc_dir(char *umc_dir, int count)
+{
+ char umc_counter_type[PATH_MAX];
+ FILE *fp;
+
+ /* Get type of iMC counter */
+ sprintf(umc_counter_type, "%s%s", umc_dir, "type");
+ fp = fopen(umc_counter_type, "r");
+ if (!fp) {
+ ksft_perror("Failed to open imc counter type file");
+ return -1;
+ }
+
+ if (fscanf(fp, "%u", _counters_config[count][READ].type) <= 0) {
+ ksft_perror("Could not get imc type");
+ fclose(fp);
+ return -1;
+ }
+
+ fclose(fp);
+
+ imc_counters_config[count][WRITE].type =
+ imc_counters_config[count][READ].type;
+
+ /*
+* Setup the event and umasks for UMC events
+* Number of CAS commands sent for reads:
+* EventCode = 0x0a, umask = 0x1
+* Number of CAS commands sent for writes:
+* EventCode = 0x0a, umask = 0x2
+*/
+ imc_counters_config[count][READ].event = 0xa;
+ imc_counters_config[count][READ].umask = 0x1;
+
+ imc_counters_config[count][WRITE].event = 0xa;
+ imc_counters_config[count][WRITE].umask = 0x2;
+
+ return 0;
+}
+
/* Get type and config (read and write) of an iMC counter */
static int read_from_imc_dir(char *imc_dir, int count)
{
@@ -233,6 +275,9 @@ static int num_of_mem_controllers(void)
if (vendor == ARCH_INTEL) {
sysfs_name = UNCORE_IMC;
size = sizeof(UNCORE_IMC);
+ } else if (vendor == ARCH_AMD) {
+ sysfs_name = AMD_UMC;
+ size = sizeof(AMD_UMC);
} else {
perror("Unsupported Vendor!\n");
return -1;
@@ -246,11 +291,12 @@ static int num_of_mem_controllers(void)
continue;
/*
-* imc counters are named as "uncore_imc_", hence
-* increment the pointer to point to . Note that
-* sizeof(UNCORE_IMC) would count for null character as
-* well and hence the last underscore character in
-* uncore_imc'_' need not be counted.
+* Intel imc counters are named as "uncore_imc_",
+* hence increment the pointer to point to .
+* Note that sizeof(UNCORE_IMC) would count for null
+* character as well and hence the last underscore
+* character in uncore_imc'_' need not be counted.
+* For AMD, it will be amd_umc_.
*/
temp = temp + size;
@@ -262,7 +308,11 @@ static int num_of_mem_controllers(void)
if (temp[0] >= '0' && temp[0] <= '9') {
sprintf(imc_dir, "%s/%s/", DYN_PMU_PATH,
ep->d_name);
- ret = read_from_imc_dir(imc_dir, count);
+ if (vendor == ARCH_INTEL)
+ ret = read_from_imc_dir(imc_dir, count);
+ else
+ ret = read_from_umc_dir(imc_dir, count);
+
if (ret) {
closedir(dp);
@@ -273,8 +323,9 @@ static int num_of_mem_controllers(void)
}
closedir(dp);
if (count == 0) {
- ksft_print_msg("Unable to find iMC counters\n");
-
+ ksft_print_msg("Unable to find iMC/UMC counters\n");
+ if (vendor == ARCH_AMD)
+ ksft_print_msg("Try loading amd_uncore
module\n");
return -1;
}
} else {
--
2.34.1
[PATCH v2 2/4] selftests/resctrl: Pass sysfs controller name of the vendor
Detect the vendor and pass the sysfs name for the vendor for searching
the controller information.
Signed-off-by: Babu Moger
---
tools/testing/selftests/resctrl/resctrl_val.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c
b/tools/testing/selftests/resctrl/resctrl_val.c
index a30cfcff605f..e3b09128ec3d 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -224,14 +224,24 @@ static int num_of_mem_controllers(void)
{
char imc_dir[512], *temp;
unsigned int count = 0;
+ int ret, vendor, size;
struct dirent *ep;
- int ret;
+ char *sysfs_name;
DIR *dp;
+ vendor = get_vendor();
+ if (vendor == ARCH_INTEL) {
+ sysfs_name = UNCORE_IMC;
+ size = sizeof(UNCORE_IMC);
+ } else {
+ perror("Unsupported Vendor!\n");
+ return -1;
+ }
+
dp = opendir(DYN_PMU_PATH);
if (dp) {
while ((ep = readdir(dp))) {
- temp = strstr(ep->d_name, UNCORE_IMC);
+ temp = strstr(ep->d_name, sysfs_name);
if (!temp)
continue;
@@ -242,7 +252,7 @@ static int num_of_mem_controllers(void)
* well and hence the last underscore character in
* uncore_imc'_' need not be counted.
*/
- temp = temp + sizeof(UNCORE_IMC);
+ temp = temp + size;
/*
* Some directories under "DYN_PMU_PATH" could have
--
2.34.1
[PATCH v2 1/4] selftests/resctrl: Rename variable imcs and num_of_imcs() to generic names
In an effort to support MBM and MBA tests for AMD, renaming for variable
and functions to generic names. For Intel, the memory controller is called
Integrated Memory Controllers (IMC). For AMD, it is called Unified
Memory Controller (UMC). No functional change.
Signed-off-by: Babu Moger
---
tools/testing/selftests/resctrl/resctrl_val.c | 59 ++-
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c
b/tools/testing/selftests/resctrl/resctrl_val.c
index 5a49f07a6c85..a30cfcff605f 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -60,7 +60,7 @@ struct imc_counter_config {
};
static char mbm_total_path[1024];
-static int imcs;
+static int mcs;
static struct imc_counter_config imc_counters_config[MAX_IMCS][2];
void membw_initialize_perf_event_attr(int i, int j)
@@ -211,15 +211,16 @@ static int read_from_imc_dir(char *imc_dir, int count)
}
/*
- * A system can have 'n' number of iMC (Integrated Memory Controller)
- * counters, get that 'n'. For each iMC counter get it's type and config.
+ * A system can have 'n' number of iMC (Integrated Memory Controller for
+ * Intel) counters, get that 'n'. In case of AMD it is called UMC (Unified
+ * Memory Controller). For each iMC/UMC counter get it's type and config.
* Also, each counter has two configs, one for read and the other for write.
* A config again has two parts, event and umask.
* Enumerate all these details into an array of structures.
*
* Return: >= 0 on success. < 0 on failure.
*/
-static int num_of_imcs(void)
+static int num_of_mem_controllers(void)
{
char imc_dir[512], *temp;
unsigned int count = 0;
@@ -275,25 +276,25 @@ static int num_of_imcs(void)
return count;
}
-static int initialize_mem_bw_imc(void)
+static int initialize_mem_bw_mc(void)
{
- int imc, j;
+ int mc, j;
- imcs = num_of_imcs();
- if (imcs <= 0)
- return imcs;
+ mcs = num_of_mem_controllers();
+ if (mcs <= 0)
+ return mcs;
/* Initialize perf_event_attr structures for all iMC's */
- for (imc = 0; imc < imcs; imc++) {
+ for (mc = 0; mc < mcs; mc++) {
for (j = 0; j < 2; j++)
- membw_initialize_perf_event_attr(imc, j);
+ membw_initialize_perf_event_attr(mc, j);
}
return 0;
}
/*
- * get_mem_bw_imc: Memory band width as reported by iMC counters
+ * get_mem_bw_mc: Memory band width as reported by iMC counters
* @cpu_no:CPU number that the benchmark PID is binded to
* @bw_report: Bandwidth report type (reads, writes)
*
@@ -302,40 +303,40 @@ static int initialize_mem_bw_imc(void)
*
* Return: = 0 on success. < 0 on failure.
*/
-static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
+static int get_mem_bw_mc(int cpu_no, char *bw_report, float *bw_imc)
{
float reads, writes, of_mul_read, of_mul_write;
- int imc, j, ret;
+ int mc, j, ret;
/* Start all iMC counters to log values (both read and write) */
reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
- for (imc = 0; imc < imcs; imc++) {
+ for (mc = 0; mc < mcs; mc++) {
for (j = 0; j < 2; j++) {
- ret = open_perf_event(imc, cpu_no, j);
+ ret = open_perf_event(mc, cpu_no, j);
if (ret)
return -1;
}
for (j = 0; j < 2; j++)
- membw_ioctl_perf_event_ioc_reset_enable(imc, j);
+ membw_ioctl_perf_event_ioc_reset_enable(mc, j);
}
sleep(1);
/* Stop counters after a second to get results (both read and write) */
- for (imc = 0; imc < imcs; imc++) {
+ for (mc = 0; mc < mcs; mc++) {
for (j = 0; j < 2; j++)
- membw_ioctl_perf_event_ioc_disable(imc, j);
+ membw_ioctl_perf_event_ioc_disable(mc, j);
}
/*
* Get results which are stored in struct type imc_counter_config
* Take over flow into consideration before calculating total b/w
*/
- for (imc = 0; imc < imcs; imc++) {
+ for (mc = 0; mc < mcs; mc++) {
struct imc_counter_config *r =
- _counters_config[imc][READ];
+ _counters_config[mc][READ];
struct imc_counter_config *w =
- _counters_config[imc][WRITE];
+ _counters_config[mc][WRITE];
if (read(r->fd, >return_value,
sizeof(struct membw_read_format)) == -1) {
@@ -368,9 +369,9 @@ static int get_mem_bw_imc(int cpu_no, char *bw_report,
float *bw_imc)
writes += w->return_value.value *
[PATCH v2 0/4] selftests/resctrl: Enable MBM and MBA tests on AMD
The MBM (Memory Bandwidth Monitoring) and MBA (Memory Bandwidth Allocation)
features are not enabled for AMD systems. The reason was lack of perf
counters to compare the resctrl test results.
Starting with the commit
25e56847821f ("perf/x86/amd/uncore: Add memory controller support"), AMD
now supports the UMC (Unified Memory Controller) perf events. These events
can be used to compare the test results.
This series adds the support to detect the UMC events and enable MBM/MBA
tests for AMD systems.
v2: Changes.
a. Rebased on top of tip/master (Apr 25, 2024)
b. Addressed Ilpo comments except the one about close call.
It seems more clear to keep READ and WRITE separate.
https://lore.kernel.org/lkml/8e4badb7-6cc5-61f1-e041-d902209a9...@linux.intel.com/
c. Used ksft_perror call when applicable.
d. Added vendor check for non contiguous CBM check.
v1: https://lore.kernel.org/lkml/cover.1708637563.git.babu.mo...@amd.com/
Babu Moger (4):
selftests/resctrl: Rename variable imcs and num_of_imcs() to generic
names
selftests/resctrl: Pass sysfs controller name of the vendor
selftests/resctrl: Add support for MBM and MBA tests on AMD
selftests/resctrl: Enable MBA/MBA tests on AMD
tools/testing/selftests/resctrl/cat_test.c| 2 +-
tools/testing/selftests/resctrl/mba_test.c| 1 -
tools/testing/selftests/resctrl/mbm_test.c| 1 -
tools/testing/selftests/resctrl/resctrl_val.c | 142 +-
4 files changed, 103 insertions(+), 43 deletions(-)
--
2.34.1
Re: [PATCH v3] selftests/bpf: Add ring_buffer__consume_n test.
On Thu, Apr 25, 2024 at 11:47:07AM -0700, Andrii Nakryiko wrote:
> On Thu, Apr 25, 2024 at 7:06 AM Andrea Righi
> wrote:
> >
> > Add a testcase for the ring_buffer__consume_n() API.
> >
> > The test produces multiple samples in a ring buffer, using a
> > sys_getpid() fentry prog, and consumes them from user-space in batches,
> > rather than consuming all of them greedily, like ring_buffer__consume()
> > does.
> >
> > Link:
> > https://lore.kernel.org/lkml/CAEf4BzaR4zqUpDmj44KNLdpJ=tpa97grvzuzvno5nm6b7ow...@mail.gmail.com
> > Signed-off-by: Andrea Righi
> > ---
> > tools/testing/selftests/bpf/Makefile | 2 +-
> > .../selftests/bpf/prog_tests/ringbuf.c| 64 +++
> > .../selftests/bpf/progs/test_ringbuf_n.c | 47 ++
> > 3 files changed, 112 insertions(+), 1 deletion(-)
> > create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_n.c
> >
> > ChangeLog v2 -> v3:
> > - move skel_n inside ringbuf_n_subtest()
> >
> > ChangeLog v1 -> v2:
> > - replace CHECK() with ASSERT_EQ()
> > - fix skel -> skel_n
> > - drop unused "seq" field from struct sample
> >
>
> [...]
>
> > + /* Produce N_TOT_SAMPLES samples in the ring buffer by calling
> > getpid() */
> > + skel_n->bss->value = SAMPLE_VALUE;
> > + for (i = 0; i < N_TOT_SAMPLES; i++)
> > + syscall(__NR_getpgid);
> > +
> > + /* Consume all samples from the ring buffer in batches of N_SAMPLES
> > */
> > + for (i = 0; i < N_TOT_SAMPLES; i += err) {
> > + err = ring_buffer__consume_n(ringbuf, N_SAMPLES);
> > + ASSERT_EQ(err, N_SAMPLES, "rb_consume");
>
> if something goes wrong and err is < 0, we might end up with a very
> long loop. I changed this to:
>
> if (!ASSERT_EQ(...))
> goto cleanup_ringbuf;
>
> to avoid this problem
Looks good, tested, just in case, and it works a expected.
Thanks!
-Andrea
>
> > + }
> > +
> > +cleanup_ringbuf:
> > + ring_buffer__free(ringbuf);
> > +cleanup:
> > + test_ringbuf_n_lskel__destroy(skel_n);
> > +}
> > +
>
> [...]
Re: [ovs-dev] selftests: openvswitch: Questions about possible enhancements
Jakub Kicinski writes: > On Thu, 25 Apr 2024 19:57:19 +0100 Simon Horman wrote: >> openvswitch.sh does not appear to have any dependencies on Open vSwitch >> user-space. My understanding is that, rather, it makes use of >> tools/testing/selftests/net/openvswitch/ovs-dpctl.py to talk to the Kernel >> using Netlink (which is also what Open vSwitch user-space does). >> >> My brief testing indicates that for this the only dependencies >> when running on Amazon Linux 2 are python3 and pyroute2. >> >> I think that it should be possible to port pmtu.sh to use ovs-dpctl.py. >> This would require some enhancements to ovs-dpctl.py to handle adding the >> tunnel vports (interfaces). I have some work from some time back: https://github.com/apconole/linux-next-work/commit/61d2b9fc68a4e3fc950a24b06232c2fbcbfa0372 https://github.com/apconole/linux-next-work/commit/7262107de7170d44b6dbf6c5ea6f7e6c0bb71d36 https://github.com/apconole/linux-next-work/commit/af5338c9044660fa0eaaa967602aec706bbaeb5b I was using it to try and build up a test to check recursions in the datapath, but didn't get a chance to finish. BUT most of the stuff is there, just needs to be cleaned up. It would at least cover the classic case, but the LWT support needs to be added. >> As an aside, to run the Open vSwitch tests in pmtu.sh the openvswitch >> kernel module is needed. So I think it would make sense to add >> CONFIG_OPENVSWITCH to tools/testing/selftests/net/config. >> >> That would mean that tools/testing/selftests/net/config also has all >> the requirements to run openvswitch.sh. If so, we probably wouldn't need to >> add tools/testing/selftests/net/openvswitch/config or otherwise do anything >> special to configure the kernel for openvswitch.sh. > > That sounds great, for simplicity we could move the ovs files down > to the .../net/ directory. It'd be cool to not have to do that, and > let us separate tests more clearly into directories. But right now > every directory has its own runner so there's a high price to pay > for a primarily aesthetic gain :( Either one would work for me. I will repost the RFC addressing some of the comments. It should be runnable in a bare VM that has the required python packages that Simon notes (pyroute2 and python3).
Re: selftests: openvswitch: Questions about possible enhancements
Simon Horman writes:
> On Wed, Apr 24, 2024 at 02:14:09PM -0400, Aaron Conole wrote:
>> Simon Horman writes:
>>
>> > Hi Aaron, Jakub, all,
>> >
>> > I have recently been exercising the Open vSwitch kernel selftests,
>> > using vng, something like this:
>> >
>> >TESTDIR="tools/testing/selftests/net/openvswitch"
>> >
>> > vng -v --run . --user root --cpus 2 \
>> > --overlay-rwdir "$PWD" -- \
>> > "modprobe openvswitch && \
>> > echo \"timeout=90\" >> \"${TESTDIR}/settings\" && \
>> > make -C \"$TESTDIR\" run_tests"
>> >
>> > And I have some observations that I'd like to ask about.
>> >
>> > 1. Building the kernel using the following command does not
>> >build the openvswitch kernel module.
>> >
>> >vng -v --build \
>> >--config tools/testing/selftests/net/config
>> >
>> >All that seems to be missing is CONFIG_OPENVSWITCH=m
>> >and I am wondering what the best way of resolving this is.
>> >
>> >Perhaps I am doing something wrong.
>> >Or perhaps tools/testing/selftests/net/openvswitch/config
>> >should be created? If so, should it include (most of?) what is in
>> >tools/testing/selftests/net/config, or just CONFIG_OPENVSWITCH=m?
>>
>> I have a series that I need to get back to fixing:
>>
>> https://mail.openvswitch.org/pipermail/ovs-dev/2024-February/411917.html
>>
>> which does include the config listed, and some of the fixes for things
>> you've noted.
>>
>> I think it makes sense to get back to it.
>
> Thanks Aaron,
>
> I was hoping you might say something like that.
>
> WRT to the config itself, as Benjamin mentioned elsewhere in this thread [1]
> there is a question about how this should be handled consistently for
> all selftests.
>
> [1] https://lore.kernel.org/netdev/ZilIgbIvB04iUal2@f4/
Yeah, I think it makes sense. There are probably some other bashisms
beyond the substitution noted. I'll add it to the RFC and rework.
>>
>> > 2. As per my example above, it seems that a modprobe openvswitch is
>> >required (if openvswitch is a module).
>> >
>> >Again, perhaps I am doing something wrong. But if not, should this be
>> >incorporated into tools/testing/selftests/net/openvswitch/openvswitch.sh
>> >or otherwise automated?
>> >
>> > 3. I have observed that the last test fails (yesterday, but not today!),
>> >because the namespace it tries to create already exists. I believe this
>> >is because it is pending deletion.
>> >
>> >My work-around is as follows:
>> >
>> > ovs_add_netns_and_veths () {
>> >info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
>> > + for i in $(seq 10); do
>> > + ovs_sbx "$1" test -e "/var/run/netns/$3" || break
>> > + info "Namespace $3 still exists (attempt $i)"
>> > + ovs_sbx "$1" ip netns del "$3"
>> > + sleep "$i"
>> > + done
>> >ovs_sbx "$1" ip netns add "$3" || return 1
>> >on_exit "ovs_sbx $1 ip netns del $3"
>> >ovs_sbx "$1" ip link add "$4" type veth peer name "$5" || return 1
>> >
>> >N.B.: the "netns del" part is probably not needed,
>> >but I'm not able to exercise it effectively right now.
>> >
>> >I am wondering if a loop like this is appropriate to add, perhaps also
>> >to namespace deletion. Or if it would be appropriate to port
>> >openvswitch.sh to use ./tools/testing/selftests/net/lib.sh, which I
>> >believe handles this.
>> >
>> > 4. I am observing timeouts whith the default value of 45s.
>> >Bumping this to 90s seems to help.
>> >Are there any objections to a patch to bump the timeout?
>
> Regarding points 3 and 4.
>
> I did a bit more testing after I sent my email yesterday.
> I have two test machines. It turns out, to my surprise, that one is
> much slower than the other when running openvswitch.sh with vng.
>
> I am unsure why, but that isn't really on topic. The point
> is that I'm currently only seeing problems 3 and 4 manifest
> on the slow machine.
>
> I think problem 3 is probably worth solving.
> But the timeout question is more subjective.
Re: selftests: openvswitch: Questions about possible enhancements
Jakub Kicinski writes: > On Wed, 24 Apr 2024 17:44:05 +0100 Simon Horman wrote: >> I have recently been exercising the Open vSwitch kernel selftests, >> using vng, > > Speaking of ovs tests, we currently don't run them in CI (and suffer > related skips in pmtu.sh) because Amazon Linux doesn't have ovs > packaged and building it looks pretty hard. > > Is there an easy way to build just the CLI tooling or get a pre-built > package somewhere? As Simon notes - we would need some support in the ovs-dpctl.py to set up the tunnel interfaces, and probably need to set them up for lwt and classic tunnels. I have a test branch where I have support for the former, and I can clean it up and submit it. > Or perhaps you'd be willing to run the OvS tests and we can move > the part of pmtu.sh into OvS test dir? I guess either would be fine, as long as they can get run.
[PATCH] riscv: selftests: Add hwprobe binaries to .gitignore
The cbo and which-cpu hwprobe selftests leave their artifacts in the
kernel tree and end up being tracked by git. Add the binaries to the
hwprobe selftest .gitignore so this no longer happens.
Signed-off-by: Charlie Jenkins
Fixes: a29e2a48afe3 ("RISC-V: selftests: Add CBO tests")
Fixes: ef7d6abb2cf5 ("RISC-V: selftests: Add which-cpus hwprobe test")
---
tools/testing/selftests/riscv/hwprobe/.gitignore | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/riscv/hwprobe/.gitignore
b/tools/testing/selftests/riscv/hwprobe/.gitignore
index 8113dc3bdd03..6e384e80ea1a 100644
--- a/tools/testing/selftests/riscv/hwprobe/.gitignore
+++ b/tools/testing/selftests/riscv/hwprobe/.gitignore
@@ -1 +1,3 @@
hwprobe
+cbo
+which-cpus
---
base-commit: ed30a4a51bb196781c8058073ea720133a65596f
change-id: 20240425-gitignore_hwprobe_artifacts-fb0f2cd3509c
--
- Charlie
Re: [PATCH 2/3] bpf: do not walk twice the hash map on free
On Thu, Apr 25, 2024 at 6:59 AM Benjamin Tissoires wrote:
>
> If someone stores both a timer and a workqueue in a hash map, on free, we
> would walk it twice.
> Add a check in htab_free_malloced_timers_or_wq and free the timers
> and workqueues if they are present.
>
> Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps
> and hashmaps")
> Signed-off-by: Benjamin Tissoires
> ---
> kernel/bpf/hashtab.c | 16 +---
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 0179183c543a..20162ae741e9 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -1515,7 +1515,7 @@ static void delete_all_elements(struct bpf_htab *htab)
> migrate_enable();
> }
>
> -static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab, bool
> is_timer)
> +static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab)
> {
> int i;
>
> @@ -1527,10 +1527,10 @@ static void htab_free_malloced_timers_or_wq(struct
> bpf_htab *htab, bool is_timer
>
> hlist_nulls_for_each_entry(l, n, head, hash_node) {
> /* We only free timer on uref dropping to zero */
> - if (is_timer)
> + if (btf_record_has_field(htab->map.record, BPF_TIMER))
> bpf_obj_free_timer(htab->map.record,
>l->key +
> round_up(htab->map.key_size, 8));
> - else
> + if (btf_record_has_field(htab->map.record,
> BPF_WORKQUEUE))
> bpf_obj_free_workqueue(htab->map.record,
>l->key +
> round_up(htab->map.key_size, 8));
> }
> @@ -1544,18 +1544,12 @@ static void htab_map_free_timers_and_wq(struct
> bpf_map *map)
> struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
>
> /* We only free timer and workqueue on uref dropping to zero */
> - if (btf_record_has_field(htab->map.record, BPF_TIMER)) {
> + if (btf_record_has_field(htab->map.record, BPF_TIMER |
> BPF_WORKQUEUE)) {
> if (!htab_is_prealloc(htab))
> - htab_free_malloced_timers_or_wq(htab, true);
> + htab_free_malloced_timers_or_wq(htab);
> else
> htab_free_prealloced_timers(htab);
> }
> - if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE)) {
> - if (!htab_is_prealloc(htab))
> - htab_free_malloced_timers_or_wq(htab, false);
> - else
> - htab_free_prealloced_wq(htab);
This looks wrong.
htab_free_prealloced_wq() is now unused as compiler says:
../kernel/bpf/hashtab.c:243:13: warning: ‘htab_free_prealloced_wq’
defined but not used [-Wunused-function]
243 | static void htab_free_prealloced_wq(struct bpf_htab *htab)
| ^~~
and prealloced maps with wq leak wq-s.
pw-bot: cr
Re: [ovs-dev] selftests: openvswitch: Questions about possible enhancements
On Thu, 25 Apr 2024 19:57:19 +0100 Simon Horman wrote: > openvswitch.sh does not appear to have any dependencies on Open vSwitch > user-space. My understanding is that, rather, it makes use of > tools/testing/selftests/net/openvswitch/ovs-dpctl.py to talk to the Kernel > using Netlink (which is also what Open vSwitch user-space does). > > My brief testing indicates that for this the only dependencies > when running on Amazon Linux 2 are python3 and pyroute2. > > I think that it should be possible to port pmtu.sh to use ovs-dpctl.py. > This would require some enhancements to ovs-dpctl.py to handle adding the > tunnel vports (interfaces). > > As an aside, to run the Open vSwitch tests in pmtu.sh the openvswitch > kernel module is needed. So I think it would make sense to add > CONFIG_OPENVSWITCH to tools/testing/selftests/net/config. > > That would mean that tools/testing/selftests/net/config also has all > the requirements to run openvswitch.sh. If so, we probably wouldn't need to > add tools/testing/selftests/net/openvswitch/config or otherwise do anything > special to configure the kernel for openvswitch.sh. That sounds great, for simplicity we could move the ovs files down to the .../net/ directory. It'd be cool to not have to do that, and let us separate tests more clearly into directories. But right now every directory has its own runner so there's a high price to pay for a primarily aesthetic gain :(
Re: [ovs-dev] selftests: openvswitch: Questions about possible enhancements
+ Aaron On Thu, Apr 25, 2024 at 09:26:37AM +0100, Simon Horman wrote: > On Wed, Apr 24, 2024 at 05:30:00PM -0700, Jakub Kicinski wrote: > > On Wed, 24 Apr 2024 17:44:05 +0100 Simon Horman wrote: > > > I have recently been exercising the Open vSwitch kernel selftests, > > > using vng, > > > > Speaking of ovs tests, we currently don't run them in CI (and suffer > > related skips in pmtu.sh) because Amazon Linux doesn't have ovs > > packaged and building it looks pretty hard. > > > > Is there an easy way to build just the CLI tooling or get a pre-built > > package somewhere? > > > > Or perhaps you'd be willing to run the OvS tests and we can move > > the part of pmtu.sh into OvS test dir? > > Thanks Jakub, > > The plot thickens. > We'll look into this (Hi Aaron!). Hi again, I took a look into this. openvswitch.sh does not appear to have any dependencies on Open vSwitch user-space. My understanding is that, rather, it makes use of tools/testing/selftests/net/openvswitch/ovs-dpctl.py to talk to the Kernel using Netlink (which is also what Open vSwitch user-space does). My brief testing indicates that for this the only dependencies when running on Amazon Linux 2 are python3 and pyroute2. I think that it should be possible to port pmtu.sh to use ovs-dpctl.py. This would require some enhancements to ovs-dpctl.py to handle adding the tunnel vports (interfaces). As an aside, to run the Open vSwitch tests in pmtu.sh the openvswitch kernel module is needed. So I think it would make sense to add CONFIG_OPENVSWITCH to tools/testing/selftests/net/config. That would mean that tools/testing/selftests/net/config also has all the requirements to run openvswitch.sh. If so, we probably wouldn't need to add tools/testing/selftests/net/openvswitch/config or otherwise do anything special to configure the kernel for openvswitch.sh.
Re: [PATCH v3] selftests/bpf: Add ring_buffer__consume_n test.
On Thu, Apr 25, 2024 at 7:06 AM Andrea Righi wrote:
>
> Add a testcase for the ring_buffer__consume_n() API.
>
> The test produces multiple samples in a ring buffer, using a
> sys_getpid() fentry prog, and consumes them from user-space in batches,
> rather than consuming all of them greedily, like ring_buffer__consume()
> does.
>
> Link:
> https://lore.kernel.org/lkml/CAEf4BzaR4zqUpDmj44KNLdpJ=tpa97grvzuzvno5nm6b7ow...@mail.gmail.com
> Signed-off-by: Andrea Righi
> ---
> tools/testing/selftests/bpf/Makefile | 2 +-
> .../selftests/bpf/prog_tests/ringbuf.c| 64 +++
> .../selftests/bpf/progs/test_ringbuf_n.c | 47 ++
> 3 files changed, 112 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_n.c
>
> ChangeLog v2 -> v3:
> - move skel_n inside ringbuf_n_subtest()
>
> ChangeLog v1 -> v2:
> - replace CHECK() with ASSERT_EQ()
> - fix skel -> skel_n
> - drop unused "seq" field from struct sample
>
[...]
> + /* Produce N_TOT_SAMPLES samples in the ring buffer by calling
> getpid() */
> + skel_n->bss->value = SAMPLE_VALUE;
> + for (i = 0; i < N_TOT_SAMPLES; i++)
> + syscall(__NR_getpgid);
> +
> + /* Consume all samples from the ring buffer in batches of N_SAMPLES */
> + for (i = 0; i < N_TOT_SAMPLES; i += err) {
> + err = ring_buffer__consume_n(ringbuf, N_SAMPLES);
> + ASSERT_EQ(err, N_SAMPLES, "rb_consume");
if something goes wrong and err is < 0, we might end up with a very
long loop. I changed this to:
if (!ASSERT_EQ(...))
goto cleanup_ringbuf;
to avoid this problem
> + }
> +
> +cleanup_ringbuf:
> + ring_buffer__free(ringbuf);
> +cleanup:
> + test_ringbuf_n_lskel__destroy(skel_n);
> +}
> +
[...]
Re: [linus:master] [selftests/harness] 0710a1a73f: kernel-selftests.pidfd.pidfd_setns_test.fail
FYI, I'm working on this issue.
Regards,
Mickaël
On Fri, Mar 29, 2024 at 10:42:51AM +0800, kernel test robot wrote:
>
>
> Hello,
>
> kernel test robot noticed "kernel-selftests.pidfd.pidfd_setns_test.fail" on:
>
> commit: 0710a1a73fb45033ebb06073e374ab7d44a05f15 ("selftests/harness: Merge
> TEST_F_FORK() into TEST_F()")
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>
> [test failed on linus/master 4cece764965020c22cff7665b18a012006359095]
>
> in testcase: kernel-selftests
> version: kernel-selftests-x86_64-4306b286-1_20240301
> with following parameters:
>
> group: pidfd
>
>
>
> compiler: gcc-12
> test machine: 36 threads 1 sockets Intel(R) Core(TM) i9-10980XE CPU @ 3.00GHz
> (Cascade Lake) with 32G memory
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
>
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version
> of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot
> | Closes:
> https://lore.kernel.org/oe-lkp/202403291015.1fcfa957-oliver.s...@intel.com
>
>
>
> # timeout set to 300
> # selftests: pidfd: pidfd_setns_test
> # TAP version 13
> # 1..7
> # # Starting 7 tests from 2 test cases.
> # # RUN global.setns_einval ...
> # #OK global.setns_einval
> # ok 1 global.setns_einval
> # # RUN current_nsset.invalid_flags ...
> # # pidfd_setns_test.c:161:invalid_flags:Expected self->child_pid_exited (0)
> > 0 (0)
> # #OK current_nsset.invalid_flags
> # ok 2 current_nsset.invalid_flags
> # # RUN current_nsset.pidfd_exited_child ...
> # # pidfd_setns_test.c:161:pidfd_exited_child:Expected self->child_pid_exited
> (0) > 0 (0)
> # #OK current_nsset.pidfd_exited_child
> # ok 3 current_nsset.pidfd_exited_child
> # # RUN current_nsset.pidfd_incremental_setns ...
> # # pidfd_setns_test.c:161:pidfd_incremental_setns:Expected
> self->child_pid_exited (0) > 0 (0)
> # # pidfd_setns_test.c:408:pidfd_incremental_setns:Managed to correctly setns
> to user namespace of 45423 via pidfd 20
> # # pidfd_setns_test.c:408:pidfd_incremental_setns:Managed to correctly setns
> to mnt namespace of 45423 via pidfd 20
> # # pidfd_setns_test.c:408:pidfd_incremental_setns:Managed to correctly setns
> to pid namespace of 45423 via pidfd 20
> # # pidfd_setns_test.c:408:pidfd_incremental_setns:Managed to correctly setns
> to uts namespace of 45423 via pidfd 20
> # # pidfd_setns_test.c:408:pidfd_incremental_setns:Managed to correctly setns
> to ipc namespace of 45423 via pidfd 20
> # # pidfd_setns_test.c:408:pidfd_incremental_setns:Managed to correctly setns
> to net namespace of 45423 via pidfd 20
> # # pidfd_setns_test.c:408:pidfd_incremental_setns:Managed to correctly setns
> to cgroup namespace of 45423 via pidfd 20
> # # pidfd_setns_test.c:408:pidfd_incremental_setns:Managed to correctly setns
> to pid_for_children namespace of 45423 via pidfd 20
> # # pidfd_setns_test.c:391:pidfd_incremental_setns:Expected
> setns(self->child_pidfd1, info->flag) (-1) == 0 (0)
> # # pidfd_setns_test.c:392:pidfd_incremental_setns:Too many users - Failed to
> setns to time namespace of 45423 via pidfd 20
> # # pidfd_incremental_setns: Test terminated by timeout
> # # FAIL current_nsset.pidfd_incremental_setns
> # not ok 4 current_nsset.pidfd_incremental_setns
> # # RUN current_nsset.nsfd_incremental_setns ...
> # # pidfd_setns_test.c:161:nsfd_incremental_setns:Expected
> self->child_pid_exited (0) > 0 (0)
> # # pidfd_setns_test.c:444:nsfd_incremental_setns:Managed to correctly setns
> to user namespace of 45524 via nsfd 19
> # # pidfd_setns_test.c:444:nsfd_incremental_setns:Managed to correctly setns
> to mnt namespace of 45524 via nsfd 24
> # # pidfd_setns_test.c:444:nsfd_incremental_setns:Managed to correctly setns
> to pid namespace of 45524 via nsfd 27
> # # pidfd_setns_test.c:444:nsfd_incremental_setns:Managed to correctly setns
> to uts namespace of 45524 via nsfd 30
> # # pidfd_setns_test.c:444:nsfd_incremental_setns:Managed to correctly setns
> to ipc namespace of 45524 via nsfd 33
> # # pidfd_setns_test.c:444:nsfd_incremental_setns:Managed to correctly setns
> to net namespace of 45524 via nsfd 36
> # # pidfd_setns_test.c:444:nsfd_incremental_setns:Managed to correctly setns
> to cgroup namespace of 45524 via nsfd 39
> # # pidfd_setns_test.c:444:nsfd_incremental_setns:Managed to correctly setns
> to pid_for_children namespace of 45524 via nsfd 42
> # # pidfd_setns_test.c:427:nsfd_incremental_setns:Expected
> setns(self->child_nsfds1[i], info->flag) (-1) == 0 (0)
> # # pidfd_setns_test.c:428:nsfd_incremental_setns:Too many users - Failed to
> setns to time namespace of 45524 via nsfd 45
> # # nsfd_incremental_setns: Test terminated by timeout
> # # FAIL current_nsset.nsfd_incremental_setns
> # not ok 5 current_nsset.nsfd_incremental_setns
> # # RUN
kselftest/next kselftest-livepatch: 2 runs, 1 regressions (v6.9-rc4-32-g693fe2f6a9ea)
kselftest/next kselftest-livepatch: 2 runs, 1 regressions (v6.9-rc4-32-g693fe2f6a9ea) Regressions Summary --- platform| arch | lab | compiler | defconfig | regressions +--+---+--+--+ imx6q-sabrelite | arm | lab-collabora | gcc-10 | multi_v7_defconfig+kselftest | 1 Details: https://kernelci.org/test/job/kselftest/branch/next/kernel/v6.9-rc4-32-g693fe2f6a9ea/plan/kselftest-livepatch/ Test: kselftest-livepatch Tree: kselftest Branch: next Describe: v6.9-rc4-32-g693fe2f6a9ea URL: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git SHA: 693fe2f6a9ea17e4241e5114f54c6ae7bc2512d3 Test Regressions platform| arch | lab | compiler | defconfig | regressions +--+---+--+--+ imx6q-sabrelite | arm | lab-collabora | gcc-10 | multi_v7_defconfig+kselftest | 1 Details: https://kernelci.org/test/plan/id/662a7e871dc4e5b1fe4c42f3 Results: 1 PASS, 1 FAIL, 0 SKIP Full config: multi_v7_defconfig+kselftest Compiler:gcc-10 (arm-linux-gnueabihf-gcc (Debian 10.2.1-6) 10.2.1 20210110) Plain log: https://storage.kernelci.org//kselftest/next/v6.9-rc4-32-g693fe2f6a9ea/arm/multi_v7_defconfig+kselftest/gcc-10/lab-collabora/kselftest-livepatch-imx6q-sabrelite.txt HTML log: https://storage.kernelci.org//kselftest/next/v6.9-rc4-32-g693fe2f6a9ea/arm/multi_v7_defconfig+kselftest/gcc-10/lab-collabora/kselftest-livepatch-imx6q-sabrelite.html Rootfs: http://storage.kernelci.org/images/rootfs/debian/bookworm-kselftest/20240313.0/armhf/initrd.cpio.gz * kselftest-livepatch.shardfile-livepatch: https://kernelci.org/test/case/id/662a7e871dc4e5b1fe4c42f5 failing since 71 days (last pass: v6.8-rc1, first fail: v6.8-rc1-32-g345e8abe4c355) 2024-04-25T16:02:14.082199 / # 2024-04-25T16:02:14.092194 2024-04-25T16:02:19.219929 / # export NFS_ROOTFS='/var/lib/lava/dispatcher/tmp/13520225/extract-nfsrootfs-37ys35mb' 2024-04-25T16:02:19.236049 export NFS_ROOTFS='/var/lib/lava/dispatcher/tmp/13520225/extract-nfsrootfs-37ys35mb' 2024-04-25T16:02:21.459762 / # export NFS_SERVER_IP='192.168.201.1' 2024-04-25T16:02:21.472128 export NFS_SERVER_IP='192.168.201.1' 2024-04-25T16:02:21.575440 / # # 2024-04-25T16:02:21.583708 # 2024-04-25T16:02:21.700517 / # export SHELL=/bin/bash 2024-04-25T16:02:21.711640 export SHELL=/bin/bash ... (149 line(s) more)
kselftest/next kselftest-lkdtm: 5 runs, 2 regressions (v6.9-rc4-32-g693fe2f6a9ea)
kselftest/next kselftest-lkdtm: 5 runs, 2 regressions (v6.9-rc4-32-g693fe2f6a9ea) Regressions Summary --- platform| arch | lab | compiler | defconfig | regressions +---+---+--+--+ imx6q-sabrelite | arm | lab-collabora | gcc-10 | multi_v7_defconfig+kselftest | 1 mt8173-elm-hana | arm64 | lab-collabora | gcc-10 | defconfig+kse...4-chromebook | 1 Details: https://kernelci.org/test/job/kselftest/branch/next/kernel/v6.9-rc4-32-g693fe2f6a9ea/plan/kselftest-lkdtm/ Test: kselftest-lkdtm Tree: kselftest Branch: next Describe: v6.9-rc4-32-g693fe2f6a9ea URL: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git SHA: 693fe2f6a9ea17e4241e5114f54c6ae7bc2512d3 Test Regressions platform| arch | lab | compiler | defconfig | regressions +---+---+--+--+ imx6q-sabrelite | arm | lab-collabora | gcc-10 | multi_v7_defconfig+kselftest | 1 Details: https://kernelci.org/test/plan/id/662a803a6a5532d9344c42ee Results: 0 PASS, 1 FAIL, 0 SKIP Full config: multi_v7_defconfig+kselftest Compiler:gcc-10 (arm-linux-gnueabihf-gcc (Debian 10.2.1-6) 10.2.1 20210110) Plain log: https://storage.kernelci.org//kselftest/next/v6.9-rc4-32-g693fe2f6a9ea/arm/multi_v7_defconfig+kselftest/gcc-10/lab-collabora/kselftest-lkdtm-imx6q-sabrelite.txt HTML log: https://storage.kernelci.org//kselftest/next/v6.9-rc4-32-g693fe2f6a9ea/arm/multi_v7_defconfig+kselftest/gcc-10/lab-collabora/kselftest-lkdtm-imx6q-sabrelite.html Rootfs: http://storage.kernelci.org/images/rootfs/debian/bookworm-kselftest/20240313.0/armhf/initrd.cpio.gz * kselftest-lkdtm.login: https://kernelci.org/test/case/id/662a803a6a5532d9344c42ef new failure (last pass: v6.9-rc4-19-g00ab560eb0e3) platform| arch | lab | compiler | defconfig | regressions +---+---+--+--+ mt8173-elm-hana | arm64 | lab-collabora | gcc-10 | defconfig+kse...4-chromebook | 1 Details: https://kernelci.org/test/plan/id/662a8272e22aaab90f4c42da Results: 0 PASS, 1 FAIL, 0 SKIP Full config: defconfig+kselftest+arm64-chromebook Compiler:gcc-10 (aarch64-linux-gnu-gcc (Debian 10.2.1-6) 10.2.1 20210110) Plain log: https://storage.kernelci.org//kselftest/next/v6.9-rc4-32-g693fe2f6a9ea/arm64/defconfig+kselftest+arm64-chromebook/gcc-10/lab-collabora/kselftest-lkdtm-mt8173-elm-hana.txt HTML log: https://storage.kernelci.org//kselftest/next/v6.9-rc4-32-g693fe2f6a9ea/arm64/defconfig+kselftest+arm64-chromebook/gcc-10/lab-collabora/kselftest-lkdtm-mt8173-elm-hana.html Rootfs: http://storage.kernelci.org/images/rootfs/debian/bookworm-kselftest/20240313.0/arm64/initrd.cpio.gz * kselftest-lkdtm.login: https://kernelci.org/test/case/id/662a8272e22aaab90f4c42db failing since 555 days (last pass: linux-kselftest-next-6.0-rc2-11-g144eeb2fc761, first fail: v6.1-rc1)
Re: [PATCH v3] selftests/bpf: Add ring_buffer__consume_n test.
On Thu, Apr 25, 2024 at 04:06:27PM +0200, Andrea Righi wrote:
> Add a testcase for the ring_buffer__consume_n() API.
>
> The test produces multiple samples in a ring buffer, using a
> sys_getpid() fentry prog, and consumes them from user-space in batches,
> rather than consuming all of them greedily, like ring_buffer__consume()
> does.
>
> Link:
> https://lore.kernel.org/lkml/CAEf4BzaR4zqUpDmj44KNLdpJ=tpa97grvzuzvno5nm6b7ow...@mail.gmail.com
> Signed-off-by: Andrea Righi
Acked-by: Jiri Olsa
jirka
> ---
> tools/testing/selftests/bpf/Makefile | 2 +-
> .../selftests/bpf/prog_tests/ringbuf.c| 64 +++
> .../selftests/bpf/progs/test_ringbuf_n.c | 47 ++
> 3 files changed, 112 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_n.c
>
> ChangeLog v2 -> v3:
> - move skel_n inside ringbuf_n_subtest()
>
> ChangeLog v1 -> v2:
> - replace CHECK() with ASSERT_EQ()
> - fix skel -> skel_n
> - drop unused "seq" field from struct sample
>
> diff --git a/tools/testing/selftests/bpf/Makefile
> b/tools/testing/selftests/bpf/Makefile
> index edc73f8f5aef..6332277edeca 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -455,7 +455,7 @@ LINKED_SKELS := test_static_linked.skel.h
> linked_funcs.skel.h \
> LSKELS := fentry_test.c fexit_test.c fexit_sleep.c atomics.c
> \
> trace_printk.c trace_vprintk.c map_ptr_kern.c \
> core_kern.c core_kern_overflow.c test_ringbuf.c \
> - test_ringbuf_map_key.c
> + test_ringbuf_n.c test_ringbuf_map_key.c
>
> # Generate both light skeleton and libbpf skeleton for these
> LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test.c \
> diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> index 48c5695b7abf..2f064d6952f0 100644
> --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> @@ -13,6 +13,7 @@
> #include
> #include
> #include "test_ringbuf.lskel.h"
> +#include "test_ringbuf_n.lskel.h"
> #include "test_ringbuf_map_key.lskel.h"
>
> #define EDONE
> @@ -326,6 +327,67 @@ static void ringbuf_subtest(void)
> test_ringbuf_lskel__destroy(skel);
> }
>
> +/*
> + * Test ring_buffer__consume_n() by producing N_TOT_SAMPLES samples in the
> ring
> + * buffer, via getpid(), and consuming them in chunks of N_SAMPLES.
> + */
> +#define N_TOT_SAMPLES32
> +#define N_SAMPLES4
> +
> +/* Sample value to verify the callback validity */
> +#define SAMPLE_VALUE 42L
> +
> +static int process_n_sample(void *ctx, void *data, size_t len)
> +{
> + struct sample *s = data;
> +
> + ASSERT_EQ(s->value, SAMPLE_VALUE, "sample_value");
> +
> + return 0;
> +}
> +
> +static void ringbuf_n_subtest(void)
> +{
> + struct test_ringbuf_n_lskel *skel_n;
> + int err, i;
> +
> + skel_n = test_ringbuf_n_lskel__open();
> + if (!ASSERT_OK_PTR(skel_n, "test_ringbuf_n_lskel__open"))
> + return;
> +
> + skel_n->maps.ringbuf.max_entries = getpagesize();
> + skel_n->bss->pid = getpid();
> +
> + err = test_ringbuf_n_lskel__load(skel_n);
> + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__load"))
> + goto cleanup;
> +
> + ringbuf = ring_buffer__new(skel_n->maps.ringbuf.map_fd,
> +process_n_sample, NULL, NULL);
> + if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
> + goto cleanup;
> +
> + err = test_ringbuf_n_lskel__attach(skel_n);
> + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__attach"))
> + goto cleanup_ringbuf;
> +
> + /* Produce N_TOT_SAMPLES samples in the ring buffer by calling getpid()
> */
> + skel_n->bss->value = SAMPLE_VALUE;
> + for (i = 0; i < N_TOT_SAMPLES; i++)
> + syscall(__NR_getpgid);
> +
> + /* Consume all samples from the ring buffer in batches of N_SAMPLES */
> + for (i = 0; i < N_TOT_SAMPLES; i += err) {
> + err = ring_buffer__consume_n(ringbuf, N_SAMPLES);
> + ASSERT_EQ(err, N_SAMPLES, "rb_consume");
> + }
> +
> +cleanup_ringbuf:
> + ring_buffer__free(ringbuf);
> +cleanup:
> + test_ringbuf_n_lskel__destroy(skel_n);
> +}
> +
> static int process_map_key_sample(void *ctx, void *data, size_t len)
> {
> struct sample *s;
> @@ -384,6 +446,8 @@ void test_ringbuf(void)
> {
> if (test__start_subtest("ringbuf"))
> ringbuf_subtest();
> + if (test__start_subtest("ringbuf_n"))
> + ringbuf_n_subtest();
> if (test__start_subtest("ringbuf_map_key"))
> ringbuf_map_key_subtest();
> }
> diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
> b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
> new file mode 100644
> index ..8669eb42dbe0
>
Re: [PATCH 0/2] tools/nolibc: implement strtol() and friends
Hi again Thomas, On Thu, Apr 25, 2024 at 06:09:25PM +0200, Thomas Weißschuh wrote: > I wanted to implement sscanf() for ksft_min_kernel_version() and this is > a prerequisite for it. > > It's also useful on its own so it gets its own submission. > > Signed-off-by: Thomas Weißschuh Nice work, thank you. For the whole series, modulo my small comments on 2/2: Acked-by: Willy Tarreau Cheers, willy
Re: [PATCH 2/2] tools/nolibc: implement strtol() and friends
Hi Thomas,
On Thu, Apr 25, 2024 at 06:09:27PM +0200, Thomas Weißschuh wrote:
> The implementation always works on uintmax_t values.
>
> This is inefficient when only 32bit are needed.
> However for all functions this only happens for strtol() on 32bit
> platforms.
That's indeed very useful! I think there's two small bugs below where
the second one hides the first one:
> +static __attribute__((unused))
> +uintmax_t __strtox(const char *nptr, char **endptr, int base, intmax_t
> lower_limit, uintmax_t upper_limit)
> +{
> + const char signed_ = lower_limit != 0;
> + unsigned char neg = 0, overflow = 0;
> + uintmax_t val = 0, limit, old_val;
> + char c;
> +
> + if (base < 0 || base > 35) {
^
should be 36 otherwise you won't support [0-9a-z].
> + SET_ERRNO(EINVAL);
> + goto out;
> + }
(...)
> + if (c > base)
> + goto out;
This should be "c >= base" otherwise 'z' is accepted in base 35 for
example. I think it could be useful to add one more test covering base
36 to make sure all chars pass ?
> + if (endptr)
> + *endptr = (char *)nptr;
> + return (neg ? -1 : 1) * val;
I just checked to see what the compiler does on this and quite frequently
it emits a multiply while the other approach involving only a negation is
always at least as short:
return neg ? -val : val;
E.g. here's the test code:
long fct1(long neg, long val)
{
return (neg ? -1 : 1) * val;
}
long fct2(long neg, long val)
{
return neg ? -val : val;
}
- on x86_64 with gcc-13.2 -Os:
:
0: f7 df neg%edi
2: 48 19 c0sbb%rax,%rax
5: 48 83 c8 01 or $0x1,%rax
9: 48 0f af c6 imul %rsi,%rax
d: c3 ret
000e :
e: 48 89 f0mov%rsi,%rax
11: 85 ff test %edi,%edi
13: 74 03 je 18
15: 48 f7 d8neg%rax
18: c3 ret
- on riscv64 with 13.2 -Os:
:
0: c509beqza0,a
2: 557dli a0,-1
4: 02b50533mul a0,a0,a1
8: 8082ret
a: 4505li a0,1
c: bfe5j 4
000e :
e: c119beqza0,14
10: 40b005b3neg a1,a1
14: 852emv a0,a1
16: 8082ret
So IMHO it would be better to go the simpler way even if these are just a
few bytes (and possibly ones less mul on some slow archs).
Thanks!
Willy
Re: [PATCH bpf-next v3 07/11] bpf: Fix a false rejection caused by AND operation
On 4/24/24 7:42 PM, Xu Kuohai wrote:
On 4/25/2024 6:06 AM, Yonghong Song wrote:
On 4/23/24 7:25 PM, Xu Kuohai wrote:
On 4/24/2024 5:55 AM, Yonghong Song wrote:
On 4/20/24 1:33 AM, Xu Kuohai wrote:
On 4/20/2024 7:00 AM, Eduard Zingerman wrote:
On Thu, 2024-04-11 at 20:27 +0800, Xu Kuohai wrote:
From: Xu Kuohai
With lsm return value check, the no-alu32 version
test_libbpf_get_fd_by_id_opts
is rejected by the verifier, and the log says:
0: R1=ctx() R10=fp0
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t
fmode) @ test_libbpf_get_fd_by_id_opts.c:27
0: (b7) r0 = 0 ; R0_w=0
1: (79) r2 = *(u64 *)(r1 +0)
func 'bpf_lsm_bpf_map' arg0 has btf_id 916 type STRUCT 'bpf_map'
2: R1=ctx() R2_w=trusted_ptr_bpf_map()
; if (map != (struct bpf_map *)_input) @
test_libbpf_get_fd_by_id_opts.c:29
2: (18) r3 = 0x9742c0951a00 ;
R3_w=map_ptr(map=data_input,ks=4,vs=4)
4: (5d) if r2 != r3 goto pc+4 ;
R2_w=trusted_ptr_bpf_map() R3_w=map_ptr(map=data_input,ks=4,vs=4)
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t
fmode) @ test_libbpf_get_fd_by_id_opts.c:27
5: (79) r0 = *(u64 *)(r1 +8) ; R0_w=scalar() R1=ctx()
; if (fmode & FMODE_WRITE) @ test_libbpf_get_fd_by_id_opts.c:32
6: (67) r0 <<= 62 ;
R0_w=scalar(smax=0x4000,umax=0xc000,smin32=0,smax32=umax32=0,var_off=(0x0;
0xc000))
7: (c7) r0 s>>= 63 ;
R0_w=scalar(smin=smin32=-1,smax=smax32=0)
; @ test_libbpf_get_fd_by_id_opts.c:0
8: (57) r0 &= -13 ;
R0_w=scalar(smax=0x7ff3,umax=0xfff3,smax32=0x7ff3,umax32=0xfff3,var_off=(0x0;
0xfff3))
; int BPF_PROG(check_access, struct bpf_map *map, fmode_t
fmode) @ test_libbpf_get_fd_by_id_opts.c:27
9: (95) exit
And here is the C code of the prog.
SEC("lsm/bpf_map")
int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode)
{
if (map != (struct bpf_map *)_input)
return 0;
if (fmode & FMODE_WRITE)
return -EACCES;
return 0;
}
It is clear that the prog can only return either 0 or -EACCESS,
and both
values are legal.
So why is it rejected by the verifier?
The verifier log shows that the second if and return value setting
statements in the prog is optimized to bitwise operations "r0
s>>= 63"
and "r0 &= -13". The verifier correctly deduces that the the
value of
r0 is in the range [-1, 0] after verifing instruction "r0 s>>= 63".
But when the verifier proceeds to verify instruction "r0 &=
-13", it
fails to deduce the correct value range of r0.
7: (c7) r0 s>>= 63 ;
R0_w=scalar(smin=smin32=-1,smax=smax32=0)
8: (57) r0 &= -13 ;
R0_w=scalar(smax=0x7ff3,umax=0xfff3,smax32=0x7ff3,umax32=0xfff3,var_off=(0x0;
0xfff3))
So why the verifier fails to deduce the result of 'r0 &= -13'?
The verifier uses tnum to track values, and the two ranges "[-1,
0]" and
"[0, -1ULL]" are encoded to the same tnum. When verifing
instruction
"r0 &= -13", the verifier erroneously deduces the result from
"[0, -1ULL] AND -13", which is out of the expected return range
[-4095, 0].
To fix it, this patch simply adds a special SCALAR32 case for the
verifier. That is, when the source operand of the AND
instruction is
a constant and the destination operand changes from negative to
non-negative and falls in range [-256, 256], deduce the result
range
by enumerating all possible AND results.
Signed-off-by: Xu Kuohai
---
Hello,
Sorry for the delay, I had to think about this issue a bit.
I found the clang transformation that generates the pattern this
patch
tries to handle.
It is located in DAGCombiner::SimplifySelectCC() method (see [1]).
The transformation happens as a part of DAG to DAG rewrites
(LLVM uses several internal representations:
- generic optimizer uses LLVM IR, most of the work is done
using this representation;
- before instruction selection IR is converted to Selection DAG,
some optimizations are applied at this stage,
all such optimizations are a set of pattern replacements;
- Selection DAG is converted to machine code, some optimizations
are applied at the machine code level).
Full pattern is described as follows:
// fold (select_cc seteq (and x, y), 0, 0, A) -> (and (sra
(shl x)) A)
// where y is has a single bit set.
// A plaintext description would be, we can turn the SELECT_CC
into an AND
// when the condition can be materialized as an all-ones
register. Any
// single bit-test can be materialized as an all-ones register
with
// shift-left and shift-right-arith.
For this particular test case the DAG is converted as follows:
. lhs The meaning of
this select_cc is:
| .--- rhs `lhs == rhs ?
true value : false value`
Re: [PATCH] KVM: selftests: Add 'malloc' failure check in test_vmx_nested_state
On Wed, Apr 24, 2024, Oliver Upton wrote: > Hey, > > On Wed, Apr 24, 2024 at 07:51:44AM -0700, Sean Christopherson wrote: > > On Wed, Apr 24, 2024, Andrew Jones wrote: > > > On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote: > > > ... > > > > I almost wonder if we should just pick a prefix that's less obviously > > > > connected > > > > to KVM and/or selftests, but unique and short. > > > > > > > > > > How about kvmsft_ ? It's based on the ksft_ prefix of kselftest.h. Maybe > > > it's too close to ksft though and would be confusing when using both in > > > the same test? > > > > I would prefer something short, and for whatever reason I have a mental > > block > > with ksft. I always read it as "k soft", which is completely nonsensical > > :-) > > I despise brevity in tests, so my strong preference is to use some form > of 'namespaced' helper. Perhaps others have better memory than > I do, but I'm quick to forget the selftests library and find the more > verbose / obvious function names helpful for jogging my memory. Hmm, I generally agree, but in this case I think there's value in having the names *not* stand out, because they really are uninteresting and would ideally blend in. I can't envision a scenario where we don't want to assert on an OOM, i.e. there should never be a need to use a raw malloc(), and so I don't see much value in making it obvious that the call sites are doing something special. > > > I'm not a huge fan of capital letters, but we could also do something like > > > MALLOC()/CALLOC(). > > > > Hmm, I'm not usually a fan either, but that could actually work quite well > > in this > > case. It would be quite intuitive, easy to visually parse whereas > > tmalloc() vs > > malloc() kinda looks like a typo, and would more clearly communicate that > > they're > > macros. > > Ooo, don't leave me out on the bikeshedding! How about TEST_MALLOC() / > TEST_CALLOC(). It is vaguely similar to TEST_ASSERT(), which I'd hope > would give the impression that an assertion is lurking below. Yeah, but it could also give the false impression that the macro does something fancier, e.g. this makes me want to peek at TEST_MALLOC() to see what it's doing cpuid = TEST_MALLOC(kvm_cpuid2_size(nr_entries)); whereas this isn't quite enough to pique my curiosity. cpuid = MALLOC(kvm_cpuid2_size(nr_entries)); So I have a slight preference for just MALLOC()/CALLOC(), but I'm also ok with a TEST_ prefix, my brain can adapt. One of those two flavors has my vote.
kselftest/next build: 5 builds: 0 failed, 5 passed (v6.9-rc4-32-g693fe2f6a9ea)
kselftest/next build: 5 builds: 0 failed, 5 passed (v6.9-rc4-32-g693fe2f6a9ea) Full Build Summary: https://kernelci.org/build/kselftest/branch/next/kernel/v6.9-rc4-32-g693fe2f6a9ea/ Tree: kselftest Branch: next Git Describe: v6.9-rc4-32-g693fe2f6a9ea Git Commit: 693fe2f6a9ea17e4241e5114f54c6ae7bc2512d3 Git URL: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git Built: 4 unique architectures Detailed per-defconfig build reports: defconfig+kselftest (arm64, gcc-10) — PASS, 0 errors, 0 warnings, 0 section mismatches defconfig+kselftest+arm64-chromebook (arm64, gcc-10) — PASS, 0 errors, 0 warnings, 0 section mismatches i386_defconfig+kselftest (i386, gcc-10) — PASS, 0 errors, 0 warnings, 0 section mismatches multi_v7_defconfig+kselftest (arm, gcc-10) — PASS, 0 errors, 0 warnings, 0 section mismatches x86_64_defconfig+kselftest (x86_64, gcc-10) — PASS, 0 errors, 0 warnings, 0 section mismatches --- For more info write to
[PATCH 1/2] tools/nolibc: add limits for {u,}intmax_t, ulong and {u,}llong
They are useful for users and necessary for strtol() and friends. Signed-off-by: Thomas Weißschuh --- tools/include/nolibc/stdint.h | 19 +++ 1 file changed, 19 insertions(+) diff --git a/tools/include/nolibc/stdint.h b/tools/include/nolibc/stdint.h index 6665e272e213..cd79ddd6170e 100644 --- a/tools/include/nolibc/stdint.h +++ b/tools/include/nolibc/stdint.h @@ -96,6 +96,10 @@ typedef uint64_t uintmax_t; #define UINT_FAST32_MAX SIZE_MAX #define UINT_FAST64_MAX UINT64_MAX +#define INTMAX_MIN INT64_MIN +#define INTMAX_MAX INT64_MAX +#define UINTMAX_MAX UINT64_MAX + #ifndef INT_MIN #define INT_MIN (-__INT_MAX__ - 1) #endif @@ -110,4 +114,19 @@ typedef uint64_t uintmax_t; #define LONG_MAX __LONG_MAX__ #endif +#ifndef ULONG_MAX +#define ULONG_MAX ((unsigned long)(__LONG_MAX__) * 2 + 1) +#endif + +#ifndef LLONG_MIN +#define LLONG_MIN (-__LONG_LONG_MAX__ - 1) +#endif +#ifndef LLONG_MAX +#define LLONG_MAX __LONG_LONG_MAX__ +#endif + +#ifndef ULLONG_MAX +#define ULLONG_MAX ((unsigned long long)(__LONG_LONG_MAX__) * 2 + 1) +#endif + #endif /* _NOLIBC_STDINT_H */ -- 2.44.0
[PATCH 2/2] tools/nolibc: implement strtol() and friends
The implementation always works on uintmax_t values.
This is inefficient when only 32bit are needed.
However for all functions this only happens for strtol() on 32bit
platforms.
Signed-off-by: Thomas Weißschuh
---
tools/include/nolibc/stdlib.h| 109 +++
tools/testing/selftests/nolibc/nolibc-test.c | 59 +++
2 files changed, 168 insertions(+)
diff --git a/tools/include/nolibc/stdlib.h b/tools/include/nolibc/stdlib.h
index 5be9d3c7435a..f66870e25389 100644
--- a/tools/include/nolibc/stdlib.h
+++ b/tools/include/nolibc/stdlib.h
@@ -438,6 +438,115 @@ char *u64toa(uint64_t in)
return itoa_buffer;
}
+static __attribute__((unused))
+uintmax_t __strtox(const char *nptr, char **endptr, int base, intmax_t
lower_limit, uintmax_t upper_limit)
+{
+ const char signed_ = lower_limit != 0;
+ unsigned char neg = 0, overflow = 0;
+ uintmax_t val = 0, limit, old_val;
+ char c;
+
+ if (base < 0 || base > 35) {
+ SET_ERRNO(EINVAL);
+ goto out;
+ }
+
+ while (isspace(*nptr))
+ nptr++;
+
+ if (*nptr == '+') {
+ nptr++;
+ } else if (*nptr == '-') {
+ neg = 1;
+ nptr++;
+ }
+
+ if (signed_ && neg)
+ limit = -(uintmax_t)lower_limit;
+ else
+ limit = upper_limit;
+
+ if ((base == 0 || base == 16) &&
+ (strncmp(nptr, "0x", 2) == 0 || strncmp(nptr, "0X", 2) == 0)) {
+ base = 16;
+ nptr += 2;
+ } else if (base == 0 && strncmp(nptr, "0", 1) == 0) {
+ base = 8;
+ nptr += 1;
+ } else if (base == 0) {
+ base = 10;
+ }
+
+ while (*nptr) {
+ c = *nptr;
+
+ if (c >= '0' && c <= '9')
+ c -= '0';
+ else if (c >= 'a' && c <= 'z')
+ c = c - 'a' + 10;
+ else if (c >= 'A' && c <= 'Z')
+ c = c - 'A' + 10;
+ else
+ goto out;
+
+ if (c > base)
+ goto out;
+
+ nptr++;
+ old_val = val;
+ val *= base;
+ val += c;
+
+ if (val > limit || val < old_val)
+ overflow = 1;
+ }
+
+out:
+ if (overflow) {
+ SET_ERRNO(ERANGE);
+ val = limit;
+ }
+ if (endptr)
+ *endptr = (char *)nptr;
+ return (neg ? -1 : 1) * val;
+}
+
+static __attribute__((unused))
+long strtol(const char *nptr, char **endptr, int base)
+{
+ return __strtox(nptr, endptr, base, LONG_MIN, LONG_MAX);
+}
+
+static __attribute__((unused))
+unsigned long strtoul(const char *nptr, char **endptr, int base)
+{
+ return __strtox(nptr, endptr, base, 0, ULONG_MAX);
+}
+
+static __attribute__((unused))
+long long strtoll(const char *nptr, char **endptr, int base)
+{
+ return __strtox(nptr, endptr, base, LLONG_MIN, LLONG_MAX);
+}
+
+static __attribute__((unused))
+unsigned long long strtoull(const char *nptr, char **endptr, int base)
+{
+ return __strtox(nptr, endptr, base, 0, ULLONG_MAX);
+}
+
+static __attribute__((unused))
+intmax_t strtoimax(const char *nptr, char **endptr, int base)
+{
+ return __strtox(nptr, endptr, base, INTMAX_MIN, INTMAX_MAX);
+}
+
+static __attribute__((unused))
+uintmax_t strtoumax(const char *nptr, char **endptr, int base)
+{
+ return __strtox(nptr, endptr, base, 0, UINTMAX_MAX);
+}
+
/* make sure to include all global symbols */
#include "nolibc.h"
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c
b/tools/testing/selftests/nolibc/nolibc-test.c
index b9c84d42edbe..6161bd57a0c9 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -621,6 +621,51 @@ int expect_str_buf_eq(size_t expr, const char *buf, size_t
val, int llen, const
return 0;
}
+#define EXPECT_STRTOX(cond, func, input, base, expected, chars,
expected_errno)\
+ do { if (!(cond)) result(llen, SKIPPED); else ret +=
expect_strtox(llen, func, input, base, expected, chars, expected_errno); }
while (0)
+
+static __attribute__((unused))
+int expect_strtox(int llen, void *func, const char *input, int base, intmax_t
expected, int expected_chars, int expected_errno)
+{
+ char *endptr;
+ int actual_errno, actual_chars;
+ intmax_t r;
+
+ errno = 0;
+ if (func == strtol) {
+ r = strtol(input, , base);
+ } else if (func == strtoul) {
+ r = strtoul(input, , base);
+ } else {
+ result(llen, FAIL);
+ return 1;
+ }
+ actual_errno = errno;
+ actual_chars = endptr - input;
+
+ llen += printf(" %lld = %lld", (long long)expected, (long long)r);
+ if
[PATCH 0/2] tools/nolibc: implement strtol() and friends
I wanted to implement sscanf() for ksft_min_kernel_version() and this is
a prerequisite for it.
It's also useful on its own so it gets its own submission.
Signed-off-by: Thomas Weißschuh
---
Thomas Weißschuh (2):
tools/nolibc: add limits for {u,}intmax_t, ulong and {u,}llong
tools/nolibc: implement strtol() and friends
tools/include/nolibc/stdint.h| 19 +
tools/include/nolibc/stdlib.h| 109 +++
tools/testing/selftests/nolibc/nolibc-test.c | 59 +++
3 files changed, 187 insertions(+)
---
base-commit: f1652790cd374bcf98efc913ec69ed18d20e7747
change-id: 20240415-nolibc-strtol-af77a1f2c766
Best regards,
--
Thomas Weißschuh
Re: [PATCH v2 00/10] selftests: Make ksft_exit functions return void instead of int
On 4/24/24 11:24, Nathan Chancellor wrote:
Hi all,
Commit f7d5bcd35d42 ("selftests: kselftest: Mark functions that
unconditionally call exit() as __noreturn") marked functions that call
exit() as __noreturn but it did not change the return type of these
functions from 'void' to 'int' like it should have (since a noreturn
function by definition cannot return an integer because it does not
return...) because there are many tests that return the result of the
ksft_exit function, even though it has never been used due to calling
exit().
Prior to adding __noreturn, the compiler would not know that the functions
that call exit() will not return, so code like
void ksft_exit_fail(void)
{
exit(1);
}
void ksft_exit_pass(void)
{
exit(0);
}
int main(void)
{
int ret;
ret = foo();
if (ret)
ksft_exit_fail();
ksft_exit_pass();
}
would cause the compiler to complain that main() does not return an
integer, even though when ksft_exit_pass() is called, exit() will cause
the program to terminate. So ksft_exit_...() returns int to make the
compiler happy.
int ksft_exit_fail(void)
{
exit(1);
}
int ksft_exit_pass(void)
{
exit(0);
}
int main(void)
{
int ret;
ret = foo();
if (ret)
return ksft_exit_fail();
return ksft_exit_pass();
}
While this results in no warnings, it is weird semantically and it has
issues as noted in the aforementioned __noreturn change. Now that
__noreturn has been added to these functions, it is much cleaner to
change the functions to 'void' and eliminate the return statements, as
it has been made clear to the compiler that these functions terminate
the program. Drop the return before all instances of ksft_exit_...() in
a mostly mechanical way.
---
Changes in v2:
- Split series into individual patches per subsystem at Shuah's
request and CC maintainers for subsystems that have one.
- Rewrite commit messages for new patches and move previous commit
message into cover letter to high level explain all changes.
- Carry forward Thomas and Muhammad's review on patch split, as there
were no functional changes, please holler if this was inappropriate.
Thank you for carrying the reviewed by tags.
- Link to v1:
https://lore.kernel.org/r/20240417-ksft-exit-int-to-void-v1-1-eff48fdba...@kernel.org
---
Nathan Chancellor (10):
selftests/clone3: ksft_exit functions do not return
selftests/ipc: ksft_exit functions do not return
selftests: membarrier: ksft_exit_pass() does not return
selftests/mm: ksft_exit functions do not return
selftests: pidfd: ksft_exit functions do not return
selftests/resctrl: ksft_exit_skip() does not return
selftests: sync: ksft_exit_pass() does not return
selftests: timers: ksft_exit functions do not return
selftests: x86: ksft_exit_pass() does not return
selftests: kselftest: Make ksft_exit functions return void instead of int
Applied to linux-kselftest next for Linux 6.10-rc1.
thanks,
-- Shuah
[PATCH v3] selftests/bpf: Add ring_buffer__consume_n test.
Add a testcase for the ring_buffer__consume_n() API.
The test produces multiple samples in a ring buffer, using a
sys_getpid() fentry prog, and consumes them from user-space in batches,
rather than consuming all of them greedily, like ring_buffer__consume()
does.
Link:
https://lore.kernel.org/lkml/CAEf4BzaR4zqUpDmj44KNLdpJ=tpa97grvzuzvno5nm6b7ow...@mail.gmail.com
Signed-off-by: Andrea Righi
---
tools/testing/selftests/bpf/Makefile | 2 +-
.../selftests/bpf/prog_tests/ringbuf.c| 64 +++
.../selftests/bpf/progs/test_ringbuf_n.c | 47 ++
3 files changed, 112 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_n.c
ChangeLog v2 -> v3:
- move skel_n inside ringbuf_n_subtest()
ChangeLog v1 -> v2:
- replace CHECK() with ASSERT_EQ()
- fix skel -> skel_n
- drop unused "seq" field from struct sample
diff --git a/tools/testing/selftests/bpf/Makefile
b/tools/testing/selftests/bpf/Makefile
index edc73f8f5aef..6332277edeca 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -455,7 +455,7 @@ LINKED_SKELS := test_static_linked.skel.h
linked_funcs.skel.h \
LSKELS := fentry_test.c fexit_test.c fexit_sleep.c atomics.c \
trace_printk.c trace_vprintk.c map_ptr_kern.c \
core_kern.c core_kern_overflow.c test_ringbuf.c \
- test_ringbuf_map_key.c
+ test_ringbuf_n.c test_ringbuf_map_key.c
# Generate both light skeleton and libbpf skeleton for these
LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test.c \
diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
index 48c5695b7abf..2f064d6952f0 100644
--- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
+++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
@@ -13,6 +13,7 @@
#include
#include
#include "test_ringbuf.lskel.h"
+#include "test_ringbuf_n.lskel.h"
#include "test_ringbuf_map_key.lskel.h"
#define EDONE
@@ -326,6 +327,67 @@ static void ringbuf_subtest(void)
test_ringbuf_lskel__destroy(skel);
}
+/*
+ * Test ring_buffer__consume_n() by producing N_TOT_SAMPLES samples in the ring
+ * buffer, via getpid(), and consuming them in chunks of N_SAMPLES.
+ */
+#define N_TOT_SAMPLES 32
+#define N_SAMPLES 4
+
+/* Sample value to verify the callback validity */
+#define SAMPLE_VALUE 42L
+
+static int process_n_sample(void *ctx, void *data, size_t len)
+{
+ struct sample *s = data;
+
+ ASSERT_EQ(s->value, SAMPLE_VALUE, "sample_value");
+
+ return 0;
+}
+
+static void ringbuf_n_subtest(void)
+{
+ struct test_ringbuf_n_lskel *skel_n;
+ int err, i;
+
+ skel_n = test_ringbuf_n_lskel__open();
+ if (!ASSERT_OK_PTR(skel_n, "test_ringbuf_n_lskel__open"))
+ return;
+
+ skel_n->maps.ringbuf.max_entries = getpagesize();
+ skel_n->bss->pid = getpid();
+
+ err = test_ringbuf_n_lskel__load(skel_n);
+ if (!ASSERT_OK(err, "test_ringbuf_n_lskel__load"))
+ goto cleanup;
+
+ ringbuf = ring_buffer__new(skel_n->maps.ringbuf.map_fd,
+ process_n_sample, NULL, NULL);
+ if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
+ goto cleanup;
+
+ err = test_ringbuf_n_lskel__attach(skel_n);
+ if (!ASSERT_OK(err, "test_ringbuf_n_lskel__attach"))
+ goto cleanup_ringbuf;
+
+ /* Produce N_TOT_SAMPLES samples in the ring buffer by calling getpid()
*/
+ skel_n->bss->value = SAMPLE_VALUE;
+ for (i = 0; i < N_TOT_SAMPLES; i++)
+ syscall(__NR_getpgid);
+
+ /* Consume all samples from the ring buffer in batches of N_SAMPLES */
+ for (i = 0; i < N_TOT_SAMPLES; i += err) {
+ err = ring_buffer__consume_n(ringbuf, N_SAMPLES);
+ ASSERT_EQ(err, N_SAMPLES, "rb_consume");
+ }
+
+cleanup_ringbuf:
+ ring_buffer__free(ringbuf);
+cleanup:
+ test_ringbuf_n_lskel__destroy(skel_n);
+}
+
static int process_map_key_sample(void *ctx, void *data, size_t len)
{
struct sample *s;
@@ -384,6 +446,8 @@ void test_ringbuf(void)
{
if (test__start_subtest("ringbuf"))
ringbuf_subtest();
+ if (test__start_subtest("ringbuf_n"))
+ ringbuf_n_subtest();
if (test__start_subtest("ringbuf_map_key"))
ringbuf_map_key_subtest();
}
diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
new file mode 100644
index ..8669eb42dbe0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2024 Andrea Righi
+
+#include
+#include
+#include
+#include
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+#define
Re: [PATCH] selftests/bpf: Add ring_buffer__consume_n test.
On Thu, Apr 25, 2024 at 02:23:30PM +0200, Jiri Olsa wrote: > On Thu, Apr 25, 2024 at 08:19:04AM +0200, Andrea Righi wrote: > > On Sun, Apr 21, 2024 at 10:11:33PM +0200, Jiri Olsa wrote: > > ... > > > > static struct test_ringbuf_map_key_lskel *skel_map_key; > > > > +static struct test_ringbuf_n_lskel *skel_n; > > > > > > seems like there's no need for this to be static variable > > > > Can you elaborate more? I think we want these pointers to be static to > > limit the scope to this file, no? > > I meant to move it directly inside ringbuf_n_subtest function, > I don't see reason why it's defined outside of that function Oh I see! Yeah, that makes sense, I'll send a v3 soon. Thanks, -Andrea
[PATCH 3/3] selftests/bpf: drop an unused local variable
Some copy/paste leftover, this is never used
Fixes: e3d9eac99afd ("selftests/bpf: wq: add bpf_wq_init() checks")
Signed-off-by: Benjamin Tissoires
---
tools/testing/selftests/bpf/prog_tests/wq.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/wq.c
b/tools/testing/selftests/bpf/prog_tests/wq.c
index c4bacd3160e1..99e438fe12ac 100644
--- a/tools/testing/selftests/bpf/prog_tests/wq.c
+++ b/tools/testing/selftests/bpf/prog_tests/wq.c
@@ -36,7 +36,5 @@ void serial_test_wq(void)
void serial_test_failures_wq(void)
{
- LIBBPF_OPTS(bpf_test_run_opts, topts);
-
RUN_TESTS(wq_failures);
}
--
2.44.0
[PATCH 2/3] bpf: do not walk twice the hash map on free
If someone stores both a timer and a workqueue in a hash map, on free, we
would walk it twice.
Add a check in htab_free_malloced_timers_or_wq and free the timers
and workqueues if they are present.
Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps and
hashmaps")
Signed-off-by: Benjamin Tissoires
---
kernel/bpf/hashtab.c | 16 +---
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 0179183c543a..20162ae741e9 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -1515,7 +1515,7 @@ static void delete_all_elements(struct bpf_htab *htab)
migrate_enable();
}
-static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab, bool
is_timer)
+static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab)
{
int i;
@@ -1527,10 +1527,10 @@ static void htab_free_malloced_timers_or_wq(struct
bpf_htab *htab, bool is_timer
hlist_nulls_for_each_entry(l, n, head, hash_node) {
/* We only free timer on uref dropping to zero */
- if (is_timer)
+ if (btf_record_has_field(htab->map.record, BPF_TIMER))
bpf_obj_free_timer(htab->map.record,
l->key +
round_up(htab->map.key_size, 8));
- else
+ if (btf_record_has_field(htab->map.record,
BPF_WORKQUEUE))
bpf_obj_free_workqueue(htab->map.record,
l->key +
round_up(htab->map.key_size, 8));
}
@@ -1544,18 +1544,12 @@ static void htab_map_free_timers_and_wq(struct bpf_map
*map)
struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
/* We only free timer and workqueue on uref dropping to zero */
- if (btf_record_has_field(htab->map.record, BPF_TIMER)) {
+ if (btf_record_has_field(htab->map.record, BPF_TIMER | BPF_WORKQUEUE)) {
if (!htab_is_prealloc(htab))
- htab_free_malloced_timers_or_wq(htab, true);
+ htab_free_malloced_timers_or_wq(htab);
else
htab_free_prealloced_timers(htab);
}
- if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE)) {
- if (!htab_is_prealloc(htab))
- htab_free_malloced_timers_or_wq(htab, false);
- else
- htab_free_prealloced_wq(htab);
- }
}
/* Called when map->refcnt goes to zero, either from workqueue or from syscall
*/
--
2.44.0
[PATCH 1/3] bpf: do not walk twice the map on free
If someone stores both a timer and a workqueue in a map, on free we
would walk it twice.
Add a check in array_map_free_timers_wq and free the timers
and workqueues if they are present.
Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps and
hashmaps")
Signed-off-by: Benjamin Tissoires
---
kernel/bpf/arraymap.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 580d07b15471..feabc0193852 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -436,13 +436,14 @@ static void array_map_free_timers_wq(struct bpf_map *map)
/* We don't reset or free fields other than timer and workqueue
* on uref dropping to zero.
*/
- if (btf_record_has_field(map->record, BPF_TIMER))
- for (i = 0; i < array->map.max_entries; i++)
- bpf_obj_free_timer(map->record,
array_map_elem_ptr(array, i));
-
- if (btf_record_has_field(map->record, BPF_WORKQUEUE))
- for (i = 0; i < array->map.max_entries; i++)
- bpf_obj_free_workqueue(map->record,
array_map_elem_ptr(array, i));
+ if (btf_record_has_field(map->record, BPF_TIMER | BPF_WORKQUEUE)) {
+ for (i = 0; i < array->map.max_entries; i++) {
+ if (btf_record_has_field(map->record, BPF_TIMER))
+ bpf_obj_free_timer(map->record,
array_map_elem_ptr(array, i));
+ if (btf_record_has_field(map->record, BPF_WORKQUEUE))
+ bpf_obj_free_workqueue(map->record,
array_map_elem_ptr(array, i));
+ }
+ }
}
/* Called when map->refcnt goes to zero, either from workqueue or from syscall
*/
--
2.44.0
[PATCH 0/3] bpf_wq followup series
Few patches that should have been there from day 1. Anyway, they are coming now. Signed-off-by: Benjamin Tissoires --- Benjamin Tissoires (3): bpf: do not walk twice the map on free bpf: do not walk twice the hash map on free selftests/bpf: drop an unused local variable kernel/bpf/arraymap.c | 15 --- kernel/bpf/hashtab.c| 16 +--- tools/testing/selftests/bpf/prog_tests/wq.c | 2 -- 3 files changed, 13 insertions(+), 20 deletions(-) --- base-commit: 52578f7f53ff8fe3a8f6f3bc8b5956615c07a16e change-id: 20240425-bpf-next-2114350587e3 Best regards, -- Benjamin Tissoires
Re: [PATCH v2] selftests/bpf: Add ring_buffer__consume_n test.
On Thu, Apr 25, 2024 at 09:33:19AM +0200, Andrea Righi wrote:
> Add a testcase for the ring_buffer__consume_n() API.
>
> The test produces multiple samples in a ring buffer, using a
> sys_getpid() fentry prog, and consumes them from user-space in batches,
> rather than consuming all of them greedily, like ring_buffer__consume()
> does.
>
> Link:
> https://lore.kernel.org/lkml/CAEf4BzaR4zqUpDmj44KNLdpJ=tpa97grvzuzvno5nm6b7ow...@mail.gmail.com
> Signed-off-by: Andrea Righi
> ---
> tools/testing/selftests/bpf/Makefile | 2 +-
> .../selftests/bpf/prog_tests/ringbuf.c| 64 +++
> .../selftests/bpf/progs/test_ringbuf_n.c | 47 ++
> 3 files changed, 112 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_n.c
>
> ChangeLog v1 -> v2:
> - replace CHECK() with ASSERT_EQ()
> - fix skel -> skel_n
> - drop unused "seq" field from struct sample
>
> diff --git a/tools/testing/selftests/bpf/Makefile
> b/tools/testing/selftests/bpf/Makefile
> index edc73f8f5aef..6332277edeca 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -455,7 +455,7 @@ LINKED_SKELS := test_static_linked.skel.h
> linked_funcs.skel.h \
> LSKELS := fentry_test.c fexit_test.c fexit_sleep.c atomics.c
> \
> trace_printk.c trace_vprintk.c map_ptr_kern.c \
> core_kern.c core_kern_overflow.c test_ringbuf.c \
> - test_ringbuf_map_key.c
> + test_ringbuf_n.c test_ringbuf_map_key.c
>
> # Generate both light skeleton and libbpf skeleton for these
> LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test.c \
> diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> index 48c5695b7abf..d59500d13a41 100644
> --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> @@ -13,6 +13,7 @@
> #include
> #include
> #include "test_ringbuf.lskel.h"
> +#include "test_ringbuf_n.lskel.h"
> #include "test_ringbuf_map_key.lskel.h"
>
> #define EDONE
> @@ -60,6 +61,7 @@ static int process_sample(void *ctx, void *data, size_t len)
> }
>
> static struct test_ringbuf_map_key_lskel *skel_map_key;
> +static struct test_ringbuf_n_lskel *skel_n;
nit, as I wrote in the other email, I think this could be put directly
in the ringbuf_n_subtest function, other than that lgtm
Acked-by: Jiri Olsa
jirka
> static struct test_ringbuf_lskel *skel;
> static struct ring_buffer *ringbuf;
>
> @@ -326,6 +328,66 @@ static void ringbuf_subtest(void)
> test_ringbuf_lskel__destroy(skel);
> }
>
> +/*
> + * Test ring_buffer__consume_n() by producing N_TOT_SAMPLES samples in the
> ring
> + * buffer, via getpid(), and consuming them in chunks of N_SAMPLES.
> + */
> +#define N_TOT_SAMPLES32
> +#define N_SAMPLES4
> +
> +/* Sample value to verify the callback validity */
> +#define SAMPLE_VALUE 42L
> +
> +static int process_n_sample(void *ctx, void *data, size_t len)
> +{
> + struct sample *s = data;
> +
> + ASSERT_EQ(s->value, SAMPLE_VALUE, "sample_value");
> +
> + return 0;
> +}
> +
> +static void ringbuf_n_subtest(void)
> +{
> + int err, i;
> +
> + skel_n = test_ringbuf_n_lskel__open();
> + if (!ASSERT_OK_PTR(skel_n, "test_ringbuf_n_lskel__open"))
> + return;
> +
> + skel_n->maps.ringbuf.max_entries = getpagesize();
> + skel_n->bss->pid = getpid();
> +
> + err = test_ringbuf_n_lskel__load(skel_n);
> + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__load"))
> + goto cleanup;
> +
> + ringbuf = ring_buffer__new(skel_n->maps.ringbuf.map_fd,
> +process_n_sample, NULL, NULL);
> + if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
> + goto cleanup;
> +
> + err = test_ringbuf_n_lskel__attach(skel_n);
> + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__attach"))
> + goto cleanup_ringbuf;
> +
> + /* Produce N_TOT_SAMPLES samples in the ring buffer by calling getpid()
> */
> + skel_n->bss->value = SAMPLE_VALUE;
> + for (i = 0; i < N_TOT_SAMPLES; i++)
> + syscall(__NR_getpgid);
> +
> + /* Consume all samples from the ring buffer in batches of N_SAMPLES */
> + for (i = 0; i < N_TOT_SAMPLES; i += err) {
> + err = ring_buffer__consume_n(ringbuf, N_SAMPLES);
> + ASSERT_EQ(err, N_SAMPLES, "rb_consume");
> + }
> +
> +cleanup_ringbuf:
> + ring_buffer__free(ringbuf);
> +cleanup:
> + test_ringbuf_n_lskel__destroy(skel_n);
> +}
> +
> static int process_map_key_sample(void *ctx, void *data, size_t len)
> {
> struct sample *s;
> @@ -384,6 +446,8 @@ void test_ringbuf(void)
> {
> if (test__start_subtest("ringbuf"))
> ringbuf_subtest();
> + if (test__start_subtest("ringbuf_n"))
> +
Re: [PATCH] selftests/bpf: Add ring_buffer__consume_n test.
On Thu, Apr 25, 2024 at 08:19:04AM +0200, Andrea Righi wrote:
> On Sun, Apr 21, 2024 at 10:11:33PM +0200, Jiri Olsa wrote:
> ...
> > > static struct test_ringbuf_map_key_lskel *skel_map_key;
> > > +static struct test_ringbuf_n_lskel *skel_n;
> >
> > seems like there's no need for this to be static variable
>
> Can you elaborate more? I think we want these pointers to be static to
> limit the scope to this file, no?
I meant to move it directly inside ringbuf_n_subtest function,
I don't see reason why it's defined outside of that function
jirka
>
> >
> > > static struct test_ringbuf_lskel *skel;
> > > static struct ring_buffer *ringbuf;
> > >
> > > @@ -326,6 +328,67 @@ static void ringbuf_subtest(void)
> > > test_ringbuf_lskel__destroy(skel);
> > > }
> > >
> > > +/*
> > > + * Test ring_buffer__consume_n() by producing N_TOT_SAMPLES samples in
> > > the ring
> > > + * buffer, via getpid(), and consuming them in chunks of N_SAMPLES.
> > > + */
> > > +#define N_TOT_SAMPLES32
> > > +#define N_SAMPLES4
> > > +
> > > +/* Sample value to verify the callback validity */
> > > +#define SAMPLE_VALUE 42L
> > > +
> > > +static int process_n_sample(void *ctx, void *data, size_t len)
> > > +{
> > > + struct sample *s = data;
> > > +
> > > + CHECK(s->value != SAMPLE_VALUE,
> > > + "sample_value", "exp %ld, got %ld\n", SAMPLE_VALUE, s->value);
> >
> > I think we should use ASSERT macros instead in the new code
>
> Good catch, I'll change this to an ASSERT_EQ().
>
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void ringbuf_n_subtest(void)
> > > +{
> > > + int err, i;
> > > +
> > > + skel_n = test_ringbuf_n_lskel__open();
> > > + if (!ASSERT_OK_PTR(skel_n, "test_ringbuf_n_lskel__open"))
> > > + return;
> > > +
> > > + skel_n->maps.ringbuf.max_entries = getpagesize();
> > > + skel_n->bss->pid = getpid();
> > > +
> > > + err = test_ringbuf_n_lskel__load(skel_n);
> > > + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__load"))
> > > + goto cleanup;
> > > +
> > > + ringbuf = ring_buffer__new(skel_n->maps.ringbuf.map_fd,
> > > +process_n_sample, NULL, NULL);
> > > + if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
> > > + goto cleanup;
> > > +
> > > + err = test_ringbuf_n_lskel__attach(skel_n);
> > > + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__attach"))
> > > + goto cleanup_ringbuf;
> > > +
> > > + /* Produce N_TOT_SAMPLES samples in the ring buffer by calling getpid()
> > > */
> > > + skel->bss->value = SAMPLE_VALUE;
> >
> > skel_n ?
>
> Absolutely... I'm suprised that it works actually, I guess pure luck
> (unluck) to reuse the old pointer and have value mapped to the same
> location. Anyway, I'll fix this.
>
> >
> > > + for (i = 0; i < N_TOT_SAMPLES; i++)
> > > + syscall(__NR_getpgid);
> > > +
> > > + /* Consume all samples from the ring buffer in batches of N_SAMPLES */
> > > + for (i = 0; i < N_TOT_SAMPLES; i += err) {
> > > + err = ring_buffer__consume_n(ringbuf, N_SAMPLES);
> > > + ASSERT_EQ(err, N_SAMPLES, "rb_consume");
> > > + }
> > > +
> >
> > SNIP
> >
> > > diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
> > > b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
> > > new file mode 100644
> > > index ..b98b5bb20699
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
> > > @@ -0,0 +1,52 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (c) 2024 Andrea Righi
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include "bpf_misc.h"
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > +
> > > +#define TASK_COMM_LEN 16
> > > +
> > > +struct sample {
> > > + int pid;
> > > + int seq;
> >
> > seq does not seem to be checked, is it needed?
>
> seq is not used at all, I can definitely drop it.
>
> Thanks for the review! I'll send a v2.
>
> -Andrea
Re: [PATCH bpf-next v2 12/16] selftests/bpf: wq: add bpf_wq_init() checks
On Apr 24 2024, Andrii Nakryiko wrote:
> On Sat, Apr 20, 2024 at 2:10 AM Benjamin Tissoires wrote:
> >
> > Allows to test if allocation/free works
> >
> > Signed-off-by: Benjamin Tissoires
> >
> > ---
> >
> > changes in v2:
> > - dropped mark_precise checks
> > ---
> > tools/testing/selftests/bpf/bpf_experimental.h | 1 +
> > tools/testing/selftests/bpf/prog_tests/wq.c | 8 +++
> > tools/testing/selftests/bpf/progs/wq.c | 10
> > tools/testing/selftests/bpf/progs/wq_failures.c | 78
> > +
> > 4 files changed, 97 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/bpf_experimental.h
> > b/tools/testing/selftests/bpf/bpf_experimental.h
> > index 3329ea080865..785b91b629be 100644
> > --- a/tools/testing/selftests/bpf/bpf_experimental.h
> > +++ b/tools/testing/selftests/bpf/bpf_experimental.h
> > @@ -470,4 +470,5 @@ extern int bpf_iter_css_new(struct bpf_iter_css *it,
> > extern struct cgroup_subsys_state *bpf_iter_css_next(struct bpf_iter_css
> > *it) __weak __ksym;
> > extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak __ksym;
> >
> > +extern int bpf_wq_init(struct bpf_wq *wq, void *p__map, unsigned int
> > flags) __weak __ksym;
> > #endif
> > diff --git a/tools/testing/selftests/bpf/prog_tests/wq.c
> > b/tools/testing/selftests/bpf/prog_tests/wq.c
> > index 9a07b8bc2c52..26ab69796103 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/wq.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/wq.c
> > @@ -2,6 +2,7 @@
> > /* Copyright (c) 2024 Benjamin Tissoires */
> > #include
> > #include "wq.skel.h"
> > +#include "wq_failures.skel.h"
> >
> > void serial_test_wq(void)
> > {
> > @@ -9,3 +10,10 @@ void serial_test_wq(void)
> >
> > RUN_TESTS(wq);
> > }
> > +
> > +void serial_test_failures_wq(void)
> > +{
> > + LIBBPF_OPTS(bpf_test_run_opts, topts);
> > +
>
> unused leftover?
Oops, yeah. Looks like it.
I'll send a removal of it while fixing 9/16
Cheers,
Benjamin
>
> > + RUN_TESTS(wq_failures);
> > +}
>
> [...]
Re: [PATCH bpf-next v2 11/16] bpf: wq: add bpf_wq_init
On Apr 24 2024, Alexei Starovoitov wrote: > On Wed, Apr 24, 2024 at 8:06 AM Alexei Starovoitov > wrote: > > > > On Tue, Apr 23, 2024 at 7:55 PM Alexei Starovoitov > > wrote: > > > > > > On Sat, Apr 20, 2024 at 2:10 AM Benjamin Tissoires > > > wrote: > > > > > > > > We need to teach the verifier about the second argument which is > > > > declared > > > > as void * but which is of type KF_ARG_PTR_TO_MAP. We could have dropped > > > > this extra case if we declared the second argument as struct bpf_map *, > > > > but that means users will have to do extra casting to have their program > > > > compile. > > > > > > > > We also need to duplicate the timer code for the checking if the map > > > > argument is matching the provided workqueue. > > > > > > > > Signed-off-by: Benjamin Tissoires > > > > > > > > --- > > > > > > > > FWIW, I still have one concern with this implementation: > > > > - bpf_wq_work() access ->prog without protection, but I think this might > > > > be racing with bpf_wq_set_callback(): if we have the following: > > > > > > > > CPU 0 CPU 1 > > > > bpf_wq_set_callback() > > > > bpf_start() > > > > bpf_wq_work(): > > > > prog = cb->prog; > > > > > > > > bpf_wq_set_callback() > > > > cb->prog = prog; > > > > bpf_prog_put(prev) > > > > rcu_assign_ptr(cb->callback_fn, > > > >callback_fn); > > > >callback = > > > > READ_ONCE(w->cb.callback_fn); > > > > > > > > As I understand callback_fn is fine, prog might be, but we clearly > > > > have an inconstency between "prog" and "callback_fn" as they can come > > > > from 2 different bpf_wq_set_callback() calls. > > > > > > > > IMO we should protect this by the async->lock, but I'm not sure if > > > > it's OK or not. > > > > > > I see the concern, but I think it's overkill. > > > Here 'prog' is used to pass it into __bpf_prog_enter_sleepable_recur() > > > to keep the standard pattern of calling into sleepable prog. > > > But it won't recurse. > > > We can open code migrate_disable,etc from there except > > > this_cpu_inc_return, > > > but it's an overkill. > > > The passed 'prog' is irrelevant. > > > If somebody tries really hard by having two progs sharing the same > > > map with bpf_wq and racing to set_callback... I can see how > > > prog won't match callback, but it won't make a difference. > > > prog is not going trigger recursion check (unless somebody > > > tries is obsessed) and not going to UAF. > > > I imagine it's possible to attach somewhere in core wq callback > > > invocation path with fentry, set_callback to the same prog, > > > and technically it's kinda sorta recursion, but different subprogs, > > > so not a safety issue. > > > The code as-is is fine. imo. > > > > After sleeping on it, I realized that the use of > > __bpf_prog_enter_sleepable_recur() here is very much incorrect :( > > The tests are passing only because we don't inc prog->active > > when we run the prog via prog_run cmd. > > Adding the following: > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > > index f6aad4ed2ab2..0732dfe22204 100644 > > --- a/net/bpf/test_run.c > > +++ b/net/bpf/test_run.c > > @@ -1514,7 +1514,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog, > > } > > > > rcu_read_lock_trace(); > > + this_cpu_inc_return(*(prog->active)); > > retval = bpf_prog_run_pin_on_cpu(prog, ctx); > > + this_cpu_dec(*(prog->active)); > > rcu_read_unlock_trace(); > > > > makes the test fail sporadically. > > Or 100% fail when the kernel is booted with 1 cpu. > > > > Could you send a quick follow up to > > replace __bpf_prog_enter_sleepable_recur() with > > rcu_read_lock_trace(); > > migrate_disable(); > > ? > > > > Or I'll do it in an hour or so. > > Considering two broken-build reports already > I've applied the following fix: > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=dc92febf7b93da5049fe177804e6b1961fcc6bd7 > > that addresses the build issue on !JIT and fixes this recursion problem. Thanks a lot for fixing this on my behalf. I was slightly puzzled by the broken-build report but really I wasn't in position to think straight yesterday (got a pretty big muscular pain in the back and had to go to the doctor to get painkillers) I haven't forgoten about the double list walk in 9/16, I'll hopefully send the fix today. Cheers, Benjamin
Re: selftests: openvswitch: Questions about possible enhancements
On Wed, Apr 24, 2024 at 05:30:00PM -0700, Jakub Kicinski wrote: > On Wed, 24 Apr 2024 17:44:05 +0100 Simon Horman wrote: > > I have recently been exercising the Open vSwitch kernel selftests, > > using vng, > > Speaking of ovs tests, we currently don't run them in CI (and suffer > related skips in pmtu.sh) because Amazon Linux doesn't have ovs > packaged and building it looks pretty hard. > > Is there an easy way to build just the CLI tooling or get a pre-built > package somewhere? > > Or perhaps you'd be willing to run the OvS tests and we can move > the part of pmtu.sh into OvS test dir? Thanks Jakub, The plot thickens. We'll look into this (Hi Aaron!).
Re: selftests: openvswitch: Questions about possible enhancements
On Wed, Apr 24, 2024 at 02:14:09PM -0400, Aaron Conole wrote:
> Simon Horman writes:
>
> > Hi Aaron, Jakub, all,
> >
> > I have recently been exercising the Open vSwitch kernel selftests,
> > using vng, something like this:
> >
> > TESTDIR="tools/testing/selftests/net/openvswitch"
> >
> > vng -v --run . --user root --cpus 2 \
> > --overlay-rwdir "$PWD" -- \
> > "modprobe openvswitch && \
> > echo \"timeout=90\" >> \"${TESTDIR}/settings\" && \
> > make -C \"$TESTDIR\" run_tests"
> >
> > And I have some observations that I'd like to ask about.
> >
> > 1. Building the kernel using the following command does not
> >build the openvswitch kernel module.
> >
> > vng -v --build \
> > --config tools/testing/selftests/net/config
> >
> >All that seems to be missing is CONFIG_OPENVSWITCH=m
> >and I am wondering what the best way of resolving this is.
> >
> >Perhaps I am doing something wrong.
> >Or perhaps tools/testing/selftests/net/openvswitch/config
> >should be created? If so, should it include (most of?) what is in
> >tools/testing/selftests/net/config, or just CONFIG_OPENVSWITCH=m?
>
> I have a series that I need to get back to fixing:
>
> https://mail.openvswitch.org/pipermail/ovs-dev/2024-February/411917.html
>
> which does include the config listed, and some of the fixes for things
> you've noted.
>
> I think it makes sense to get back to it.
Thanks Aaron,
I was hoping you might say something like that.
WRT to the config itself, as Benjamin mentioned elsewhere in this thread [1]
there is a question about how this should be handled consistently for
all selftests.
[1] https://lore.kernel.org/netdev/ZilIgbIvB04iUal2@f4/
>
> > 2. As per my example above, it seems that a modprobe openvswitch is
> >required (if openvswitch is a module).
> >
> >Again, perhaps I am doing something wrong. But if not, should this be
> >incorporated into tools/testing/selftests/net/openvswitch/openvswitch.sh
> >or otherwise automated?
> >
> > 3. I have observed that the last test fails (yesterday, but not today!),
> >because the namespace it tries to create already exists. I believe this
> >is because it is pending deletion.
> >
> >My work-around is as follows:
> >
> > ovs_add_netns_and_veths () {
> > info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
> > + for i in $(seq 10); do
> > + ovs_sbx "$1" test -e "/var/run/netns/$3" || break
> > + info "Namespace $3 still exists (attempt $i)"
> > + ovs_sbx "$1" ip netns del "$3"
> > + sleep "$i"
> > + done
> > ovs_sbx "$1" ip netns add "$3" || return 1
> > on_exit "ovs_sbx $1 ip netns del $3"
> > ovs_sbx "$1" ip link add "$4" type veth peer name "$5" || return 1
> >
> >N.B.: the "netns del" part is probably not needed,
> >but I'm not able to exercise it effectively right now.
> >
> >I am wondering if a loop like this is appropriate to add, perhaps also
> >to namespace deletion. Or if it would be appropriate to port
> >openvswitch.sh to use ./tools/testing/selftests/net/lib.sh, which I
> >believe handles this.
> >
> > 4. I am observing timeouts whith the default value of 45s.
> >Bumping this to 90s seems to help.
> >Are there any objections to a patch to bump the timeout?
Regarding points 3 and 4.
I did a bit more testing after I sent my email yesterday.
I have two test machines. It turns out, to my surprise, that one is
much slower than the other when running openvswitch.sh with vng.
I am unsure why, but that isn't really on topic. The point
is that I'm currently only seeing problems 3 and 4 manifest
on the slow machine.
I think problem 3 is probably worth solving.
But the timeout question is more subjective.
[PATCH v2] selftests/bpf: Add ring_buffer__consume_n test.
Add a testcase for the ring_buffer__consume_n() API.
The test produces multiple samples in a ring buffer, using a
sys_getpid() fentry prog, and consumes them from user-space in batches,
rather than consuming all of them greedily, like ring_buffer__consume()
does.
Link:
https://lore.kernel.org/lkml/CAEf4BzaR4zqUpDmj44KNLdpJ=tpa97grvzuzvno5nm6b7ow...@mail.gmail.com
Signed-off-by: Andrea Righi
---
tools/testing/selftests/bpf/Makefile | 2 +-
.../selftests/bpf/prog_tests/ringbuf.c| 64 +++
.../selftests/bpf/progs/test_ringbuf_n.c | 47 ++
3 files changed, 112 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/progs/test_ringbuf_n.c
ChangeLog v1 -> v2:
- replace CHECK() with ASSERT_EQ()
- fix skel -> skel_n
- drop unused "seq" field from struct sample
diff --git a/tools/testing/selftests/bpf/Makefile
b/tools/testing/selftests/bpf/Makefile
index edc73f8f5aef..6332277edeca 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -455,7 +455,7 @@ LINKED_SKELS := test_static_linked.skel.h
linked_funcs.skel.h \
LSKELS := fentry_test.c fexit_test.c fexit_sleep.c atomics.c \
trace_printk.c trace_vprintk.c map_ptr_kern.c \
core_kern.c core_kern_overflow.c test_ringbuf.c \
- test_ringbuf_map_key.c
+ test_ringbuf_n.c test_ringbuf_map_key.c
# Generate both light skeleton and libbpf skeleton for these
LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test.c \
diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
index 48c5695b7abf..d59500d13a41 100644
--- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
+++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
@@ -13,6 +13,7 @@
#include
#include
#include "test_ringbuf.lskel.h"
+#include "test_ringbuf_n.lskel.h"
#include "test_ringbuf_map_key.lskel.h"
#define EDONE
@@ -60,6 +61,7 @@ static int process_sample(void *ctx, void *data, size_t len)
}
static struct test_ringbuf_map_key_lskel *skel_map_key;
+static struct test_ringbuf_n_lskel *skel_n;
static struct test_ringbuf_lskel *skel;
static struct ring_buffer *ringbuf;
@@ -326,6 +328,66 @@ static void ringbuf_subtest(void)
test_ringbuf_lskel__destroy(skel);
}
+/*
+ * Test ring_buffer__consume_n() by producing N_TOT_SAMPLES samples in the ring
+ * buffer, via getpid(), and consuming them in chunks of N_SAMPLES.
+ */
+#define N_TOT_SAMPLES 32
+#define N_SAMPLES 4
+
+/* Sample value to verify the callback validity */
+#define SAMPLE_VALUE 42L
+
+static int process_n_sample(void *ctx, void *data, size_t len)
+{
+ struct sample *s = data;
+
+ ASSERT_EQ(s->value, SAMPLE_VALUE, "sample_value");
+
+ return 0;
+}
+
+static void ringbuf_n_subtest(void)
+{
+ int err, i;
+
+ skel_n = test_ringbuf_n_lskel__open();
+ if (!ASSERT_OK_PTR(skel_n, "test_ringbuf_n_lskel__open"))
+ return;
+
+ skel_n->maps.ringbuf.max_entries = getpagesize();
+ skel_n->bss->pid = getpid();
+
+ err = test_ringbuf_n_lskel__load(skel_n);
+ if (!ASSERT_OK(err, "test_ringbuf_n_lskel__load"))
+ goto cleanup;
+
+ ringbuf = ring_buffer__new(skel_n->maps.ringbuf.map_fd,
+ process_n_sample, NULL, NULL);
+ if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
+ goto cleanup;
+
+ err = test_ringbuf_n_lskel__attach(skel_n);
+ if (!ASSERT_OK(err, "test_ringbuf_n_lskel__attach"))
+ goto cleanup_ringbuf;
+
+ /* Produce N_TOT_SAMPLES samples in the ring buffer by calling getpid()
*/
+ skel_n->bss->value = SAMPLE_VALUE;
+ for (i = 0; i < N_TOT_SAMPLES; i++)
+ syscall(__NR_getpgid);
+
+ /* Consume all samples from the ring buffer in batches of N_SAMPLES */
+ for (i = 0; i < N_TOT_SAMPLES; i += err) {
+ err = ring_buffer__consume_n(ringbuf, N_SAMPLES);
+ ASSERT_EQ(err, N_SAMPLES, "rb_consume");
+ }
+
+cleanup_ringbuf:
+ ring_buffer__free(ringbuf);
+cleanup:
+ test_ringbuf_n_lskel__destroy(skel_n);
+}
+
static int process_map_key_sample(void *ctx, void *data, size_t len)
{
struct sample *s;
@@ -384,6 +446,8 @@ void test_ringbuf(void)
{
if (test__start_subtest("ringbuf"))
ringbuf_subtest();
+ if (test__start_subtest("ringbuf_n"))
+ ringbuf_n_subtest();
if (test__start_subtest("ringbuf_map_key"))
ringbuf_map_key_subtest();
}
diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
new file mode 100644
index ..8669eb42dbe0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+//
Re: [ovs-dev] selftests: openvswitch: Questions about possible enhancements
On Wed, Apr 24, 2024 at 01:59:29PM -0400, Benjamin Poirier wrote:
> On 2024-04-24 18:37 +0100, Simon Horman wrote:
> > On Wed, Apr 24, 2024 at 05:44:05PM +0100, Simon Horman wrote:
> > > Hi Aaron, Jakub, all,
> > >
> > > I have recently been exercising the Open vSwitch kernel selftests,
> > > using vng, something like this:
> > >
> > > TESTDIR="tools/testing/selftests/net/openvswitch"
> > >
> > > vng -v --run . --user root --cpus 2 \
> > > --overlay-rwdir "$PWD" -- \
> > > "modprobe openvswitch && \
> > >echo \"timeout=90\" >> \"${TESTDIR}/settings\" && \
> > > make -C \"$TESTDIR\" run_tests"
> > >
> > > And I have some observations that I'd like to ask about.
> > >
> > > 1. Building the kernel using the following command does not
> > >build the openvswitch kernel module.
> > >
> > > vng -v --build \
> > > --config tools/testing/selftests/net/config
> > >
> > >All that seems to be missing is CONFIG_OPENVSWITCH=m
> > >and I am wondering what the best way of resolving this is.
> > >
> > >Perhaps I am doing something wrong.
> > >Or perhaps tools/testing/selftests/net/openvswitch/config
> > >should be created? If so, should it include (most of?) what is in
> > >tools/testing/selftests/net/config, or just CONFIG_OPENVSWITCH=m?
>
> I noticed something similar when testing Jiri's virtio_net selftests
> patchset [1].
>
> drivers/net/virtio_net/config includes virtio options but the
> test also needs at least CONFIG_NET_VRF=y which is part of net/config.
>
> Whatever the answer to your question, all config files should be
> coherent on this matter.
Yes, agreed. That is the main reason I'm asking about this.
> [1] https://lore.kernel.org/netdev/20240424104049.3935572-1-j...@resnulli.us/
>
> [...]
> >
> > 5. openvswitch.sh starts with "#!/bin/sh".
> > But substitutions such as "${ns:0:1}0" fail if /bin/sh is dash.
> > Perhaps we should change openvswitch.sh to use bash?
>
> I think so. A similar change was done in
> c2518da8e6b0 selftests: bonding: Change script interpreter (v6.8-rc1)
Thanks, this one seems straightforward.
Re: [PATCH] selftests/bpf: Add ring_buffer__consume_n test.
On Sun, Apr 21, 2024 at 10:11:33PM +0200, Jiri Olsa wrote:
...
> > static struct test_ringbuf_map_key_lskel *skel_map_key;
> > +static struct test_ringbuf_n_lskel *skel_n;
>
> seems like there's no need for this to be static variable
Can you elaborate more? I think we want these pointers to be static to
limit the scope to this file, no?
>
> > static struct test_ringbuf_lskel *skel;
> > static struct ring_buffer *ringbuf;
> >
> > @@ -326,6 +328,67 @@ static void ringbuf_subtest(void)
> > test_ringbuf_lskel__destroy(skel);
> > }
> >
> > +/*
> > + * Test ring_buffer__consume_n() by producing N_TOT_SAMPLES samples in the
> > ring
> > + * buffer, via getpid(), and consuming them in chunks of N_SAMPLES.
> > + */
> > +#define N_TOT_SAMPLES 32
> > +#define N_SAMPLES 4
> > +
> > +/* Sample value to verify the callback validity */
> > +#define SAMPLE_VALUE 42L
> > +
> > +static int process_n_sample(void *ctx, void *data, size_t len)
> > +{
> > + struct sample *s = data;
> > +
> > + CHECK(s->value != SAMPLE_VALUE,
> > + "sample_value", "exp %ld, got %ld\n", SAMPLE_VALUE, s->value);
>
> I think we should use ASSERT macros instead in the new code
Good catch, I'll change this to an ASSERT_EQ().
>
> > +
> > + return 0;
> > +}
> > +
> > +static void ringbuf_n_subtest(void)
> > +{
> > + int err, i;
> > +
> > + skel_n = test_ringbuf_n_lskel__open();
> > + if (!ASSERT_OK_PTR(skel_n, "test_ringbuf_n_lskel__open"))
> > + return;
> > +
> > + skel_n->maps.ringbuf.max_entries = getpagesize();
> > + skel_n->bss->pid = getpid();
> > +
> > + err = test_ringbuf_n_lskel__load(skel_n);
> > + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__load"))
> > + goto cleanup;
> > +
> > + ringbuf = ring_buffer__new(skel_n->maps.ringbuf.map_fd,
> > + process_n_sample, NULL, NULL);
> > + if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
> > + goto cleanup;
> > +
> > + err = test_ringbuf_n_lskel__attach(skel_n);
> > + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__attach"))
> > + goto cleanup_ringbuf;
> > +
> > + /* Produce N_TOT_SAMPLES samples in the ring buffer by calling getpid()
> > */
> > + skel->bss->value = SAMPLE_VALUE;
>
> skel_n ?
Absolutely... I'm suprised that it works actually, I guess pure luck
(unluck) to reuse the old pointer and have value mapped to the same
location. Anyway, I'll fix this.
>
> > + for (i = 0; i < N_TOT_SAMPLES; i++)
> > + syscall(__NR_getpgid);
> > +
> > + /* Consume all samples from the ring buffer in batches of N_SAMPLES */
> > + for (i = 0; i < N_TOT_SAMPLES; i += err) {
> > + err = ring_buffer__consume_n(ringbuf, N_SAMPLES);
> > + ASSERT_EQ(err, N_SAMPLES, "rb_consume");
> > + }
> > +
>
> SNIP
>
> > diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
> > b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
> > new file mode 100644
> > index ..b98b5bb20699
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_n.c
> > @@ -0,0 +1,52 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2024 Andrea Righi
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "bpf_misc.h"
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +#define TASK_COMM_LEN 16
> > +
> > +struct sample {
> > + int pid;
> > + int seq;
>
> seq does not seem to be checked, is it needed?
seq is not used at all, I can definitely drop it.
Thanks for the review! I'll send a v2.
-Andrea
