Re: [RFC v2 1/2] perf: support build BPF skeletons with perf

[Jiri Olsa <jolsa@redhat.com>]

On Tue, Dec 01, 2020 at 10:47:59PM +0000, Song Liu wrote:

SNIP

> >> # As per kernel Makefile, avoid funny character set dependencies
> >> unexport LC_ALL
> >> @@ -735,7 +737,8 @@ prepare: $(OUTPUT)PERF-VERSION-FILE $(OUTPUT)common-cmds.h archheaders $(drm_ioc
> >> 	$(x86_arch_prctl_code_array) \
> >> 	$(rename_flags_array) \
> >> 	$(arch_errno_name_array) \
> >> -	$(sync_file_range_arrays)
> >> +	$(sync_file_range_arrays) \
> >> +	bpf-skel
> > 
> > I think the 'prepare' target is misused already with other stuff,
> > there's generated bpf_counter.c dependency on util/bpf_skel/bpf_prog_profiler.skel.h
> > in util/.bpf_counter.o.cmd, that should triger the build no?
> 
> This doesn't work for me. Once bpf-skel is removed from "prepare", we hit
> compilation error before util/.bpf_counter.o.cmd is generated. 

ok, I'll check on that with your new version

SNIP

> >> +submake_extras := feature_display=0
> >> +
> >> +$(SKEL_TMP_OUT):
> >> +	$(Q)$(MKDIR) -p $@
> >> +
> >> +$(BPFTOOL): | $(SKEL_TMP_OUT)
> >> +	CFLAGS= $(MAKE) $(submake_extras) -C ../bpf/bpftool \
> >> +		OUTPUT=$(SKEL_TMP_OUT)/ bootstrap
> >> +
> >> +$(SKEL_TMP_OUT)/%.bpf.o: util/bpf_skel/%.bpf.c $(BPFOBJ) | $(SKEL_TMP_OUT)
> >> +	$(call QUIET_CLANG, $@)
> >> +	$(Q)$(CLANG) -g -O2 -target bpf	-c $(filter util/bpf_skel/%.bpf.c,$^) -o $@ && \
> >> +	$(LLVM_STRIP) -g $@
> >> +
> >> +$(SKEL_OUT)/%.skel.h: $(SKEL_TMP_OUT)/%.bpf.o | $(BPFTOOL)
> >> +	$(call QUIET_GENSKEL, $@)
> > 
> > is there a reason to use call in here? you could define QUIET_GENSKEL
> > to use $@ and use it the same way as we use the rest of QUIET_* stuf
> > in Makefile.perf
> 
> I am not following here. $(CALL QUIET_xx) was used for QUIET_CLEAN and 
> QUIET_INSTALL in Makefile.perf. What is the preferred pattern here?

right it's QUIET_CLEAN and QUIET_INSTALL because they need argument,
QUIET_GENSKEL uses $@, which is used directly in other QUIET_* macros

> 
> > 
> >> new file mode 100644
> >> index 0000000000000..5263e9e6c5d83
> >> --- /dev/null
> >> +++ b/tools/perf/util/bpf_skel/.gitignore
> >> @@ -0,0 +1,3 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only
> >> +.tmp
> >> +*.skel.h
> >> \ No newline at end of file
> >> diff --git a/tools/perf/util/bpf_skel/dummy.bpf.c b/tools/perf/util/bpf_skel/dummy.bpf.c
> >> new file mode 100644
> >> index 0000000000000..492a43a851deb
> >> --- /dev/null
> >> +++ b/tools/perf/util/bpf_skel/dummy.bpf.c
> > 
> > hum, what's the reason for dummy skeleton? it just adds
> > time to compilation no?
> 
> It is mostly to test 1/2 of the set works fine. I guess we can remove 
> it in 2/2?

so it needs to be there otherwise the compilation fails?

I'll check your new version

thanks,
jirka