On Wed, May 11, 2022 at 10:07:35AM IST, Alexei Starovoitov wrote:
> On Tue, May 10, 2022 at 2:17 PM Kumar Kartikeya Dwivedi
> <memxor@xxxxxxxxx> wrote:
> >
> > In an effort to actually test the refcounting logic at runtime, add a
> > refcount_t member to prog_test_ref_kfunc and use it in selftests to
> > verify and test the whole logic more exhaustively.
> >
> > To ensure reading the count to verify it remains stable, make
> > prog_test_ref_kfunc a per-CPU variable, so that inside a BPF program the
> > count can be read reliably based on number of acquisitions made. Then,
> > pairing them with releases and reading from the global per-CPU variable
> > will allow verifying whether release operations put the refcount.
>
> The patches look good, but the per-cpu part is a puzzle.
> The test is not parallel. Everything looks sequential
> and there are no races.
> It seems to me if it was
> static struct prog_test_ref_kfunc prog_test_struct = {..};
> and none of [bpf_]this_cpu_ptr()
> the test would work the same way.
> What am I missing?
You are not missing anything. It would work the same. I just made it per-CPU for
the off chance that someone runs ./test_progs -t map_kptr in parallel on the
same machine. Then one or both might fail, since count won't just be inc/dec by
us, and reading it would produce something other than what we expect.
One other benefit is getting non-ref PTR_TO_BTF_ID to prog_test_struct to
inspect cnt after releasing acquired pointer (using bpf_this_cpu_ptr), but that
can also be done by non-ref kfunc returning a pointer to it.
If you feel it's not worth it, I will drop it in the next version.
--
Kartikeya
