Hi,
On Tue, Mar 4, 2025 at 9:04 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 3/4/2025 11:18 AM, Emil Tsalapatis wrote:
> > Hi,
> >
> > On Fri, Feb 28, 2025 at 7:56 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
> >> Hi,
> >>
> >> On 2/28/2025 8:33 AM, Emil Tsalapatis wrote:
> >>> Add a helper kfunc that sets the bitmap of a bpf_cpumask from BPF
> >>> memory.
> >>>
> >>> Signed-off-by: Emil Tsalapatis (Meta) <emil@xxxxxxxxxxxxxxx>
> >>> ---
> >>> kernel/bpf/cpumask.c | 21 +++++++++++++++++++++
> >>> 1 file changed, 21 insertions(+)
> >>>
> >>> diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
> >>> index cfa1c18e3a48..a13839b3595f 100644
> >>> --- a/kernel/bpf/cpumask.c
> >>> +++ b/kernel/bpf/cpumask.c
> >>> @@ -420,6 +420,26 @@ __bpf_kfunc u32 bpf_cpumask_weight(const struct cpumask *cpumask)
> >>> return cpumask_weight(cpumask);
> >>> }
> >>>
> >>> +/**
> >>> + * bpf_cpumask_fill() - Populate the CPU mask from the contents of
> >>> + * a BPF memory region.
> >>> + *
> >>> + * @cpumask: The cpumask being populated.
> >>> + * @src: The BPF memory holding the bit pattern.
> >>> + * @src__sz: Length of the BPF memory region in bytes.
> >>> + *
> >>> + */
> >>> +__bpf_kfunc int bpf_cpumask_fill(struct cpumask *cpumask, void *src, size_t src__sz)
> >>> +{
> >>> + /* The memory region must be large enough to populate the entire CPU mask. */
> >>> + if (src__sz < BITS_TO_BYTES(nr_cpu_ids))
> >>> + return -EACCES;
> >>> +
> >>> + bitmap_copy(cpumask_bits(cpumask), src, nr_cpu_ids);
> >> Should we use src__sz < bitmap_size(nr_cpu_ids) instead ? Because in
> >> bitmap_copy(), it assumes the size of src should be bitmap_size(nr_cpu_ids).
> > This is a great catch, thank you. Comparing with
> > BITS_TO_BYTES(nr_cpu_ids) allows byte-aligned
> > masks through, even though bitmap_copy assumes all masks are long-aligned.
>
> Er, the long-aligned assumption raises another problem. Do we need to
> make the src pointer be long-aligned because bitmap_copy() may use "*dst
> = *src" to dereference the src pointer ? Or would it be better to use
> memcpy() to copy the cpumask directly ?
I would be fine with either, IMO the former is preferable. We are
rounding up the
size of the BPF-side CPU mask to the nearest long anyway, so it makes
sense for the
memory region to be long-aligned. The alternative would make the copy
slightly slower
on machines with nr_cpu_ids <= 64, though at least for sched_ext this
function should
be rare enough that the performance impact is be minimal.
If that makes sense, I will add an alignment check and an associated selftest.
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> __bpf_kfunc_end_defs();
> >>>
> >>> BTF_KFUNCS_START(cpumask_kfunc_btf_ids)
> >>> @@ -448,6 +468,7 @@ BTF_ID_FLAGS(func, bpf_cpumask_copy, KF_RCU)
> >>> BTF_ID_FLAGS(func, bpf_cpumask_any_distribute, KF_RCU)
> >>> BTF_ID_FLAGS(func, bpf_cpumask_any_and_distribute, KF_RCU)
> >>> BTF_ID_FLAGS(func, bpf_cpumask_weight, KF_RCU)
> >>> +BTF_ID_FLAGS(func, bpf_cpumask_fill, KF_RCU)
> >>> BTF_KFUNCS_END(cpumask_kfunc_btf_ids)
> >>>
> >>> static const struct btf_kfunc_id_set cpumask_kfunc_set = {
>
