Re: [PATCH] selftests/bpf: simplify cgroup_hierarchical_stats selftest

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Sep 6, 2022 at 2:35 PM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Mon, Aug 29, 2022 at 6:50 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > On Mon, Aug 29, 2022 at 6:42 PM Hao Luo <haoluo@google.com> wrote:
> > >
> > > On Mon, Aug 29, 2022 at 6:07 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> > > >
> > > > On Mon, Aug 29, 2022 at 3:15 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> > > > >
> > > > > On Mon, Aug 29, 2022 at 1:08 PM Hao Luo <haoluo@google.com> wrote:
> > > > > >
> > > > > > On Fri, Aug 26, 2022 at 4:06 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> > > > > > >
> > > [...]
> > > > > > >
> > > > > > > -SEC("tp_btf/mm_vmscan_memcg_reclaim_begin")
> > > > > > > -int BPF_PROG(vmscan_start, int order, gfp_t gfp_flags)
> > > > > > > +SEC("fentry/cgroup_attach_task")
> > > > > >
> > > > > > Can we select an attachpoint that is more stable? It seems
> > > > > > 'cgroup_attach_task' is an internal helper function in cgroup, and its
> > > > > > signature can change. I'd prefer using those commonly used tracepoints
> > > > > > and EXPORT'ed functions. IMHO their interfaces are more stable.
> > > > > >
> > > > >
> > > > > Will try to find a more stable attach point. Thanks!
> > > >
> > > > Hey Hao,
> > > >
> > > > I couldn't find any suitable stable attach points under kernel/cgroup.
> > > > Most tracepoints are created using TRACE_CGROUP_PATH which only
> > > > invokes the tracepoint if the trace event is enabled, which I assume
> > > > is not something we can rely on. Otherwise, there is only
> > >
> > > Can we explicitly enable the cgroup_attach_task event, just for this
> > > test? If it's not easy, I am fine with using fentry.
> >
> > I see a couple of tests that read from /sys/kernel/debug/tracing, but
> > they are mostly reading event ids, I don't see any tests enabling or
> > disabling a tracing event, so I am not sure if that's an accepted
> > pattern. Also I am not sure if we can rely on tracefs being in that
> > path. Andrii, is this considered acceptable?
> >
>
> Anyone with thoughts here? Is it acceptable to explicitly enable a
> trace event in a BPF selftest to attach to a tracepoint that is only
> invoked if the trace event is enabled (e.g. cgroup_attach_task) ?
> Otherwise the test program would attach to the fentry of an internal
> function, which is more vulnerable to being changed and breaking the
> test (until someone updates the test with the new signature).
>

IMO it's fine to use fentry. If something changes about signature,
we'll detect it soon enough and adjust selftests.

Messing with global tracefs in selftests is less desirable. It will
also potentially force tests to be sequential.

> > >
> > > > trace_cgroup_setup_root() and trace_cgroup_destroy_root() which are
> > > > irrelevant here. A lot of EXPORT'ed functions are not called in the
> > > > kernel, or cannot be invoked from userspace (the test) in a
> > > > straightforward way. Even if they did, future changes to such code
> > > > paths can also change in the future, so I don't think there is really
> > > > a way to guarantee that future changes don't break the test.
> > > >
> > > > Let me know what you think.
> > > >