Message ID | 20230120192523.3650503-5-void@manifault.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | BPF |
Headers | show |
Series | Enable cpumasks to be used as kptrs | expand |
On Fri, Jan 20, 2023 at 01:25:18PM -0600, David Vernet wrote: > + > +/** > + * struct bpf_cpumask - refcounted BPF cpumask wrapper structure > + * @cpumask: The actual cpumask embedded in the struct. > + * @usage: Object reference counter. When the refcount goes to 0, the > + * memory is released back to the BPF allocator, which provides > + * RCU safety. > + * > + * Note that we explicitly embed a cpumask_t rather than a cpumask_var_t. This > + * is done to avoid confusing the verifier due to the typedef of cpumask_var_t > + * changing depending on whether CONFIG_CPUMASK_OFFSTACK is defined or not. See > + * the details in <linux/cpumask.h>. The consequence is that this structure is > + * likely a bit larger than it needs to be when CONFIG_CPUMASK_OFFSTACK is > + * defined due to embedding the whole NR_CPUS-size bitmap, but the extra memory > + * overhead is minimal. For the more typical case of CONFIG_CPUMASK_OFFSTACK > + * not being defined, the structure is the same size regardless. > + */ > +struct bpf_cpumask { > + cpumask_t cpumask; > + refcount_t usage; > +}; > + > +static struct bpf_mem_alloc bpf_cpumask_ma; > + > +static bool cpu_valid(u32 cpu) > +{ > + return cpu < nr_cpu_ids; > +} > + > +__diag_push(); > +__diag_ignore_all("-Wmissing-prototypes", > + "Global kfuncs as their definitions will be in BTF"); > + > +struct bpf_cpumask *bpf_cpumask_create(void) > +{ > + struct bpf_cpumask *cpumask; > + > + cpumask = bpf_mem_alloc(&bpf_cpumask_ma, sizeof(*cpumask)); > + if (!cpumask) > + return NULL; > + > + memset(cpumask, 0, sizeof(*cpumask)); > + refcount_set(&cpumask->usage, 1); > + > + return cpumask; > +} Applied patches 1 and 2. Patch 3 doesn't apply anymore. Pls rebase. I'm fine with existing bpf_cpumask proposal, but can we do better? This is so close to be a bitmap template. Can we generalize it as struct bpf_bitmap { refcount_t refcnt; int num_bits; u64 bits[]; }; struct bpf_bitmap *bpf_bitmap_create(int bits) { bitmap = bpf_mem_alloc(&bpf_cpumask_ma, sizeof(*bitmap) + BITS_TO_LONGS(bits) * sizeof(u64)); bitmap->num_bits = bits; } and special case few custom kfuncs in the verifier that allow type cast from bpf_bitmap with to 'struct cpumask *' ? Like struct cpumask *bpf_bitmap_cast_to_cpumask(struct bpf_bitmap *bitmap) { if (bitmap->num_bits == nr_cpu_ids) return bitmap->bits; return NULL; } BTF_ID_FLAGS(func, bpf_bitmap_cast_to_cpumask, KF_TRUSTED_ARGS | KF_RET_NULL) The UX will be a bit worse, since bpf prog would need to do !=NULL check but with future bpf_assert() we may get rid of !=NULL check. We can keep direct cpumask accessors as kfuncs: u32 bpf_cpumask_first(const struct cpumask *cpumask); u32 bpf_cpumask_first_zero(const struct cpumask *cpumask); and add bpf_find_first_bit() and the rest of bit manipulations. Since all of the bpf_cpumask do run-time cpu_valid() check we're not sacrificing performance. Feels more generic with wider applicability at the expense of little bit worse UX. I haven't thought about acq/rel consequences. wdyt?
On Tue, Jan 24, 2023 at 08:36:02PM -0800, Alexei Starovoitov wrote: > On Fri, Jan 20, 2023 at 01:25:18PM -0600, David Vernet wrote: > > + > > +/** > > + * struct bpf_cpumask - refcounted BPF cpumask wrapper structure > > + * @cpumask: The actual cpumask embedded in the struct. > > + * @usage: Object reference counter. When the refcount goes to 0, the > > + * memory is released back to the BPF allocator, which provides > > + * RCU safety. > > + * > > + * Note that we explicitly embed a cpumask_t rather than a cpumask_var_t. This > > + * is done to avoid confusing the verifier due to the typedef of cpumask_var_t > > + * changing depending on whether CONFIG_CPUMASK_OFFSTACK is defined or not. See > > + * the details in <linux/cpumask.h>. The consequence is that this structure is > > + * likely a bit larger than it needs to be when CONFIG_CPUMASK_OFFSTACK is > > + * defined due to embedding the whole NR_CPUS-size bitmap, but the extra memory > > + * overhead is minimal. For the more typical case of CONFIG_CPUMASK_OFFSTACK > > + * not being defined, the structure is the same size regardless. > > + */ > > +struct bpf_cpumask { > > + cpumask_t cpumask; > > + refcount_t usage; > > +}; > > + > > +static struct bpf_mem_alloc bpf_cpumask_ma; > > + > > +static bool cpu_valid(u32 cpu) > > +{ > > + return cpu < nr_cpu_ids; > > +} > > + > > +__diag_push(); > > +__diag_ignore_all("-Wmissing-prototypes", > > + "Global kfuncs as their definitions will be in BTF"); > > + > > +struct bpf_cpumask *bpf_cpumask_create(void) > > +{ > > + struct bpf_cpumask *cpumask; > > + > > + cpumask = bpf_mem_alloc(&bpf_cpumask_ma, sizeof(*cpumask)); > > + if (!cpumask) > > + return NULL; > > + > > + memset(cpumask, 0, sizeof(*cpumask)); > > + refcount_set(&cpumask->usage, 1); > > + > > + return cpumask; > > +} > > Applied patches 1 and 2. Patch 3 doesn't apply anymore. Pls rebase. Ack, will rebase for v3. > I'm fine with existing bpf_cpumask proposal, but can we do better? > This is so close to be a bitmap template. Agreed that they're close, but I'm not a fan of the UX taxes for what we get out of it. More below. > Can we generalize it as > struct bpf_bitmap { > refcount_t refcnt; > int num_bits; > u64 bits[]; > }; > > struct bpf_bitmap *bpf_bitmap_create(int bits) > { > bitmap = bpf_mem_alloc(&bpf_cpumask_ma, sizeof(*bitmap) + BITS_TO_LONGS(bits) * sizeof(u64)); > bitmap->num_bits = bits; > } +1 that having bitmap kfuncs would be nice to expose, and should be pretty easy to add. Happy to do so in a follow-on patch set. > > and special case few custom kfuncs in the verifier that allow > type cast from bpf_bitmap with to 'struct cpumask *' ? Like > struct cpumask *bpf_bitmap_cast_to_cpumask(struct bpf_bitmap *bitmap) > { > if (bitmap->num_bits == nr_cpu_ids) > return bitmap->bits; > return NULL; > } > BTF_ID_FLAGS(func, bpf_bitmap_cast_to_cpumask, KF_TRUSTED_ARGS | KF_RET_NULL) This I'm not a huge fan of though. It seems like we're removing a useful abstraction and adding a UX tax just to avoid defining and exporting an additional small set of kfuncs for allocating, and acquire/releasing a struct bpf_cpumask. That logic is very minimal, just around 100 lines of code including doxygen comments. It's kind of unfortunate that cpumask is so close to bitmap, but that's nothing new -- <linux/cpumask.h> in the kernel is little more than a thin wrapper around a bitmap that simply provides some more ergonomic APIs, along with some magic that makes it safe to access cpumask_var_t on the stack regardless of NR_CPUS. The latter doesn't apply to BPF, but the former does. > The UX will be a bit worse, since bpf prog would need to do !=NULL check > but with future bpf_assert() we may get rid of !=NULL check. > > We can keep direct cpumask accessors as kfuncs: > > u32 bpf_cpumask_first(const struct cpumask *cpumask); > u32 bpf_cpumask_first_zero(const struct cpumask *cpumask); > > and add bpf_find_first_bit() and the rest of bit manipulations. Worth noting as well is that I think struct bpf_bitmap is going to be treated somewhat differently than struct bpf_cpumask and struct cpumask. There is no type-safety for bitmaps in the kernel. They're just represented as unsigned long *, so I don't we'll be able to allow programs to pass bitmaps allocated elsewhere in the kernel to read-only bitmap kfuncs like we do for struct cpumask *, as the verifier will just interpret them as pointers to statically sized scalars. > Since all of the bpf_cpumask do run-time cpu_valid() check we're not > sacrificing performance. > > Feels more generic with wider applicability at the expense of little bit worse UX. > I haven't thought about acq/rel consequences. The TL;DR from me is that I agree that having bitmap kfuncs is a great idea, but I don't see the need to tie the two at the hip at the cost of a worse UX. I'd prefer to push the extra complexity into the BPF backend in favor of a simpler programming front-end for users. Thoughts?
On Tue, Jan 24, 2023 at 9:36 PM David Vernet <void@manifault.com> wrote: > > > The UX will be a bit worse, since bpf prog would need to do !=NULL check > > but with future bpf_assert() we may get rid of !=NULL check. > > > > We can keep direct cpumask accessors as kfuncs: > > > > u32 bpf_cpumask_first(const struct cpumask *cpumask); > > u32 bpf_cpumask_first_zero(const struct cpumask *cpumask); > > > > and add bpf_find_first_bit() and the rest of bit manipulations. > > Worth noting as well is that I think struct bpf_bitmap is going to be > treated somewhat differently than struct bpf_cpumask and struct cpumask. > There is no type-safety for bitmaps in the kernel. They're just > represented as unsigned long *, so I don't we'll be able to allow > programs to pass bitmaps allocated elsewhere in the kernel to read-only > bitmap kfuncs like we do for struct cpumask *, as the verifier will just > interpret them as pointers to statically sized scalars. That's a good point. That's where run-time and verification-time safety hurts UX too much. > > Since all of the bpf_cpumask do run-time cpu_valid() check we're not > > sacrificing performance. > > > > Feels more generic with wider applicability at the expense of little bit worse UX. > > I haven't thought about acq/rel consequences. > > The TL;DR from me is that I agree that having bitmap kfuncs is a great > idea, but I don't see the need to tie the two at the hip at the cost of > a worse UX. I'd prefer to push the extra complexity into the BPF backend > in favor of a simpler programming front-end for users. > > Thoughts? Fair enough. Let's proceed with what you have.
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile index 3a12e6b400a2..02242614dcc7 100644 --- a/kernel/bpf/Makefile +++ b/kernel/bpf/Makefile @@ -36,6 +36,7 @@ obj-$(CONFIG_DEBUG_INFO_BTF) += sysfs_btf.o endif ifeq ($(CONFIG_BPF_JIT),y) obj-$(CONFIG_BPF_SYSCALL) += bpf_struct_ops.o +obj-$(CONFIG_BPF_SYSCALL) += cpumask.o obj-${CONFIG_BPF_LSM} += bpf_lsm.o endif obj-$(CONFIG_BPF_PRELOAD) += preload/ diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c new file mode 100644 index 000000000000..92eedc84dbfc --- /dev/null +++ b/kernel/bpf/cpumask.c @@ -0,0 +1,269 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright (c) 2023 Meta, Inc + */ +#include <linux/bpf.h> +#include <linux/bpf_mem_alloc.h> +#include <linux/btf.h> +#include <linux/btf_ids.h> +#include <linux/cpumask.h> + +/** + * struct bpf_cpumask - refcounted BPF cpumask wrapper structure + * @cpumask: The actual cpumask embedded in the struct. + * @usage: Object reference counter. When the refcount goes to 0, the + * memory is released back to the BPF allocator, which provides + * RCU safety. + * + * Note that we explicitly embed a cpumask_t rather than a cpumask_var_t. This + * is done to avoid confusing the verifier due to the typedef of cpumask_var_t + * changing depending on whether CONFIG_CPUMASK_OFFSTACK is defined or not. See + * the details in <linux/cpumask.h>. The consequence is that this structure is + * likely a bit larger than it needs to be when CONFIG_CPUMASK_OFFSTACK is + * defined due to embedding the whole NR_CPUS-size bitmap, but the extra memory + * overhead is minimal. For the more typical case of CONFIG_CPUMASK_OFFSTACK + * not being defined, the structure is the same size regardless. + */ +struct bpf_cpumask { + cpumask_t cpumask; + refcount_t usage; +}; + +static struct bpf_mem_alloc bpf_cpumask_ma; + +static bool cpu_valid(u32 cpu) +{ + return cpu < nr_cpu_ids; +} + +__diag_push(); +__diag_ignore_all("-Wmissing-prototypes", + "Global kfuncs as their definitions will be in BTF"); + +struct bpf_cpumask *bpf_cpumask_create(void) +{ + struct bpf_cpumask *cpumask; + + cpumask = bpf_mem_alloc(&bpf_cpumask_ma, sizeof(*cpumask)); + if (!cpumask) + return NULL; + + memset(cpumask, 0, sizeof(*cpumask)); + refcount_set(&cpumask->usage, 1); + + return cpumask; +} + +struct bpf_cpumask *bpf_cpumask_acquire(struct bpf_cpumask *cpumask) +{ + refcount_inc(&cpumask->usage); + return cpumask; +} + +struct bpf_cpumask *bpf_cpumask_kptr_get(struct bpf_cpumask **cpumaskp) +{ + struct bpf_cpumask *cpumask; + + /* The BPF memory allocator frees memory backing its caches in an RCU + * callback. Thus, we can safely use RCU to ensure that the cpumask is + * safe to read. + */ + rcu_read_lock(); + + cpumask = READ_ONCE(*cpumaskp); + if (cpumask && !refcount_inc_not_zero(&cpumask->usage)) + cpumask = NULL; + + rcu_read_unlock(); + return cpumask; +} + +void bpf_cpumask_release(struct bpf_cpumask *cpumask) +{ + if (!cpumask) + return; + + if (refcount_dec_and_test(&cpumask->usage)) { + migrate_disable(); + bpf_mem_free(&bpf_cpumask_ma, cpumask); + migrate_enable(); + } +} + +u32 bpf_cpumask_first(const struct cpumask *cpumask) +{ + return cpumask_first(cpumask); +} + +u32 bpf_cpumask_first_zero(const struct cpumask *cpumask) +{ + return cpumask_first_zero(cpumask); +} + +void bpf_cpumask_set_cpu(u32 cpu, struct bpf_cpumask *cpumask) +{ + if (!cpu_valid(cpu)) + return; + + cpumask_set_cpu(cpu, (struct cpumask *)cpumask); +} + +void bpf_cpumask_clear_cpu(u32 cpu, struct bpf_cpumask *cpumask) +{ + if (!cpu_valid(cpu)) + return; + + cpumask_clear_cpu(cpu, (struct cpumask *)cpumask); +} + +bool bpf_cpumask_test_cpu(u32 cpu, const struct cpumask *cpumask) +{ + if (!cpu_valid(cpu)) + return false; + + return cpumask_test_cpu(cpu, (struct cpumask *)cpumask); +} + +bool bpf_cpumask_test_and_set_cpu(u32 cpu, struct bpf_cpumask *cpumask) +{ + if (!cpu_valid(cpu)) + return false; + + return cpumask_test_and_set_cpu(cpu, (struct cpumask *)cpumask); +} + +bool bpf_cpumask_test_and_clear_cpu(u32 cpu, struct bpf_cpumask *cpumask) +{ + if (!cpu_valid(cpu)) + return false; + + return cpumask_test_and_clear_cpu(cpu, (struct cpumask *)cpumask); +} + +void bpf_cpumask_setall(struct bpf_cpumask *cpumask) +{ + cpumask_setall((struct cpumask *)cpumask); +} + +void bpf_cpumask_clear(struct bpf_cpumask *cpumask) +{ + cpumask_clear((struct cpumask *)cpumask); +} + +bool bpf_cpumask_and(struct bpf_cpumask *dst, + const struct cpumask *src1, + const struct cpumask *src2) +{ + return cpumask_and((struct cpumask *)dst, src1, src2); +} + +void bpf_cpumask_or(struct bpf_cpumask *dst, + const struct cpumask *src1, + const struct cpumask *src2) +{ + cpumask_or((struct cpumask *)dst, src1, src2); +} + +void bpf_cpumask_xor(struct bpf_cpumask *dst, + const struct cpumask *src1, + const struct cpumask *src2) +{ + cpumask_xor((struct cpumask *)dst, src1, src2); +} + +bool bpf_cpumask_equal(const struct cpumask *src1, const struct cpumask *src2) +{ + return cpumask_equal(src1, src2); +} + +bool bpf_cpumask_intersects(const struct cpumask *src1, const struct cpumask *src2) +{ + return cpumask_intersects(src1, src2); +} + +bool bpf_cpumask_subset(const struct cpumask *src1, const struct cpumask *src2) +{ + return cpumask_subset(src1, src2); +} + +bool bpf_cpumask_empty(const struct cpumask *cpumask) +{ + return cpumask_empty(cpumask); +} + +bool bpf_cpumask_full(const struct cpumask *cpumask) +{ + return cpumask_full(cpumask); +} + +void bpf_cpumask_copy(struct bpf_cpumask *dst, const struct cpumask *src) +{ + cpumask_copy((struct cpumask *)dst, src); +} + +u32 bpf_cpumask_any(const struct cpumask *cpumask) +{ + return cpumask_any(cpumask); +} + +u32 bpf_cpumask_any_and(const struct cpumask *src1, const struct cpumask *src2) +{ + return cpumask_any_and(src1, src2); +} + +__diag_pop(); + +BTF_SET8_START(cpumask_kfunc_btf_ids) +BTF_ID_FLAGS(func, bpf_cpumask_create, KF_ACQUIRE | KF_RET_NULL) +BTF_ID_FLAGS(func, bpf_cpumask_release, KF_RELEASE | KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_acquire, KF_ACQUIRE | KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_kptr_get, KF_ACQUIRE | KF_KPTR_GET | KF_RET_NULL) +BTF_ID_FLAGS(func, bpf_cpumask_first, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_first_zero, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_set_cpu, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_clear_cpu, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_test_cpu, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_test_and_set_cpu, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_test_and_clear_cpu, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_setall, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_clear, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_and, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_or, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_xor, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_equal, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_intersects, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_subset, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_empty, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_full, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_copy, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_any, KF_TRUSTED_ARGS) +BTF_ID_FLAGS(func, bpf_cpumask_any_and, KF_TRUSTED_ARGS) +BTF_SET8_END(cpumask_kfunc_btf_ids) + +static const struct btf_kfunc_id_set cpumask_kfunc_set = { + .owner = THIS_MODULE, + .set = &cpumask_kfunc_btf_ids, +}; + +BTF_ID_LIST(cpumask_dtor_ids) +BTF_ID(struct, bpf_cpumask) +BTF_ID(func, bpf_cpumask_release) + +static int __init cpumask_kfunc_init(void) +{ + int ret; + const struct btf_id_dtor_kfunc cpumask_dtors[] = { + { + .btf_id = cpumask_dtor_ids[0], + .kfunc_btf_id = cpumask_dtor_ids[1] + }, + }; + + ret = bpf_mem_alloc_init(&bpf_cpumask_ma, 0, false); + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &cpumask_kfunc_set); + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &cpumask_kfunc_set); + return ret ?: register_btf_id_dtor_kfuncs(cpumask_dtors, + ARRAY_SIZE(cpumask_dtors), + THIS_MODULE); +} + +late_initcall(cpumask_kfunc_init);
Certain programs may wish to be able to query cpumasks. For example, if a program that is tracing percpu operations may wish to track which tasks end up running on which CPUs, and it could be useful to associate that with the tasks' cpumasks. Similarly, a program tracking NUMA allocations, CPU scheduling domains, etc, would potentially benefit from being able to see which CPUs a task could be migrated to, etc. This patch enables these such cases by introducing a series of bpf_cpumask_* kfuncs. Amongst these kfuncs, there are two separate "classes" of operations: 1. kfuncs which allow the caller to allocate and mutate their own cpumasks in the form of a struct bpf_cpumask * object. Such kfuncs include e.g. bpf_cpumask_create() to allocate the cpumask, and bpf_cpumask_or() to mutate it. "Regular" cpumasks such as p->cpus_ptr may not be passed to these kfuncs, and the verifier will ensure this is the case by comparing BTF IDs. 2. Read-only operations which operate on const struct cpumask * arguments. For example, bpf_cpumask_test_cpu(), which tests whether a CPU is set in the cpumask. Any trusted struct cpumask * or struct bpf_cpumask * may be passed to these kfuncs. The verifier allows struct bpf_cpumask * even though the kfunc is defined with struct cpumask * because the first element of a struct bpf_cpumask is a cpumask_t, so it is safe to cast. A follow-on patch will add selftests which validate these kfuncs, and another will document them. Note that some of the kfuncs that were added would benefit from additional verification logic. For example, any kfunc taking a CPU argument that exceeds the number of CPUs on the system, etc. For now, we silently check for and ignore these cases at runtime. When we have e.g. per-argument kfunc flags, it might be helpful to add another KF_CPU-type flag that specifies that the verifier should validate that it's a valid CPU. Signed-off-by: David Vernet <void@manifault.com> --- kernel/bpf/Makefile | 1 + kernel/bpf/cpumask.c | 269 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 270 insertions(+) create mode 100644 kernel/bpf/cpumask.c