Re: [PATCHv3 bpf-next 2/9] selftests/bpf: Move test_progs helpers to testing_helpers object

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Feb 07, 2023 at 08:38:09AM -0600, David Vernet wrote:
> On Fri, Feb 03, 2023 at 05:23:29PM +0100, Jiri Olsa wrote:
> > Moving test_progs helpers to testing_helpers object so they can be
> > used from test_verifier in following changes.
> > 
> > Also adding missing ifndef header guard to testing_helpers.h header.
> > 
> > Using stderr instead of env.stderr because un/load_bpf_testmod helpers
> > will be used outside test_progs. Also at the point of calling them
> > in test_progs the std files are not hijacked yet and stderr is the
> > same as env.stderr.
> 
> Makes sense. Possibly something to clean up at another time but given
> that we were being inconsistent with env.stdout and env.stderr in
> load_bpf_testmod() in the first place, this seems totally fine.

ok

> 
> Acked-by: David Vernet <void@manifault.com>
> 
> Left one question about kern_sync_rcu() below that need not block this
> patch series, and can be addressed in a follow-up if it's even relevant.

SNIP

> > +void unload_bpf_testmod(bool verbose)
> > +{
> > +	if (kern_sync_rcu())
> > +		fprintf(stderr, "Failed to trigger kernel-side RCU sync!\n");
> 
> I realize there's no behavior change here, but out of curiosity, do you
> know why we need a synchronize_rcu() here? In general this feels kind of
> sketchy, and like something we should just put in bpf_testmod_exit() if
> it's really needed for something in the kernel.

it's explained in here:

635599bace25 selftests/bpf: Sync RCU before unloading bpf_testmod

    If some of the subtests use module BTFs through ksyms, they will cause
    bpf_prog to take a refcount on bpf_testmod module, which will prevent it from
    successfully unloading. Module's refcnt is decremented when bpf_prog is freed,
    which generally happens in RCU callback. So we need to trigger
    syncronize_rcu() in the kernel, which can be achieved nicely with
    membarrier(MEMBARRIER_CMD_SHARED) or membarrier(MEMBARRIER_CMD_GLOBAL) syscall.
    So do that in kernel_sync_rcu() and make it available to other test inside the
    test_progs. This synchronize_rcu() is called before attempting to unload
    bpf_testmod.

jirka

> 
> > +	if (delete_module("bpf_testmod", 0)) {
> > +		if (errno == ENOENT) {
> > +			if (verbose)
> > +				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
> > +			return;
> > +		}
> > +		fprintf(stderr, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
> > +		return;
> > +	}
> > +	if (verbose)
> > +		fprintf(stdout, "Successfully unloaded bpf_testmod.ko.\n");
> > +}
> > +

SNIP