- Linux-kselftest-mirror - lists.linaro.org

[PATCH v7 0/2] selftests/resctrl: Bug fix and optimization

by Maciej Wieczor-Retman

Write_schemata() uses fprintf() to write a bitmask into a schemata file inside resctrl FS. It checks fprintf() return value but it doesn't check fclose() return value. Error codes from fprintf() such as write errors, are buffered and flushed back to the user only after fclose() is executed which means any invalid bitmask can be written into the schemata file. Rewrite write_schemata() to use syscalls instead of stdio file operations to avoid the buffering. The resctrlfs.c defines functions that interact with the resctrl FS while resctrl_val.c defines functions that perform measurements on the cache. Run_benchmark() fits logically into the second file before resctrl_val() that uses it. Move run_benchmark() from resctrlfs.c to resctrl_val.c and remove redundant part of the kernel-doc comment. Make run_benchmark() static and remove it from the header file. Patch series is based on [1] which is based on [2] which are based on kselftest next branch. Changelog v7: - Add label for non-empty schema error case to Patch 1/2. (Reinette) - Add Reinette's reviewed-by tag to Patch 1/2. Changelog v6: - Align schema_len error checking with typical snprintf format. (Reinette) - Initialize schema string for early return eventuality. (Reinette) Changelog v5: - Add Ilpo's reviewed-by tag to Patch 1/2. - Reword patch messages slightly. - Add error check to schema_len variable. Changelog v4: - Change git signature from Wieczor-Retman Maciej to Maciej Wieczor-Retman. - Rebase onto [1] which is based on [2]. (Reinette) - Add fcntl.h explicitly to provide glibc backward compatibility. (Reinette) Changelog v3: - Use snprintf() return value instead of strlen() in write_schemata(). (Ilpo) - Make run_benchmark() static and remove it from the header file. (Reinette) - Add Ilpo's reviewed-by tag to Patch 2/2. - Patch messages and cover letter rewording. Changelog v2: - Change sprintf() to snprintf() in write_schemata(). - Redo write_schemata() with syscalls instead of stdio functions. - Fix typos and missing dots in patch messages. - Branch printf attribute patch to a separate series. [v1] https://lore.kernel.org/all/cover.1692880423.git.maciej.wieczor-retman@inte… [v2] https://lore.kernel.org/all/cover.1693213468.git.maciej.wieczor-retman@inte… [v3] https://lore.kernel.org/all/cover.1693575451.git.maciej.wieczor-retman@inte… [v4] https://lore.kernel.org/all/cover.1695369120.git.maciej.wieczor-retman@inte… [v5] https://lore.kernel.org/all/cover.1695975327.git.maciej.wieczor-retman@inte… [v6] https://lore.kernel.org/all/cover.1696848653.git.maciej.wieczor-retman@inte… [1] https://lore.kernel.org/all/20231002094813.6633-1-ilpo.jarvinen@linux.intel… [2] https://lore.kernel.org/all/20230904095339.11321-1-ilpo.jarvinen@linux.inte… Maciej Wieczor-Retman (2): selftests/resctrl: Fix schemata write error check selftests/resctrl: Move run_benchmark() to a more fitting file tools/testing/selftests/resctrl/resctrl.h | 1 - tools/testing/selftests/resctrl/resctrl_val.c | 50 ++++++++++ tools/testing/selftests/resctrl/resctrlfs.c | 93 ++++++------------- 3 files changed, 76 insertions(+), 68 deletions(-) -- 2.42.0

2 years

1
2
0 0

[PATCH v6 0/2] selftests/resctrl: Bug fix and optimization

by Maciej Wieczor-Retman

Write_schemata() uses fprintf() to write a bitmask into a schemata file inside resctrl FS. It checks fprintf() return value but it doesn't check fclose() return value. Error codes from fprintf() such as write errors, are buffered and flushed back to the user only after fclose() is executed which means any invalid bitmask can be written into the schemata file. Rewrite write_schemata() to use syscalls instead of stdio file operations to avoid the buffering. The resctrlfs.c defines functions that interact with the resctrl FS while resctrl_val.c defines functions that perform measurements on the cache. Run_benchmark() fits logically into the second file before resctrl_val() that uses it. Move run_benchmark() from resctrlfs.c to resctrl_val.c and remove redundant part of the kernel-doc comment. Make run_benchmark() static and remove it from the header file. Patch series is based on [1] which is based on [2] which are based on kselftest next branch. Changelog v6: - Align schema_len error checking with typical snprintf format. (Reinette) - Initialize schema string for early return eventuality. (Reinette) Changelog v5: - Add Ilpo's reviewed-by tag to Patch 1/2. - Reword patch messages slightly. - Add error check to schema_len variable. Changelog v4: - Change git signature from Wieczor-Retman Maciej to Maciej Wieczor-Retman. - Rebase onto [1] which is based on [2]. (Reinette) - Add fcntl.h explicitly to provide glibc backward compatibility. (Reinette) Changelog v3: - Use snprintf() return value instead of strlen() in write_schemata(). (Ilpo) - Make run_benchmark() static and remove it from the header file. (Reinette) - Add Ilpo's reviewed-by tag to Patch 2/2. - Patch messages and cover letter rewording. Changelog v2: - Change sprintf() to snprintf() in write_schemata(). - Redo write_schemata() with syscalls instead of stdio functions. - Fix typos and missing dots in patch messages. - Branch printf attribute patch to a separate series. [v1] https://lore.kernel.org/all/cover.1692880423.git.maciej.wieczor-retman@inte… [v2] https://lore.kernel.org/all/cover.1693213468.git.maciej.wieczor-retman@inte… [v3] https://lore.kernel.org/all/cover.1693575451.git.maciej.wieczor-retman@inte… [v4] https://lore.kernel.org/all/cover.1695369120.git.maciej.wieczor-retman@inte… [v5] https://lore.kernel.org/all/cover.1695975327.git.maciej.wieczor-retman@inte… [1] https://lore.kernel.org/all/20231002094813.6633-1-ilpo.jarvinen@linux.intel… [2] https://lore.kernel.org/all/20230904095339.11321-1-ilpo.jarvinen@linux.inte… Maciej Wieczor-Retman (2): selftests/resctrl: Fix schemata write error check selftests/resctrl: Move run_benchmark() to a more fitting file tools/testing/selftests/resctrl/resctrl.h | 1 - tools/testing/selftests/resctrl/resctrl_val.c | 50 +++++++++++ tools/testing/selftests/resctrl/resctrlfs.c | 88 +++++-------------- 3 files changed, 73 insertions(+), 66 deletions(-) -- 2.42.0

2 years

3
4
0 0

kselftest/next kselftest-seccomp: 1 runs, 1 regressions (v6.6-rc2-18-g2531f374f922e)

by kernelci.org bot

kselftest/next kselftest-seccomp: 1 runs, 1 regressions (v6.6-rc2-18-g2531f374f922e) Regressions Summary ------------------- platform | arch | lab | compiler | defconfig | regressions -----------------------------+-------+-------------+----------+---------------------+------------ meson-gxl-s905x-libretech-cc | arm64 | lab-broonie | gcc-10 | defconfig+kselftest | 1 Details: https://kernelci.org/test/job/kselftest/branch/next/kernel/v6.6-rc2-18-g253… Test: kselftest-seccomp Tree: kselftest Branch: next Describe: v6.6-rc2-18-g2531f374f922e URL: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git SHA: 2531f374f922e77ba51f24d1aa6fa11c7f4c36b8 Test Regressions ---------------- platform | arch | lab | compiler | defconfig | regressions -----------------------------+-------+-------------+----------+---------------------+------------ meson-gxl-s905x-libretech-cc | arm64 | lab-broonie | gcc-10 | defconfig+kselftest | 1 Details: https://kernelci.org/test/plan/id/6524e454e2290a6d03efcf87 Results: 0 PASS, 1 FAIL, 0 SKIP Full config: defconfig+kselftest 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.6-rc2-18-g2531f374f922e/arm… HTML log: https://storage.kernelci.org//kselftest/next/v6.6-rc2-18-g2531f374f922e/arm… Rootfs: http://storage.kernelci.org/images/rootfs/debian/bullseye-kselftest/2023062… * kselftest-seccomp.login: https://kernelci.org/test/case/id/6524e454e2290a6d03efcf88 failing since 354 days (last pass: linux-kselftest-next-6.0-rc2-11-g144eeb2fc761, first fail: v6.1-rc1-1-gde3ee3f63400a)

2 years

1
0
0 0

kselftest/next kselftest-cpufreq: 3 runs, 2 regressions (v6.6-rc2-18-g2531f374f922e)

by kernelci.org bot

kselftest/next kselftest-cpufreq: 3 runs, 2 regressions (v6.6-rc2-18-g2531f374f922e) Regressions Summary ------------------- platform | arch | lab | compiler | defconfig | regressions -----------------------------+-------+-------------+----------+---------------------+------------ meson-gxl-s905x-libretech-cc | arm64 | lab-broonie | gcc-10 | defconfig+kselftest | 1 sun50i-a64-pine64-plus | arm64 | lab-broonie | gcc-10 | defconfig+kselftest | 1 Details: https://kernelci.org/test/job/kselftest/branch/next/kernel/v6.6-rc2-18-g253… Test: kselftest-cpufreq Tree: kselftest Branch: next Describe: v6.6-rc2-18-g2531f374f922e URL: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git SHA: 2531f374f922e77ba51f24d1aa6fa11c7f4c36b8 Test Regressions ---------------- platform | arch | lab | compiler | defconfig | regressions -----------------------------+-------+-------------+----------+---------------------+------------ meson-gxl-s905x-libretech-cc | arm64 | lab-broonie | gcc-10 | defconfig+kselftest | 1 Details: https://kernelci.org/test/plan/id/6524e59941eb7e28c5efcf2e Results: 0 PASS, 1 FAIL, 0 SKIP Full config: defconfig+kselftest 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.6-rc2-18-g2531f374f922e/arm… HTML log: https://storage.kernelci.org//kselftest/next/v6.6-rc2-18-g2531f374f922e/arm… Rootfs: http://storage.kernelci.org/images/rootfs/debian/bullseye-kselftest/2023062… * kselftest-cpufreq.login: https://kernelci.org/test/case/id/6524e59941eb7e28c5efcf2f failing since 354 days (last pass: linux-kselftest-next-6.0-rc2-11-g144eeb2fc761, first fail: v6.1-rc1-1-gde3ee3f63400a) platform | arch | lab | compiler | defconfig | regressions -----------------------------+-------+-------------+----------+---------------------+------------ sun50i-a64-pine64-plus | arm64 | lab-broonie | gcc-10 | defconfig+kselftest | 1 Details: https://kernelci.org/test/plan/id/6524e5fc49aca55061efcf00 Results: 0 PASS, 1 FAIL, 0 SKIP Full config: defconfig+kselftest 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.6-rc2-18-g2531f374f922e/arm… HTML log: https://storage.kernelci.org//kselftest/next/v6.6-rc2-18-g2531f374f922e/arm… Rootfs: http://storage.kernelci.org/images/rootfs/debian/bullseye-kselftest/2023062… * kselftest-cpufreq.login: https://kernelci.org/test/case/id/6524e5fc49aca55061efcf01 failing since 345 days (last pass: linux-kselftest-next-6.0-rc2-11-g144eeb2fc761, first fail: v6.1-rc1-8-g94fea664ae4ee)

2 years

1
0
0 0

kselftest/next kselftest-lib: 2 runs, 2 regressions (v6.6-rc2-18-g2531f374f922e)

by kernelci.org bot

kselftest/next kselftest-lib: 2 runs, 2 regressions (v6.6-rc2-18-g2531f374f922e) Regressions Summary ------------------- platform | arch | lab | compiler | defconfig | regressions -----------------------------+-------+---------------+----------+---------------------+------------ meson-g12b-a311d-khadas-vim3 | arm64 | lab-collabora | gcc-10 | defconfig+kselftest | 1 meson-gxl-s905x-libretech-cc | arm64 | lab-broonie | gcc-10 | defconfig+kselftest | 1 Details: https://kernelci.org/test/job/kselftest/branch/next/kernel/v6.6-rc2-18-g253… Test: kselftest-lib Tree: kselftest Branch: next Describe: v6.6-rc2-18-g2531f374f922e URL: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git SHA: 2531f374f922e77ba51f24d1aa6fa11c7f4c36b8 Test Regressions ---------------- platform | arch | lab | compiler | defconfig | regressions -----------------------------+-------+---------------+----------+---------------------+------------ meson-g12b-a311d-khadas-vim3 | arm64 | lab-collabora | gcc-10 | defconfig+kselftest | 1 Details: https://kernelci.org/test/plan/id/6524e1f5d00dd397beefcf88 Results: 0 PASS, 1 FAIL, 0 SKIP Full config: defconfig+kselftest 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.6-rc2-18-g2531f374f922e/arm… HTML log: https://storage.kernelci.org//kselftest/next/v6.6-rc2-18-g2531f374f922e/arm… Rootfs: http://storage.kernelci.org/images/rootfs/debian/bullseye-kselftest/2023062… * kselftest-lib.login: https://kernelci.org/test/case/id/6524e1f5d00dd397beefcf89 failing since 357 days (last pass: linux-kselftest-next-6.0-rc2-11-g144eeb2fc761, first fail: v6.1-rc1) platform | arch | lab | compiler | defconfig | regressions -----------------------------+-------+---------------+----------+---------------------+------------ meson-gxl-s905x-libretech-cc | arm64 | lab-broonie | gcc-10 | defconfig+kselftest | 1 Details: https://kernelci.org/test/plan/id/6524e54af7539cc9a9efcf22 Results: 0 PASS, 1 FAIL, 0 SKIP Full config: defconfig+kselftest 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.6-rc2-18-g2531f374f922e/arm… HTML log: https://storage.kernelci.org//kselftest/next/v6.6-rc2-18-g2531f374f922e/arm… Rootfs: http://storage.kernelci.org/images/rootfs/debian/bullseye-kselftest/2023062… * kselftest-lib.login: https://kernelci.org/test/case/id/6524e54af7539cc9a9efcf23 failing since 354 days (last pass: linux-kselftest-next-6.0-rc2-11-g144eeb2fc761, first fail: v6.1-rc1-1-gde3ee3f63400a)

2 years

1
0
0 0

[PATCH v4 0/8] Add printf attribute to kselftest functions

by Maciej Wieczor-Retman

Kselftest.h declares many variadic functions that can print some formatted message while also executing selftest logic. These declarations don't have any compiler mechanism to verify if passed arguments are valid in comparison with format specifiers used in printf() calls. Attribute addition can make debugging easier, the code more consistent and prevent mismatched or missing variables. Add a __printf() macro that validates types of variables passed to the format string. The macro is similarly used in other tools in the kernel. Add __printf() attributes to function definitions inside kselftest.h that use printing. Adding the __printf() macro exposes some mismatches in format strings across different selftests. Fix the mismatched format specifiers in multiple tests. Series is based on kselftests next branch. Changelog v4: - Fix patch 1/8 subject typo. - Add Reinette's reviewed-by tags. - Rebased onto updated kselftests next branch. Changelog v3: - Changed git signature from Wieczor-Retman Maciej to Maciej Wieczor-Retman. - Added one review tag. - Rebased onto updated kselftests next branch. Changelog v2: - Add review and fixes tags to patches. - Add two patches with mismatch fixes. - Fix missed attribute in selftests/kvm. (Andrew) - Fix previously missed issues in selftests/mm (Ilpo) [v3] https://lore.kernel.org/all/cover.1695373131.git.maciej.wieczor-retman@inte… [v2] https://lore.kernel.org/all/cover.1693829810.git.maciej.wieczor-retman@inte… [v1] https://lore.kernel.org/all/cover.1693216959.git.maciej.wieczor-retman@inte… Maciej Wieczor-Retman (8): selftests: Add printf attribute to kselftest prints selftests/cachestat: Fix print_cachestat format selftests/openat2: Fix wrong format specifier selftests/pidfd: Fix ksft print formats selftests/sigaltstack: Fix wrong format specifier selftests/kvm: Replace attribute with macro selftests/mm: Substitute attribute with a macro selftests/resctrl: Fix wrong format specifier .../selftests/cachestat/test_cachestat.c | 2 +- tools/testing/selftests/kselftest.h | 18 ++++++++++-------- .../testing/selftests/kvm/include/test_util.h | 8 ++++---- tools/testing/selftests/mm/mremap_test.c | 2 +- tools/testing/selftests/mm/pkey-helpers.h | 2 +- tools/testing/selftests/openat2/openat2_test.c | 2 +- .../selftests/pidfd/pidfd_fdinfo_test.c | 2 +- tools/testing/selftests/pidfd/pidfd_test.c | 12 ++++++------ tools/testing/selftests/resctrl/cache.c | 2 +- tools/testing/selftests/sigaltstack/sas.c | 2 +- 10 files changed, 27 insertions(+), 25 deletions(-) base-commit: f1020c687153609f246f3314db5b74821025c185 -- 2.42.0

2 years

3
10
0 0

kselftest/next build: 6 builds: 0 failed, 6 passed, 1 warning (v6.6-rc2-18-g2531f374f922e)

by kernelci.org bot

kselftest/next build: 6 builds: 0 failed, 6 passed, 1 warning (v6.6-rc2-18-g2531f374f922e) Full Build Summary: https://kernelci.org/build/kselftest/branch/next/kernel/v6.6-rc2-18-g2531f3… Tree: kselftest Branch: next Git Describe: v6.6-rc2-18-g2531f374f922e Git Commit: 2531f374f922e77ba51f24d1aa6fa11c7f4c36b8 Git URL: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git Built: 4 unique architectures Warnings Detected: arm64: arm: i386: x86_64: x86_64_defconfig+kselftest (clang-16): 1 warning Warnings summary: 1 vmlinux.o: warning: objtool: set_ftrace_ops_ro+0x23: relocation to !ENDBR: .text+0x13950b ================================================================================ 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 -------------------------------------------------------------------------------- x86_64_defconfig+kselftest (x86_64, clang-16) — PASS, 0 errors, 1 warning, 0 section mismatches Warnings: vmlinux.o: warning: objtool: set_ftrace_ops_ro+0x23: relocation to !ENDBR: .text+0x13950b --- For more info write to <info(a)kernelci.org>

2 years

1
0
0 0

[RFC 0/8] iommufd support pasid attach/replace

by Yi Liu

PASID (Process Address Space ID) is a PCIe extension to tag the DMA transactions out of a physical device, and most modern IOMMU hardware have supported PASID granular address translation. So a PASID-capable devices can be attached to multiple hwpts (a.k.a. domains), each attachment is tagged with a PASID. This series first adds a missing iommu API to replace domain for a pasid, then adds iommufd APIs for device drivers to attach/replace/detach pasid to/from hwpt per userspace's request, and adds selftest to validate the iommufd APIs. pasid attach/replace is mandatory on Intel VT-d given the PASID table locates in the physical address space hence must be managed by the kernel, both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD which allow configuring the PASID/CD table either in host physical address space or nested on top of an GPA address space. This series only add VT-d support as the minimal requirement. Complete code can be found in below link: https://github.com/yiliu1765/iommufd/tree/iommufd_pasid Regards, Yi Liu Kevin Tian (1): iommufd: Support attach/replace hwpt per pasid Lu Baolu (2): iommu: Introduce a replace API for device pasid iommu/vt-d: Add set_dev_pasid callback for nested domain Yi Liu (5): iommufd: replace attach_fn with a structure iommufd/selftest: Add set_dev_pasid and remove_dev_pasid in mock iommu iommufd/selftest: Add a helper to get test device iommufd/selftest: Add test ops to test pasid attach/detach iommufd/selftest: Add coverage for iommufd pasid attach/detach drivers/iommu/intel/nested.c | 47 +++++ drivers/iommu/iommu-priv.h | 2 + drivers/iommu/iommu.c | 73 ++++++-- drivers/iommu/iommufd/Makefile | 1 + drivers/iommu/iommufd/device.c | 42 +++-- drivers/iommu/iommufd/iommufd_private.h | 16 ++ drivers/iommu/iommufd/iommufd_test.h | 24 +++ drivers/iommu/iommufd/pasid.c | 152 ++++++++++++++++ drivers/iommu/iommufd/selftest.c | 158 ++++++++++++++-- include/linux/iommufd.h | 6 + tools/testing/selftests/iommu/iommufd.c | 172 ++++++++++++++++++ .../selftests/iommu/iommufd_fail_nth.c | 28 ++- tools/testing/selftests/iommu/iommufd_utils.h | 78 ++++++++ 13 files changed, 756 insertions(+), 43 deletions(-) create mode 100644 drivers/iommu/iommufd/pasid.c -- 2.34.1

2 years

4
20
0 0

Re: [RFC PATCH 5/7] tun: Introduce virtio-net hashing feature

by Willem de Bruijn

On Sun, Oct 8, 2023 at 12:22 AM Akihiko Odaki <akihiko.odaki(a)daynix.com> wrote: > > virtio-net have two usage of hashes: one is RSS and another is hash > reporting. Conventionally the hash calculation was done by the VMM. > However, computing the hash after the queue was chosen defeats the > purpose of RSS. > > Another approach is to use eBPF steering program. This approach has > another downside: it cannot report the calculated hash due to the > restrictive nature of eBPF. > > Introduce the code to compute hashes to the kernel in order to overcome > thse challenges. An alternative solution is to extend the eBPF steering > program so that it will be able to report to the userspace, but it makes > little sense to allow to implement different hashing algorithms with > eBPF since the hash value reported by virtio-net is strictly defined by > the specification. > > The hash value already stored in sk_buff is not used and computed > independently since it may have been computed in a way not conformant > with the specification. > > Signed-off-by: Akihiko Odaki <akihiko.odaki(a)daynix.com> > @@ -2116,31 +2172,49 @@ static ssize_t tun_put_user(struct tun_struct *tun, > } > > if (vnet_hdr_sz) { > - struct virtio_net_hdr gso; > + union { > + struct virtio_net_hdr hdr; > + struct virtio_net_hdr_v1_hash v1_hash_hdr; > + } hdr; > + int ret; > > if (iov_iter_count(iter) < vnet_hdr_sz) > return -EINVAL; > > - if (virtio_net_hdr_from_skb(skb, &gso, > - tun_is_little_endian(tun), true, > - vlan_hlen)) { > + if ((READ_ONCE(tun->vnet_hash.flags) & TUN_VNET_HASH_REPORT) && > + vnet_hdr_sz >= sizeof(hdr.v1_hash_hdr) && > + skb->tun_vnet_hash) { Isn't vnet_hdr_sz guaranteed to be >= hdr.v1_hash_hdr, by virtue of the set hash ioctl failing otherwise? Such checks should be limited to control path where possible > + vnet_hdr_content_sz = sizeof(hdr.v1_hash_hdr); > + ret = virtio_net_hdr_v1_hash_from_skb(skb, > + &hdr.v1_hash_hdr, > + true, > + vlan_hlen, > + &vnet_hash); > + } else { > + vnet_hdr_content_sz = sizeof(hdr.hdr); > + ret = virtio_net_hdr_from_skb(skb, &hdr.hdr, > + tun_is_little_endian(tun), > + true, vlan_hlen); > + } > +

2 years

3
8
0 0

Re: [RFC PATCH 5/7] tun: Introduce virtio-net hashing feature

by Michael S. Tsirkin

Akihiko Odaki sorry about reposts. Having an email with two "@" in the CC list: xuanzhuo@linux.alibaba.comshuah@kernel.org tripped up mutt's reply-all for me and made it send to you only. I am guessing you meant two addresses: xuanzhuo(a)linux.alibaba.com and shuah(a)kernel.org. On Sun, Oct 08, 2023 at 02:20:49PM +0900, Akihiko Odaki wrote: > virtio-net have two usage of hashes: one is RSS and another is hash > reporting. Conventionally the hash calculation was done by the VMM. > However, computing the hash after the queue was chosen defeats the > purpose of RSS. > > Another approach is to use eBPF steering program. This approach has > another downside: it cannot report the calculated hash due to the > restrictive nature of eBPF. > > Introduce the code to compute hashes to the kernel in order to overcome > thse challenges. An alternative solution is to extend the eBPF steering > program so that it will be able to report to the userspace, but it makes > little sense to allow to implement different hashing algorithms with > eBPF since the hash value reported by virtio-net is strictly defined by > the specification. > > The hash value already stored in sk_buff is not used and computed > independently since it may have been computed in a way not conformant > with the specification. > > Signed-off-by: Akihiko Odaki <akihiko.odaki(a)daynix.com> > --- > drivers/net/tun.c | 187 ++++++++++++++++++++++++++++++++---- > include/uapi/linux/if_tun.h | 16 +++ > 2 files changed, 182 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 89ab9efe522c..561a573cd008 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -171,6 +171,9 @@ struct tun_prog { > struct bpf_prog *prog; > }; > > +#define TUN_VNET_HASH_MAX_KEY_SIZE 40 > +#define TUN_VNET_HASH_MAX_INDIRECTION_TABLE_LENGTH 128 > + where do these come from? > /* Since the socket were moved to tun_file, to preserve the behavior of persist > * device, socket filter, sndbuf and vnet header size were restore when the > * file were attached to a persist device. > @@ -209,6 +212,9 @@ struct tun_struct { > struct tun_prog __rcu *steering_prog; > struct tun_prog __rcu *filter_prog; > struct ethtool_link_ksettings link_ksettings; > + struct tun_vnet_hash vnet_hash; > + u16 vnet_hash_indirection_table[TUN_VNET_HASH_MAX_INDIRECTION_TABLE_LENGTH]; > + u32 vnet_hash_key[TUN_VNET_HASH_MAX_KEY_SIZE / 4]; That's quite a lot of data to add in this struct, and will be used by a small minority of users. Are you pushing any hot data out of cache with this? Why not allocate these as needed? > /* init args */ > struct file *file; > struct ifreq *ifr; > @@ -219,6 +225,13 @@ struct veth { > __be16 h_vlan_TCI; > }; > > +static const struct tun_vnet_hash_cap tun_vnet_hash_cap = { > + .max_indirection_table_length = > + TUN_VNET_HASH_MAX_INDIRECTION_TABLE_LENGTH, > + > + .types = VIRTIO_NET_SUPPORTED_HASH_TYPES > +}; > + > static void tun_flow_init(struct tun_struct *tun); > static void tun_flow_uninit(struct tun_struct *tun); > > @@ -320,10 +333,16 @@ static long tun_set_vnet_be(struct tun_struct *tun, int __user *argp) > if (get_user(be, argp)) > return -EFAULT; > > - if (be) > + if (be) { > + if (!(tun->flags & TUN_VNET_LE) && > + (tun->vnet_hash.flags & TUN_VNET_HASH_REPORT)) { > + return -EINVAL; > + } > + > tun->flags |= TUN_VNET_BE; > - else > + } else { > tun->flags &= ~TUN_VNET_BE; > + } > > return 0; > } > @@ -558,15 +577,47 @@ static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb) > return ret % numqueues; > } > > +static u16 tun_vnet_select_queue(struct tun_struct *tun, struct sk_buff *skb) > +{ > + u32 value = qdisc_skb_cb(skb)->tun_vnet_hash_value; > + u32 numqueues; > + u32 index; > + u16 queue; > + > + numqueues = READ_ONCE(tun->numqueues); > + if (!numqueues) > + return 0; > + > + index = value & READ_ONCE(tun->vnet_hash.indirection_table_mask); > + queue = READ_ONCE(tun->vnet_hash_indirection_table[index]); > + if (!queue) > + queue = READ_ONCE(tun->vnet_hash.unclassified_queue); Apparently 0 is an illegal queue value? You are making this part of UAPI better document things like this. > + > + return queue % numqueues; > +} > + > static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb, > struct net_device *sb_dev) > { > struct tun_struct *tun = netdev_priv(dev); > + u8 vnet_hash_flags = READ_ONCE(tun->vnet_hash.flags); > + struct virtio_net_hash hash; > u16 ret; > > + if (vnet_hash_flags & (TUN_VNET_HASH_RSS | TUN_VNET_HASH_REPORT)) { > + virtio_net_hash(skb, READ_ONCE(tun->vnet_hash.types), > + tun->vnet_hash_key, &hash); > + What are all these READ_ONCE things doing? E.g. you seem to be very content to have tun->vnet_hash.types read twice apparently? This is volatile which basically breaks all compiler's attempts to optimize code - needs to be used judiciously. > + skb->tun_vnet_hash = true; > + qdisc_skb_cb(skb)->tun_vnet_hash_value = hash.value; > + qdisc_skb_cb(skb)->tun_vnet_hash_report = hash.report; > + } > + > rcu_read_lock(); > if (rcu_dereference(tun->steering_prog)) > ret = tun_ebpf_select_queue(tun, skb); > + else if (vnet_hash_flags & TUN_VNET_HASH_RSS) > + ret = tun_vnet_select_queue(tun, skb); > else > ret = tun_automq_select_queue(tun, skb); > rcu_read_unlock(); > @@ -2088,10 +2139,15 @@ static ssize_t tun_put_user(struct tun_struct *tun, > struct iov_iter *iter) > { > struct tun_pi pi = { 0, skb->protocol }; > + struct virtio_net_hash vnet_hash = { > + .value = qdisc_skb_cb(skb)->tun_vnet_hash_value, > + .report = qdisc_skb_cb(skb)->tun_vnet_hash_report > + }; > ssize_t total; > int vlan_offset = 0; > int vlan_hlen = 0; > int vnet_hdr_sz = 0; > + size_t vnet_hdr_content_sz; > > if (skb_vlan_tag_present(skb)) > vlan_hlen = VLAN_HLEN; > @@ -2116,31 +2172,49 @@ static ssize_t tun_put_user(struct tun_struct *tun, > } > > if (vnet_hdr_sz) { > - struct virtio_net_hdr gso; > + union { > + struct virtio_net_hdr hdr; > + struct virtio_net_hdr_v1_hash v1_hash_hdr; > + } hdr; > + int ret; > > if (iov_iter_count(iter) < vnet_hdr_sz) > return -EINVAL; > > - if (virtio_net_hdr_from_skb(skb, &gso, > - tun_is_little_endian(tun), true, > - vlan_hlen)) { > + if ((READ_ONCE(tun->vnet_hash.flags) & TUN_VNET_HASH_REPORT) && > + vnet_hdr_sz >= sizeof(hdr.v1_hash_hdr) && > + skb->tun_vnet_hash) { > + vnet_hdr_content_sz = sizeof(hdr.v1_hash_hdr); > + ret = virtio_net_hdr_v1_hash_from_skb(skb, > + &hdr.v1_hash_hdr, > + true, > + vlan_hlen, > + &vnet_hash); > + } else { > + vnet_hdr_content_sz = sizeof(hdr.hdr); > + ret = virtio_net_hdr_from_skb(skb, &hdr.hdr, > + tun_is_little_endian(tun), > + true, vlan_hlen); > + } > + > + if (ret) { > struct skb_shared_info *sinfo = skb_shinfo(skb); > pr_err("unexpected GSO type: " > "0x%x, gso_size %d, hdr_len %d\n", > - sinfo->gso_type, tun16_to_cpu(tun, gso.gso_size), > - tun16_to_cpu(tun, gso.hdr_len)); > + sinfo->gso_type, tun16_to_cpu(tun, hdr.hdr.gso_size), > + tun16_to_cpu(tun, hdr.hdr.hdr_len)); > print_hex_dump(KERN_ERR, "tun: ", > DUMP_PREFIX_NONE, > 16, 1, skb->head, > - min((int)tun16_to_cpu(tun, gso.hdr_len), 64), true); > + min((int)tun16_to_cpu(tun, hdr.hdr.hdr_len), 64), true); > WARN_ON_ONCE(1); > return -EINVAL; > } > > - if (copy_to_iter(&gso, sizeof(gso), iter) != sizeof(gso)) > + if (copy_to_iter(&hdr, vnet_hdr_content_sz, iter) != vnet_hdr_content_sz) > return -EFAULT; > > - iov_iter_advance(iter, vnet_hdr_sz - sizeof(gso)); > + iov_iter_advance(iter, vnet_hdr_sz - vnet_hdr_content_sz); > } > > if (vlan_hlen) { > @@ -3007,24 +3081,27 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) > return ret; > } > > -static int tun_set_ebpf(struct tun_struct *tun, struct tun_prog __rcu **prog_p, > - void __user *data) > +static struct bpf_prog *tun_set_ebpf(struct tun_struct *tun, > + struct tun_prog __rcu **prog_p, > + void __user *data) > { > struct bpf_prog *prog; > int fd; > + int ret; > > if (copy_from_user(&fd, data, sizeof(fd))) > - return -EFAULT; > + return ERR_PTR(-EFAULT); > > if (fd == -1) { > prog = NULL; > } else { > prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_FILTER); > if (IS_ERR(prog)) > - return PTR_ERR(prog); > + return prog; > } > > - return __tun_set_ebpf(tun, prog_p, prog); > + ret = __tun_set_ebpf(tun, prog_p, prog); > + return ret ? ERR_PTR(ret) : prog; > } > > /* Return correct value for tun->dev->addr_len based on tun->dev->type. */ > @@ -3082,6 +3159,11 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > int le; > int ret; > bool do_notify = false; > + struct bpf_prog *bpf_ret; > + struct tun_vnet_hash vnet_hash; > + u16 vnet_hash_indirection_table[TUN_VNET_HASH_MAX_INDIRECTION_TABLE_LENGTH]; > + u8 vnet_hash_key[TUN_VNET_HASH_MAX_KEY_SIZE]; > + size_t len; > > if (cmd == TUNSETIFF || cmd == TUNSETQUEUE || > (_IOC_TYPE(cmd) == SOCK_IOC_TYPE && cmd != SIOCGSKNS)) { > @@ -3295,7 +3377,10 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > ret = -EFAULT; > break; > } > - if (vnet_hdr_sz < (int)sizeof(struct virtio_net_hdr)) { > + if (vnet_hdr_sz < > + (int)((tun->vnet_hash.flags & TUN_VNET_HASH_REPORT) ? > + sizeof(struct virtio_net_hdr_v1_hash) : > + sizeof(struct virtio_net_hdr))) { > ret = -EINVAL; > break; > } > @@ -3314,10 +3399,16 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > ret = -EFAULT; > break; > } > - if (le) > + if (le) { > tun->flags |= TUN_VNET_LE; > - else > + } else { > + if (!tun_legacy_is_little_endian(tun)) { > + ret = -EINVAL; > + break; > + } > + > tun->flags &= ~TUN_VNET_LE; > + } > break; > > case TUNGETVNETBE: > @@ -3360,11 +3451,17 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > break; > > case TUNSETSTEERINGEBPF: > - ret = tun_set_ebpf(tun, &tun->steering_prog, argp); > + bpf_ret = tun_set_ebpf(tun, &tun->steering_prog, argp); > + if (IS_ERR(bpf_ret)) > + ret = PTR_ERR(bpf_ret); > + else if (bpf_ret) > + tun->vnet_hash.flags &= ~TUN_VNET_HASH_RSS; what is this doing? > break; > > case TUNSETFILTEREBPF: > - ret = tun_set_ebpf(tun, &tun->filter_prog, argp); > + bpf_ret = tun_set_ebpf(tun, &tun->filter_prog, argp); > + if (IS_ERR(bpf_ret)) > + ret = PTR_ERR(bpf_ret); > break; > > case TUNSETCARRIER: > @@ -3382,6 +3479,54 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd, > ret = open_related_ns(&net->ns, get_net_ns); > break; > > + case TUNGETVNETHASHCAP: > + if (copy_to_user(argp, &tun_vnet_hash_cap, > + sizeof(tun_vnet_hash_cap))) > + ret = -EFAULT; > + break; > + > + case TUNSETVNETHASH: > + len = sizeof(vnet_hash); > + if (copy_from_user(&vnet_hash, argp, len)) { > + ret = -EFAULT; > + break; > + } what if flags has some bits set you don't know how to handle? should these be ignored as now or cause a failure? these decisions all affect uapi. > + > + if (((vnet_hash.flags & TUN_VNET_HASH_REPORT) && > + (tun->vnet_hdr_sz < sizeof(struct virtio_net_hdr_v1_hash) || > + !tun_is_little_endian(tun))) || > + vnet_hash.indirection_table_mask >= > + TUN_VNET_HASH_MAX_INDIRECTION_TABLE_LENGTH) { > + ret = -EINVAL; > + break; > + } Given this is later used to index within an array one has to be very careful about spectre things here, which this code isn't. > + > + argp = (u8 __user *)argp + len; > + len = (vnet_hash.indirection_table_mask + 1) * 2; comment pointer math tricks like this extensively please. > + if (copy_from_user(vnet_hash_indirection_table, argp, len)) { > + ret = -EFAULT; > + break; > + } > + > + argp = (u8 __user *)argp + len; > + len = virtio_net_hash_key_length(vnet_hash.types); > + > + if (copy_from_user(vnet_hash_key, argp, len)) { > + ret = -EFAULT; > + break; > + } > + > + tun->vnet_hash = vnet_hash; > + memcpy(tun->vnet_hash_indirection_table, > + vnet_hash_indirection_table, > + (vnet_hash.indirection_table_mask + 1) * 2); > + memcpy(tun->vnet_hash_key, vnet_hash_key, len); > + > + if (vnet_hash.flags & TUN_VNET_HASH_RSS) > + __tun_set_ebpf(tun, &tun->steering_prog, NULL); > + > + break; > + > default: > ret = -EINVAL; > break; > diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h > index 287cdc81c939..dc591cd897c8 100644 > --- a/include/uapi/linux/if_tun.h > +++ b/include/uapi/linux/if_tun.h > @@ -61,6 +61,8 @@ > #define TUNSETFILTEREBPF _IOR('T', 225, int) > #define TUNSETCARRIER _IOW('T', 226, int) > #define TUNGETDEVNETNS _IO('T', 227) > +#define TUNGETVNETHASHCAP _IO('T', 228) > +#define TUNSETVNETHASH _IOW('T', 229, unsigned int) > > /* TUNSETIFF ifr flags */ > #define IFF_TUN 0x0001 > @@ -115,4 +117,18 @@ struct tun_filter { > __u8 addr[][ETH_ALEN]; > }; > > +struct tun_vnet_hash_cap { > + __u16 max_indirection_table_length; > + __u32 types; > +}; > + There's hidden padding in this struct - not good, copy will leak kernel info out. > +#define TUN_VNET_HASH_RSS 0x01 > +#define TUN_VNET_HASH_REPORT 0x02 Do you intend to add more flags down the road? How will userspace know what is supported? > +struct tun_vnet_hash { > + __u8 flags; > + __u32 types; > + __u16 indirection_table_mask; > + __u16 unclassified_queue; > +}; > + Padding here too. Best avoided. In any case, document UAPI please. > #endif /* _UAPI__IF_TUN_H */ > -- > 2.42.0

2 years

2
1
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

Linux-kselftest-mirror