Date:   Tue, 24 Jan 2023 20:36:02 -0800
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     David Vernet <void@manifault.com>
Cc:     bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
        yhs@meta.com, john.fastabend@gmail.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@meta.com, tj@kernel.org,
        memxor@gmail.com
Subject: Re: [PATCH bpf-next v2 4/9] bpf: Enable cpumasks to be queried and
 used as kptrs
Message-ID: <20230125043602.gmpi54ixerelmzzx@iphone-mikan.dhcp.thefacebook.com>
References: <20230120192523.3650503-1-void@manifault.com>
 <20230120192523.3650503-5-void@manifault.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230120192523.3650503-5-void@manifault.com>
Precedence: bulk

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?