diff mbox series

[v3,7/8] perf python: Switch module to linking libraries from building source

Message ID 20240613233122.3564730-8-irogers@google.com (mailing list archive)
State Not Applicable
Delegated to: BPF
Headers show
Series Refactor perf python module build | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Ian Rogers June 13, 2024, 11:31 p.m. UTC
setup.py was building most perf sources causing setup.py to mimic the
Makefile logic as well as flex/bison code to be stubbed out, due to
complexity building. By using libraries fewer functions are stubbed
out, the build is faster and the Makefile logic is reused which should
simplify updating. The libraries are passed through LDFLAGS to avoid
complexity in python.

Force the -fPIC flag for libbpf.a to ensure it is suitable for linking
into the perf python module.

Signed-off-by: Ian Rogers <irogers@google.com>
Reviewed-by: James Clark <james.clark@arm.com>
---
 tools/perf/Makefile.config |   5 +
 tools/perf/Makefile.perf   |   6 +-
 tools/perf/util/python.c   | 271 ++++++++++++++-----------------------
 tools/perf/util/setup.py   |  33 +----
 4 files changed, 110 insertions(+), 205 deletions(-)

Comments

Namhyung Kim June 24, 2024, 10:26 p.m. UTC | #1
On Thu, Jun 13, 2024 at 04:31:21PM -0700, Ian Rogers wrote:
> setup.py was building most perf sources causing setup.py to mimic the
> Makefile logic as well as flex/bison code to be stubbed out, due to
> complexity building. By using libraries fewer functions are stubbed
> out, the build is faster and the Makefile logic is reused which should
> simplify updating. The libraries are passed through LDFLAGS to avoid
> complexity in python.
> 
> Force the -fPIC flag for libbpf.a to ensure it is suitable for linking
> into the perf python module.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Reviewed-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/Makefile.config |   5 +
>  tools/perf/Makefile.perf   |   6 +-
>  tools/perf/util/python.c   | 271 ++++++++++++++-----------------------
>  tools/perf/util/setup.py   |  33 +----
>  4 files changed, 110 insertions(+), 205 deletions(-)
> 
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index 7f1e016a9253..639be696f597 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -910,6 +910,11 @@ else
>           endif
>           CFLAGS += -DHAVE_LIBPYTHON_SUPPORT
>           $(call detected,CONFIG_LIBPYTHON)
> +	 ifeq ($(filter -fPIC,$(CFLAGS)),)

Nitpick: mixed TAB and SPACEs.


> +           # Building a shared library requires position independent code.
> +           CFLAGS += -fPIC
> +           CXXFLAGS += -fPIC
> +         endif


I'm curious if it's ok for static libraries too..

Thanks,
Namhyung


>        endif
>      endif
>    endif
James Clark June 25, 2024, 12:24 p.m. UTC | #2
On 24/06/2024 23:26, Namhyung Kim wrote:
> On Thu, Jun 13, 2024 at 04:31:21PM -0700, Ian Rogers wrote:
>> setup.py was building most perf sources causing setup.py to mimic the
>> Makefile logic as well as flex/bison code to be stubbed out, due to
>> complexity building. By using libraries fewer functions are stubbed
>> out, the build is faster and the Makefile logic is reused which should
>> simplify updating. The libraries are passed through LDFLAGS to avoid
>> complexity in python.
>>
>> Force the -fPIC flag for libbpf.a to ensure it is suitable for linking
>> into the perf python module.
>>
>> Signed-off-by: Ian Rogers <irogers@google.com>
>> Reviewed-by: James Clark <james.clark@arm.com>
>> ---
>>  tools/perf/Makefile.config |   5 +
>>  tools/perf/Makefile.perf   |   6 +-
>>  tools/perf/util/python.c   | 271 ++++++++++++++-----------------------
>>  tools/perf/util/setup.py   |  33 +----
>>  4 files changed, 110 insertions(+), 205 deletions(-)
>>
>> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
>> index 7f1e016a9253..639be696f597 100644
>> --- a/tools/perf/Makefile.config
>> +++ b/tools/perf/Makefile.config
>> @@ -910,6 +910,11 @@ else
>>           endif
>>           CFLAGS += -DHAVE_LIBPYTHON_SUPPORT
>>           $(call detected,CONFIG_LIBPYTHON)
>> +	 ifeq ($(filter -fPIC,$(CFLAGS)),)
> 
> Nitpick: mixed TAB and SPACEs.
> 
> 
>> +           # Building a shared library requires position independent code.
>> +           CFLAGS += -fPIC
>> +           CXXFLAGS += -fPIC
>> +         endif
> 
> 
> I'm curious if it's ok for static libraries too..
> 
> Thanks,
> Namhyung
> 

I think I tested the whole set with a static build so it should be ok.

> 
>>        endif
>>      endif
>>    endif
Namhyung Kim June 25, 2024, 5:48 p.m. UTC | #3
Hello,

On Tue, Jun 25, 2024 at 5:24 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 24/06/2024 23:26, Namhyung Kim wrote:
> > On Thu, Jun 13, 2024 at 04:31:21PM -0700, Ian Rogers wrote:
> >> setup.py was building most perf sources causing setup.py to mimic the
> >> Makefile logic as well as flex/bison code to be stubbed out, due to
> >> complexity building. By using libraries fewer functions are stubbed
> >> out, the build is faster and the Makefile logic is reused which should
> >> simplify updating. The libraries are passed through LDFLAGS to avoid
> >> complexity in python.
> >>
> >> Force the -fPIC flag for libbpf.a to ensure it is suitable for linking
> >> into the perf python module.
> >>
> >> Signed-off-by: Ian Rogers <irogers@google.com>
> >> Reviewed-by: James Clark <james.clark@arm.com>
> >> ---
> >>  tools/perf/Makefile.config |   5 +
> >>  tools/perf/Makefile.perf   |   6 +-
> >>  tools/perf/util/python.c   | 271 ++++++++++++++-----------------------
> >>  tools/perf/util/setup.py   |  33 +----
> >>  4 files changed, 110 insertions(+), 205 deletions(-)
> >>
> >> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> >> index 7f1e016a9253..639be696f597 100644
> >> --- a/tools/perf/Makefile.config
> >> +++ b/tools/perf/Makefile.config
> >> @@ -910,6 +910,11 @@ else
> >>           endif
> >>           CFLAGS += -DHAVE_LIBPYTHON_SUPPORT
> >>           $(call detected,CONFIG_LIBPYTHON)
> >> +     ifeq ($(filter -fPIC,$(CFLAGS)),)
> >
> > Nitpick: mixed TAB and SPACEs.
> >
> >
> >> +           # Building a shared library requires position independent code.
> >> +           CFLAGS += -fPIC
> >> +           CXXFLAGS += -fPIC
> >> +         endif
> >
> >
> > I'm curious if it's ok for static libraries too..
> >
> > Thanks,
> > Namhyung
> >
>
> I think I tested the whole set with a static build so it should be ok.

Ok, thanks for checking that!

Namhyung
Ian Rogers June 25, 2024, 6:14 p.m. UTC | #4
On Tue, Jun 25, 2024 at 10:48 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Tue, Jun 25, 2024 at 5:24 AM James Clark <james.clark@arm.com> wrote:
> >
> >
> >
> > On 24/06/2024 23:26, Namhyung Kim wrote:
> > > On Thu, Jun 13, 2024 at 04:31:21PM -0700, Ian Rogers wrote:
> > >> setup.py was building most perf sources causing setup.py to mimic the
> > >> Makefile logic as well as flex/bison code to be stubbed out, due to
> > >> complexity building. By using libraries fewer functions are stubbed
> > >> out, the build is faster and the Makefile logic is reused which should
> > >> simplify updating. The libraries are passed through LDFLAGS to avoid
> > >> complexity in python.
> > >>
> > >> Force the -fPIC flag for libbpf.a to ensure it is suitable for linking
> > >> into the perf python module.
> > >>
> > >> Signed-off-by: Ian Rogers <irogers@google.com>
> > >> Reviewed-by: James Clark <james.clark@arm.com>
> > >> ---
> > >>  tools/perf/Makefile.config |   5 +
> > >>  tools/perf/Makefile.perf   |   6 +-
> > >>  tools/perf/util/python.c   | 271 ++++++++++++++-----------------------
> > >>  tools/perf/util/setup.py   |  33 +----
> > >>  4 files changed, 110 insertions(+), 205 deletions(-)
> > >>
> > >> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> > >> index 7f1e016a9253..639be696f597 100644
> > >> --- a/tools/perf/Makefile.config
> > >> +++ b/tools/perf/Makefile.config
> > >> @@ -910,6 +910,11 @@ else
> > >>           endif
> > >>           CFLAGS += -DHAVE_LIBPYTHON_SUPPORT
> > >>           $(call detected,CONFIG_LIBPYTHON)
> > >> +     ifeq ($(filter -fPIC,$(CFLAGS)),)
> > >
> > > Nitpick: mixed TAB and SPACEs.

Will fix it in v2.

> > >
> > >
> > >> +           # Building a shared library requires position independent code.
> > >> +           CFLAGS += -fPIC
> > >> +           CXXFLAGS += -fPIC
> > >> +         endif
> > >
> > >
> > > I'm curious if it's ok for static libraries too..
> > >
> > > Thanks,
> > > Namhyung
> > >
> >
> > I think I tested the whole set with a static build so it should be ok.
>
> Ok, thanks for checking that!

Even with a static build it is often the case that the executable is
position independent (-fPIE, and related compiler options) and so
would need this too.

Thanks,
Ian

> Namhyung
diff mbox series

Patch

diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index 7f1e016a9253..639be696f597 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -910,6 +910,11 @@  else
          endif
          CFLAGS += -DHAVE_LIBPYTHON_SUPPORT
          $(call detected,CONFIG_LIBPYTHON)
+	 ifeq ($(filter -fPIC,$(CFLAGS)),)
+           # Building a shared library requires position independent code.
+           CFLAGS += -fPIC
+           CXXFLAGS += -fPIC
+         endif
       endif
     endif
   endif
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index ff03f0431013..4a1a9f09fa09 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -715,9 +715,9 @@  all: shell_compatibility_test $(ALL_PROGRAMS) $(LANG_BINDINGS) $(OTHER_PROGRAMS)
 # Create python binding output directory if not already present
 $(shell [ -d '$(OUTPUT)python' ] || mkdir -p '$(OUTPUT)python')
 
-$(OUTPUT)python/perf$(PYTHON_EXTENSION_SUFFIX): $(PYTHON_EXT_SRCS) $(PYTHON_EXT_DEPS) $(LIBPERF) $(LIBSUBCMD)
+$(OUTPUT)python/perf$(PYTHON_EXTENSION_SUFFIX): $(PYTHON_EXT_SRCS) $(PYTHON_EXT_DEPS) $(PERFLIBS)
 	$(QUIET_GEN)LDSHARED="$(CC) -pthread -shared" \
-        CFLAGS='$(CFLAGS)' LDFLAGS='$(LDFLAGS)' \
+        CFLAGS='$(CFLAGS)' LDFLAGS='$(LDFLAGS) $(LIBS)' \
 	  $(PYTHON_WORD) util/setup.py \
 	  --quiet build_ext; \
 	cp $(PYTHON_EXTBUILD_LIB)perf*.so $(OUTPUT)python/
@@ -933,7 +933,7 @@  $(LIBAPI)-clean:
 $(LIBBPF): FORCE | $(LIBBPF_OUTPUT)
 	$(Q)$(MAKE) -C $(LIBBPF_DIR) FEATURES_DUMP=$(FEATURE_DUMP_EXPORT) \
 		O= OUTPUT=$(LIBBPF_OUTPUT)/ DESTDIR=$(LIBBPF_DESTDIR) prefix= subdir= \
-		$@ install_headers
+		EXTRA_CFLAGS="-fPIC" $@ install_headers
 
 $(LIBBPF)-clean:
 	$(call QUIET_CLEAN, libbpf)
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 0aeb97c11c03..3be882b2e845 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -10,21 +10,19 @@ 
 #endif
 #include <perf/mmap.h>
 #include "evlist.h"
-#include "callchain.h"
 #include "evsel.h"
 #include "event.h"
 #include "print_binary.h"
 #include "thread_map.h"
 #include "trace-event.h"
 #include "mmap.h"
-#include "stat.h"
-#include "metricgroup.h"
 #include "util/bpf-filter.h"
 #include "util/env.h"
-#include "util/pmu.h"
-#include "util/pmus.h"
+#include "util/kvm-stat.h"
+#include "util/kwork.h"
+#include "util/lock-contention.h"
 #include <internal/lib.h>
-#include "util.h"
+#include "../builtin.h"
 
 #if PY_MAJOR_VERSION < 3
 #define _PyUnicode_FromString(arg) \
@@ -50,166 +48,6 @@ 
 #define Py_TYPE(ob) (((PyObject*)(ob))->ob_type)
 #endif
 
-/*
- * Avoid bringing in event parsing.
- */
-int parse_event(struct evlist *evlist __maybe_unused, const char *str __maybe_unused)
-{
-	return 0;
-}
-
-/*
- * Provide these two so that we don't have to link against callchain.c and
- * start dragging hist.c, etc.
- */
-struct callchain_param callchain_param;
-
-int parse_callchain_record(const char *arg __maybe_unused,
-			   struct callchain_param *param __maybe_unused)
-{
-	return 0;
-}
-
-/*
- * Add these not to drag util/env.c
- */
-struct perf_env perf_env;
-
-const char *perf_env__cpuid(struct perf_env *env __maybe_unused)
-{
-	return NULL;
-}
-
-// This one is a bit easier, wouldn't drag too much, but leave it as a stub we need it here
-const char *perf_env__arch(struct perf_env *env __maybe_unused)
-{
-	return NULL;
-}
-
-/*
- * These ones are needed not to drag the PMU bandwagon, jevents generated
- * pmu_sys_event_tables, etc and evsel__find_pmu() is used so far just for
- * doing per PMU perf_event_attr.exclude_guest handling, not really needed, so
- * far, for the perf python binding known usecases, revisit if this become
- * necessary.
- */
-struct perf_pmu *evsel__find_pmu(const struct evsel *evsel __maybe_unused)
-{
-	return NULL;
-}
-
-int perf_pmu__scan_file(const struct perf_pmu *pmu, const char *name, const char *fmt, ...)
-{
-	return EOF;
-}
-
-const char *perf_pmu__name_from_config(struct perf_pmu *pmu __maybe_unused, u64 config __maybe_unused)
-{
-	return NULL;
-}
-
-struct perf_pmu *perf_pmus__find_by_type(unsigned int type __maybe_unused)
-{
-	return NULL;
-}
-
-int perf_pmus__num_core_pmus(void)
-{
-	return 1;
-}
-
-bool evsel__is_aux_event(const struct evsel *evsel __maybe_unused)
-{
-	return false;
-}
-
-bool perf_pmus__supports_extended_type(void)
-{
-	return false;
-}
-
-/*
- * Add this one here not to drag util/metricgroup.c
- */
-int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
-				    struct rblist *new_metric_events,
-				    struct rblist *old_metric_events)
-{
-	return 0;
-}
-
-/*
- * Add this one here not to drag util/trace-event-info.c
- */
-char *tracepoint_id_to_name(u64 config)
-{
-	return NULL;
-}
-
-/*
- * XXX: All these evsel destructors need some better mechanism, like a linked
- * list of destructors registered when the relevant code indeed is used instead
- * of having more and more calls in perf_evsel__delete(). -- acme
- *
- * For now, add some more:
- *
- * Not to drag the BPF bandwagon...
- */
-void bpf_counter__destroy(struct evsel *evsel);
-int bpf_counter__install_pe(struct evsel *evsel, int cpu, int fd);
-int bpf_counter__disable(struct evsel *evsel);
-
-void bpf_counter__destroy(struct evsel *evsel __maybe_unused)
-{
-}
-
-int bpf_counter__install_pe(struct evsel *evsel __maybe_unused, int cpu __maybe_unused, int fd __maybe_unused)
-{
-	return 0;
-}
-
-int bpf_counter__disable(struct evsel *evsel __maybe_unused)
-{
-	return 0;
-}
-
-// not to drag util/bpf-filter.c
-#ifdef HAVE_BPF_SKEL
-int perf_bpf_filter__prepare(struct evsel *evsel __maybe_unused)
-{
-	return 0;
-}
-
-int perf_bpf_filter__destroy(struct evsel *evsel __maybe_unused)
-{
-	return 0;
-}
-#endif
-
-/*
- * Support debug printing even though util/debug.c is not linked.  That means
- * implementing 'verbose' and 'eprintf'.
- */
-int verbose;
-int debug_kmaps;
-int debug_peo_args;
-
-int eprintf(int level, int var, const char *fmt, ...);
-
-int eprintf(int level, int var, const char *fmt, ...)
-{
-	va_list args;
-	int ret = 0;
-
-	if (var >= level) {
-		va_start(args, fmt);
-		ret = vfprintf(stderr, fmt, args);
-		va_end(args);
-	}
-
-	return ret;
-}
-
 /* Define PyVarObject_HEAD_INIT for python 2.5 */
 #ifndef PyVarObject_HEAD_INIT
 # define PyVarObject_HEAD_INIT(type, size) PyObject_HEAD_INIT(type) size,
@@ -1510,15 +1348,102 @@  PyMODINIT_FUNC PyInit_perf(void)
 #endif
 }
 
-/*
- * Dummy, to avoid dragging all the test_attr infrastructure in the python
- * binding.
- */
-void test_attr__open(struct perf_event_attr *attr, pid_t pid, struct perf_cpu cpu,
-                     int fd, int group_fd, unsigned long flags)
+
+/* The following are stubs to avoid dragging in builtin-* objects. */
+/* TODO: move the code out of the builtin-* file into util. */
+
+unsigned int scripting_max_stack = PERF_MAX_STACK_DEPTH;
+
+bool kvm_entry_event(struct evsel *evsel __maybe_unused)
 {
+	return false;
+}
+
+bool kvm_exit_event(struct evsel *evsel __maybe_unused)
+{
+	return false;
+}
+
+bool exit_event_begin(struct evsel *evsel __maybe_unused,
+		      struct perf_sample *sample  __maybe_unused,
+		      struct event_key *key  __maybe_unused)
+{
+	return false;
+}
+
+bool exit_event_end(struct evsel *evsel __maybe_unused,
+		    struct perf_sample *sample __maybe_unused,
+		    struct event_key *key __maybe_unused)
+{
+	return false;
+}
+
+void exit_event_decode_key(struct perf_kvm_stat *kvm __maybe_unused,
+			   struct event_key *key __maybe_unused,
+			   char *decode __maybe_unused)
+{
+}
+
+int find_scripts(char **scripts_array  __maybe_unused, char **scripts_path_array  __maybe_unused,
+		int num  __maybe_unused, int pathlen __maybe_unused)
+{
+	return -1;
+}
+
+void perf_stat__set_no_csv_summary(int set __maybe_unused)
+{
+}
+
+void perf_stat__set_big_num(int set __maybe_unused)
+{
+}
+
+int script_spec_register(const char *spec __maybe_unused, struct scripting_ops *ops __maybe_unused)
+{
+	return -1;
+}
+
+arch_syscalls__strerrno_t *arch_syscalls__strerrno_function(const char *arch __maybe_unused)
+{
+	return NULL;
+}
+
+struct kwork_work *perf_kwork_add_work(struct perf_kwork *kwork __maybe_unused,
+				       struct kwork_class *class __maybe_unused,
+				       struct kwork_work *key  __maybe_unused)
+{
+	return NULL;
+}
+
+void script_fetch_insn(struct perf_sample *sample __maybe_unused,
+		struct thread *thread __maybe_unused,
+		struct machine *machine __maybe_unused)
+{
+}
+
+int perf_sample__sprintf_flags(u32 flags __maybe_unused, char *str __maybe_unused,
+			size_t sz __maybe_unused)
+{
+	return -1;
+}
+
+bool match_callstack_filter(struct machine *machine __maybe_unused, u64 *callstack __maybe_unused)
+{
+	return false;
+}
+
+struct lock_stat *lock_stat_find(u64 addr __maybe_unused)
+{
+	return NULL;
+}
+
+struct lock_stat *lock_stat_findnew(u64 addr __maybe_unused, const char *name __maybe_unused,
+				int flags __maybe_unused)
+{
+	return NULL;
 }
 
-void evlist__free_stats(struct evlist *evlist)
+int cmd_inject(int argc __maybe_unused, const char *argv[] __maybe_unused)
 {
+	return -1;
 }
diff --git a/tools/perf/util/setup.py b/tools/perf/util/setup.py
index 3107f5aa8c9a..142e9d447ce7 100644
--- a/tools/perf/util/setup.py
+++ b/tools/perf/util/setup.py
@@ -60,7 +60,7 @@  class install_lib(_install_lib):
 
 cflags = getenv('CFLAGS', '').split()
 # switch off several checks (need to be at the end of cflags list)
-cflags += ['-fno-strict-aliasing', '-Wno-write-strings', '-Wno-unused-parameter', '-Wno-redundant-decls', '-DPYTHON_PERF' ]
+cflags += ['-fno-strict-aliasing', '-Wno-write-strings', '-Wno-unused-parameter', '-Wno-redundant-decls' ]
 if cc_is_clang:
     cflags += ["-Wno-unused-command-line-argument" ]
 else:
@@ -72,36 +72,11 @@  cflags += [ "-Wno-declaration-after-statement" ]
 src_perf  = getenv('srctree') + '/tools/perf'
 build_lib = getenv('PYTHON_EXTBUILD_LIB')
 build_tmp = getenv('PYTHON_EXTBUILD_TMP')
-libtraceevent = getenv('LIBTRACEEVENT')
-libapikfs = getenv('LIBAPI')
-libperf = getenv('LIBPERF')
-
-ext_sources = [f.strip() for f in open('util/python-ext-sources')
-				if len(f.strip()) > 0 and f[0] != '#']
-
-extra_libraries = []
-
-if '-DHAVE_LIBTRACEEVENT' in cflags:
-    extra_libraries += [ 'traceevent' ]
-else:
-    ext_sources.remove('util/trace-event.c')
-    ext_sources.remove('util/trace-event-parse.c')
-
-# use full paths with source files
-ext_sources = list(map(lambda x: '%s/%s' % (src_perf, x) , ext_sources))
-
-if '-DHAVE_LIBNUMA_SUPPORT' in cflags:
-    extra_libraries += [ 'numa' ]
-if '-DHAVE_LIBCAP_SUPPORT' in cflags:
-    extra_libraries += [ 'cap' ]
 
 perf = Extension('perf',
-		  sources = ext_sources,
-		  include_dirs = ['util/include'],
-		  libraries = extra_libraries,
-		  extra_compile_args = cflags,
-		  extra_objects = [ x for x in [libtraceevent, libapikfs, libperf]
-                                    if x is not None],
+                 sources = [ src_perf + '/util/python.c' ],
+		         include_dirs = ['util/include'],
+		         extra_compile_args = cflags,
                  )
 
 setup(name='perf',