[PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

By default perf currently links with the GTK2 gui. This pulls
in a lot of external libraries. It also causes dependency
problems for distribution packages: simply installing perf
requires pulling in GTK2 with all its dependencies.

I think the UI is valuable, but it shouldn't be everywhere.

The interfaces between the main perf and the GTK2 perf are
already quite clean, so it's very straight forward to just
add a few weak stubs and then generate two executables:
perf and perfgtk

The only difference is that the gtk version links in the
GTK code and overrides the weak stubs.
(so everything is still only compiled once)

I currently gave it the preliminary name "perfgtk".

This cuts down the library dependencies on the main perf
dramatically. It also completely eliminates the GTK2_SUPPORT
ifdef.

% ldd ./perf | wc -l
18
% ldd ./perfgtk | wc -l
53

Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: mingo@kernel.org
Cc: peterz@infradead.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/Makefile        | 34 +++++++++++++++++++++-------------
 tools/perf/config/Makefile |  4 ++--
 tools/perf/ui/gtkstub.c    | 37 +++++++++++++++++++++++++++++++++++++
 tools/perf/ui/ui.h         |  8 --------
 tools/perf/util/annotate.h | 11 -----------
 tools/perf/util/hist.h     | 11 -----------
 6 files changed, 60 insertions(+), 45 deletions(-)
 create mode 100644 tools/perf/ui/gtkstub.c

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 641fccd..25116fc 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -113,6 +113,7 @@ SPARSE_FLAGS = -D__BIG_ENDIAN__ -D__powerpc__
 BUILTIN_OBJS =
 LIB_H =
 LIB_OBJS =
+GUI_OBJS =
 PYRF_OBJS =
 SCRIPT_SH =
 
@@ -161,11 +162,12 @@ $(OUTPUT)python/perf.so: $(PYTHON_EXT_SRCS) $(PYTHON_EXT_DEPS)
 
 SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH))
 
-#
-# Single 'perf' binary right now:
-#
 PROGRAMS += $(OUTPUT)perf
 
+ifndef NO_GTK2
+  PROGRAMS += $(OUTPUT)perfgtk
+endif
+
 # what 'all' will build and 'install' will install, in perfexecdir
 ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
 
@@ -366,6 +368,7 @@ LIB_OBJS += $(OUTPUT)ui/helpline.o
 LIB_OBJS += $(OUTPUT)ui/progress.o
 LIB_OBJS += $(OUTPUT)ui/util.o
 LIB_OBJS += $(OUTPUT)ui/hist.o
+LIB_OBJS += $(OUTPUT)ui/gtkstub.o
 LIB_OBJS += $(OUTPUT)ui/stdio/hist.o
 
 LIB_OBJS += $(OUTPUT)arch/common.o
@@ -481,13 +484,13 @@ ifndef NO_SLANG
 endif
 
 ifndef NO_GTK2
-  LIB_OBJS += $(OUTPUT)ui/gtk/browser.o
-  LIB_OBJS += $(OUTPUT)ui/gtk/hists.o
-  LIB_OBJS += $(OUTPUT)ui/gtk/setup.o
-  LIB_OBJS += $(OUTPUT)ui/gtk/util.o
-  LIB_OBJS += $(OUTPUT)ui/gtk/helpline.o
-  LIB_OBJS += $(OUTPUT)ui/gtk/progress.o
-  LIB_OBJS += $(OUTPUT)ui/gtk/annotate.o
+  GUI_OBJS += $(OUTPUT)ui/gtk/browser.o
+  GUI_OBJS += $(OUTPUT)ui/gtk/hists.o
+  GUI_OBJS += $(OUTPUT)ui/gtk/setup.o
+  GUI_OBJS += $(OUTPUT)ui/gtk/util.o
+  GUI_OBJS += $(OUTPUT)ui/gtk/helpline.o
+  GUI_OBJS += $(OUTPUT)ui/gtk/progress.o
+  GUI_OBJS += $(OUTPUT)ui/gtk/annotate.o
 endif
 
 ifndef NO_LIBPERL
@@ -541,6 +544,10 @@ $(OUTPUT)perf: $(OUTPUT)perf.o $(BUILTIN_OBJS) $(PERFLIBS)
 	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $(OUTPUT)perf.o \
                $(BUILTIN_OBJS) $(LIBS) -o $@
 
+$(OUTPUT)perfgtk: $(OUTPUT)perf.o $(BUILTIN_OBJS) $(PERFLIBS) $(GUI_OBJS)
+	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $(OUTPUT)perf.o \
+               $(BUILTIN_OBJS) $(GUI_OBJS) $(LIBS) $(GUI_EXTLIBS) -o $@
+
 $(OUTPUT)builtin-help.o: builtin-help.c $(OUTPUT)common-cmds.h $(OUTPUT)PERF-CFLAGS
 	$(QUIET_CC)$(CC) -o $@ -c $(CFLAGS) \
 		'-DPERF_HTML_PATH="$(htmldir_SQ)"' \
@@ -645,12 +652,12 @@ $(OUTPUT)scripts/python/Perf-Trace-Util/Context.o: scripts/python/Perf-Trace-Uti
 $(OUTPUT)perf-%: %.o $(PERFLIBS)
 	$(QUIET_LINK)$(CC) $(CFLAGS) -o $@ $(LDFLAGS) $(filter %.o,$^) $(LIBS)
 
-$(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
+$(LIB_OBJS) $(BUILTIN_OBJS) $(GUI_OBJS): $(LIB_H)
 $(patsubst perf-%,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)
 
 # we compile into subdirectories. if the target directory is not the source directory, they might not exists. So
 # we depend the various files onto their directories.
-DIRECTORY_DEPS = $(LIB_OBJS) $(BUILTIN_OBJS) $(OUTPUT)PERF-VERSION-FILE $(OUTPUT)common-cmds.h
+DIRECTORY_DEPS = $(LIB_OBJS) $(GUI_OBJS) $(BUILTIN_OBJS) $(OUTPUT)PERF-VERSION-FILE $(OUTPUT)common-cmds.h
 $(DIRECTORY_DEPS): | $(sort $(dir $(DIRECTORY_DEPS)))
 # In the second step, we make a rule to actually create these directories
 $(sort $(dir $(DIRECTORY_DEPS))):
@@ -792,7 +799,8 @@ $(INSTALL_DOC_TARGETS):
 ### Cleaning rules
 
 clean: $(LIBTRACEEVENT)-clean $(LIBLK)-clean
-	$(RM) $(LIB_OBJS) $(BUILTIN_OBJS) $(LIB_FILE) $(OUTPUT)perf-archive $(OUTPUT)perf.o $(LANG_BINDINGS)
+	$(RM) $(LIB_OBJS) $(GUI_OBJS) $(BUILTIN_OBJS) $(LIB_FILE)
+	$(RM) $(OUTPUT)perf-archive $(OUTPUT)perf.o $(LANG_BINDINGS)
 	$(RM) $(ALL_PROGRAMS) perf
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo $(OUTPUT)common-cmds.h TAGS tags cscope*
 	$(QUIET_SUBDIR0)Documentation $(QUIET_SUBDIR1) clean
diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index b5d9238..d718961 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -86,6 +86,7 @@ CFLAGS += -Wextra
 CFLAGS += -std=gnu99
 
 EXTLIBS = -lelf -lpthread -lrt -lm
+GUI_EXTLIBS =
 
 ifeq ($(call try-cc,$(SOURCE_HELLO),$(CFLAGS) -Werror -fstack-protector-all,-fstack-protector-all),y)
   CFLAGS += -fstack-protector-all
@@ -268,9 +269,8 @@ ifndef NO_GTK2
     ifeq ($(call try-cc,$(SOURCE_GTK2_INFOBAR),$(FLAGS_GTK2),-DHAVE_GTK_INFO_BAR),y)
       CFLAGS += -DHAVE_GTK_INFO_BAR
     endif
-    CFLAGS += -DGTK2_SUPPORT
     CFLAGS += $(shell pkg-config --cflags gtk+-2.0 2>/dev/null)
-    EXTLIBS += $(shell pkg-config --libs gtk+-2.0 2>/dev/null)
+    GUI_EXTLIBS += $(shell pkg-config --libs gtk+-2.0 2>/dev/null)
   endif
 endif
 
diff --git a/tools/perf/ui/gtkstub.c b/tools/perf/ui/gtkstub.c
new file mode 100644
index 0000000..cb58d31
--- /dev/null
+++ b/tools/perf/ui/gtkstub.c
@@ -0,0 +1,37 @@
+#include "ui/ui.h"
+#include "util/annotate.h"
+
+/* Stubs used when the gtk2 code is not linked in */
+
+#define __weak __attribute__((weak))
+
+__weak int perf_gtk__init(void)
+{
+	return -1;
+}
+
+__weak void perf_gtk__exit(bool wait_for_ok __maybe_unused)
+{
+}
+
+__weak int perf_evlist__gtk_browse_hists(struct perf_evlist *evlist __maybe_unused,
+					 const char *help __maybe_unused,
+					 struct hist_browser_timer *hbt __maybe_unused,
+					 float min_pcnt __maybe_unused)
+{
+	return 0;
+}
+
+
+__weak void perf_gtk__show_annotations(void)
+{
+}
+
+__weak int symbol__gtk_annotate(struct symbol *sym __maybe_unused,
+				struct map *map __maybe_unused,
+				struct perf_evsel *evsel __maybe_unused,
+				struct hist_browser_timer *hbt __maybe_unused)
+{
+	return 0;
+}
+
diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
index 70cb0d4..3a6ca86 100644
--- a/tools/perf/ui/ui.h
+++ b/tools/perf/ui/ui.h
@@ -23,16 +23,8 @@ static inline int ui__init(void)
 static inline void ui__exit(bool wait_for_ok __maybe_unused) {}
 #endif
 
-#ifdef GTK2_SUPPORT
 int perf_gtk__init(void);
 void perf_gtk__exit(bool wait_for_ok);
-#else
-static inline int perf_gtk__init(void)
-{
-	return -1;
-}
-static inline void perf_gtk__exit(bool wait_for_ok __maybe_unused) {}
-#endif
 
 void ui__refresh_dimensions(bool force);
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index af75515..ac70cc6 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -165,7 +165,6 @@ static inline int symbol__tui_annotate(struct symbol *sym __maybe_unused,
 }
 #endif
 
-#ifdef GTK2_SUPPORT
 int symbol__gtk_annotate(struct symbol *sym, struct map *map,
 			 struct perf_evsel *evsel,
 			 struct hist_browser_timer *hbt);
@@ -178,16 +177,6 @@ static inline int hist_entry__gtk_annotate(struct hist_entry *he,
 }
 
 void perf_gtk__show_annotations(void);
-#else
-static inline int hist_entry__gtk_annotate(struct hist_entry *he __maybe_unused,
-				struct perf_evsel *evsel __maybe_unused,
-				struct hist_browser_timer *hbt __maybe_unused)
-{
-	return 0;
-}
-
-static inline void perf_gtk__show_annotations(void) {}
-#endif
 
 extern const char	*disassembler_style;
 
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 2d3790f..9fa8b2d 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -229,20 +229,9 @@ static inline int script_browse(const char *script_opt __maybe_unused)
 #define K_SWITCH_INPUT_DATA -3000
 #endif
 
-#ifdef GTK2_SUPPORT
 int perf_evlist__gtk_browse_hists(struct perf_evlist *evlist, const char *help,
 				  struct hist_browser_timer *hbt __maybe_unused,
 				  float min_pcnt);
-#else
-static inline
-int perf_evlist__gtk_browse_hists(struct perf_evlist *evlist __maybe_unused,
-				  const char *help __maybe_unused,
-				  struct hist_browser_timer *hbt __maybe_unused,
-				  float min_pcnt __maybe_unused)
-{
-	return 0;
-}
-#endif
 
 unsigned int hists__sort_list_width(struct hists *self);
 
-- 
1.8.3.1

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> From: Andi Kleen <ak@linux.intel.com>
> 
> By default perf currently links with the GTK2 gui. This pulls
> in a lot of external libraries. It also causes dependency
> problems for distribution packages: simply installing perf
> requires pulling in GTK2 with all its dependencies.
> 
> I think the UI is valuable, but it shouldn't be everywhere.
> 
> The interfaces between the main perf and the GTK2 perf are
> already quite clean, so it's very straight forward to just
> add a few weak stubs and then generate two executables:
> perf and perfgtk
> 
> The only difference is that the gtk version links in the
> GTK code and overrides the weak stubs.
> (so everything is still only compiled once)
> 
> I currently gave it the preliminary name "perfgtk".
> 
> This cuts down the library dependencies on the main perf
> dramatically. It also completely eliminates the GTK2_SUPPORT
> ifdef.
> 
> % ldd ./perf | wc -l
> 18
> % ldd ./perfgtk | wc -l
> 53

If you want fewer dependencies then build with 'make NO_GTK=1'.

Furthermore, what really matters in practice is binary size - and the GKT 
UI frontend code isn't really big:

 comet:~/tip/tools/perf> ls -l perf.gtk perf.nogtk
 -rwxrwxr-x 1 mingo mingo 2525416 Aug  5 10:09 perf.gtk.stripped
 -rwxrwxr-x 1 mingo mingo 2497480 Aug  5 10:09 perf.nogtk.stripped

that's only a 1% difference ...

So this is not a good idea, as it breaks the single binary structure of 
perf, which is a rather powerful concept that has served us really well in 
the past.

Thanks,

	Ingo

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Christoph Hellwig]

On Mon, Aug 05, 2013 at 10:16:41AM +0200, Ingo Molnar wrote:
> If you want fewer dependencies then build with 'make NO_GTK=1'.

Doesn't help the distros.  Installing perf and pulling all the graphics
libraries in is highly annoying, especially in size constrained VM or
Cloud images.  Having a separate binary or at least an ldopen()able
plugin that can go into a separate package is highly desirable.

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Aug 05, 2013 at 10:16:41AM +0200, Ingo Molnar wrote:
>
> > If you want fewer dependencies then build with 'make NO_GTK=1'.
> 
> Doesn't help the distros.  Installing perf and pulling all the graphics 
> libraries in is highly annoying, especially in size constrained VM or 
> Cloud images.  Having a separate binary or at least an ldopen()able 
> plugin that can go into a separate package is highly desirable.

Nonsense, a distro, if it truly worried about this, could create two 
packages already, there's no need to expose configuration options in the 
binary name itself and burden users with the separation. I sometimes 
switch the UI frontend of perf depending on the workflow and the terminal, 
it would be highly annoying if the binary name was changed to expose 
configuration options.

The thing is, you strongly objected to perf itself when we offered it up 
for an upstream merge and I'm not surprised you still don't like it.

Thanks,

	Ingo

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Christoph Hellwig]

On Mon, Aug 05, 2013 at 10:31:32AM +0200, Ingo Molnar wrote:
> Nonsense, a distro, if it truly worried about this, could create two 
> packages already, there's no need to expose configuration options in the 
> binary name itself and burden users with the separation. I sometimes 
> switch the UI frontend of perf depending on the workflow and the terminal, 
> it would be highly annoying if the binary name was changed to expose 
> configuration options.

Which means you'd have to use a different tool name or have incompatible
packages, both of which aren't desirable.

> The thing is, you strongly objected to perf itself when we offered it up 
> for an upstream merge and I'm not surprised you still don't like it.

I strongly objected to adding it to the kernel tree, and I still stand
to that opinion because it makes using perf much more painful than it
needs to be.  I never disliked perf itself and use it frequently now
that I can bypass some of the pains by just using an older distro
package.

But I'd much rather get this back to technical discussions than personal
attacks..

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Namhyung Kim]

Hi,

On Mon, 5 Aug 2013 01:23:20 -0700, Christoph Hellwig wrote:
> On Mon, Aug 05, 2013 at 10:16:41AM +0200, Ingo Molnar wrote:
>> If you want fewer dependencies then build with 'make NO_GTK=1'.
>
> Doesn't help the distros.  Installing perf and pulling all the graphics
> libraries in is highly annoying, especially in size constrained VM or
> Cloud images.  Having a separate binary or at least an ldopen()able
> plugin that can go into a separate package is highly desirable.

I wrote a patch series [1] separating gtk code to a dso and use it with
libdl last year.  But I didn't get much feedback probably due to the
mistake of not installing the dso to a proper place.  It'd be great if
you guys take a look at it and give some comments.

Thanks,
Namhyung


[1] https://lkml.org/lkml/2012/11/14/510

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Aug 05, 2013 at 10:31:32AM +0200, Ingo Molnar wrote:
>
> > Nonsense, a distro, if it truly worried about this, could create two 
> > packages already, there's no need to expose configuration options in 
> > the binary name itself and burden users with the separation. I 
> > sometimes switch the UI frontend of perf depending on the workflow and 
> > the terminal, it would be highly annoying if the binary name was 
> > changed to expose configuration options.
> 
> Which means you'd have to use a different tool name or have incompatible 
> packages, both of which aren't desirable.

You'd have alternative packages - i.e. the configuration and dependency 
difference is exposed in the packaging space, not in the user interface, 
command name space.

(and yes, gtk linking in 20+ libraries is suboptimal, hopefully that will 
eventually be fixed by the GTK project. If a leaner project with similarly 
good UI elements comes around we might switch to it - without having to 
rename the binary yet again.)

I.e. put the burden on packagers for too high library dependency 
complexity, not on end users. A fair deal by any count.

> > The thing is, you strongly objected to perf itself when we offered it 
> > up for an upstream merge and I'm not surprised you still don't like 
> > it.
> 
> I strongly objected to adding it to the kernel tree, and I still stand 
> to that opinion because it makes using perf much more painful than it 
> needs to be. [...]

That's still a red herring - 'using' perf for 99% of the users is to 
install the perf package or to type 'make install' ...

> [...]  I never disliked perf itself and use it frequently now that I can 
> bypass some of the pains by just using an older distro package.

If you have special needs you could lobby your distro for different 
versions - or you could build it from source.

Your solution, to split the binary into two parts, just to expose a 
configuration option you don't want to enable in certain uses, burdens the 
regular user of perf.

> But I'd much rather get this back to technical discussions than personal 
> attacks..

You never replied to the original counter-arguments, such as this one from 
Linus:

  http://article.gmane.org/gmane.linux.kernel/849965

Or this one from Andrew:

  http://article.gmane.org/gmane.linux.kernel/850067

so I assumed your (still arguably dishonest) objections are still valid 
and still broad - and are reflected in this thread.

That's not a personal attack by any means - we met before and I actually 
like you as a person, I just don't like your opinion here and I don't like 
your occasionally dishonest discussion style: because it only focuses on 
the narrow issue of packaging complexity and does not look at the bigger 
picture such as health of development and end user ease of use.

Thanks,

	Ingo

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> Hi,
> 
> On Mon, 5 Aug 2013 01:23:20 -0700, Christoph Hellwig wrote:
> > On Mon, Aug 05, 2013 at 10:16:41AM +0200, Ingo Molnar wrote:
> >> If you want fewer dependencies then build with 'make NO_GTK=1'.
> >
> > Doesn't help the distros.  Installing perf and pulling all the 
> > graphics libraries in is highly annoying, especially in size 
> > constrained VM or Cloud images.  Having a separate binary or at least 
> > an ldopen()able plugin that can go into a separate package is highly 
> > desirable.
> 
> I wrote a patch series [1] separating gtk code to a dso and use it with 
> libdl last year.  But I didn't get much feedback probably due to the 
> mistake of not installing the dso to a proper place.  It'd be great if 
> you guys take a look at it and give some comments.
> 
> Thanks,
> Namhyung
> 
> 
> [1] https://lkml.org/lkml/2012/11/14/510

Assuming that in the normal case it can be made to work like a full build 
of perf today (i.e. without forcing LD_PRELOAD) then splitting the 
libraries away looks like a much better solution than splitting the 
binary.

Thanks,

	Ingo

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Pekka Enberg]

On Mon, Aug 5, 2013 at 12:12 PM, Ingo Molnar <mingo@kernel.org> wrote:
> Assuming that in the normal case it can be made to work like a full build
> of perf today (i.e. without forcing LD_PRELOAD) then splitting the
> libraries away looks like a much better solution than splitting the
> binary.

Yup, if we can make it work, it's probably the best option. I'm not very
keen on splitting the binary but pulling all the GTK dependencies
like we do now is definitely a problem.

                        Pekka

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Andi Kleen]

Namhyung Kim <namhyung@kernel.org> writes:
>
> I wrote a patch series [1] separating gtk code to a dso and use it with
> libdl last year.  But I didn't get much feedback probably due to the
> mistake of not installing the dso to a proper place.  It'd be great if
> you guys take a look at it and give some comments.

It looks good as a replacement of this patch

Except for the requirement to build libperf as -fPIC
In some setups the PIC penalty can be high, and libperf
has some very hot code (e.g. callstack merging)

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Christoph Hellwig]

On Mon, Aug 05, 2013 at 11:08:57AM +0200, Ingo Molnar wrote:
> You never replied to the original counter-arguments, such as this one from 
> Linus:
> 
>   http://article.gmane.org/gmane.linux.kernel/849965

The only thing Linus sais is that it's trivial to generate a subpackage,
and that opofile is a desaster.  Both of them are 100% correct but at
the same time entirely miss the point.

Yes, oprofile was and is a desaster, but that has aboslutely nothing to
do with where the code lives.

And yes, it's easy to generate a subpackage, but you still need all the
source tree first.  There's a reason why things like X.org got split up
(too fine grained in my opinion, but that's another story).

As said I very much disagree with having the userspace perf tree in the
kernel still, but I've also given up on the fight as I have more
important things to do.

And as said before it has nothing to do with the issue discussed here
right now.

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Aug 05, 2013 at 11:08:57AM +0200, Ingo Molnar wrote:
> > You never replied to the original counter-arguments, such as this one from 
> > Linus:
> > 
> >   http://article.gmane.org/gmane.linux.kernel/849965
> 
> The only thing Linus sais is that it's trivial to generate a subpackage, 
> and that opofile is a desaster.  Both of them are 100% correct but at 
> the same time entirely miss the point.
> 
> Yes, oprofile was and is a desaster, but that has aboslutely nothing to 
> do with where the code lives.
> 
> And yes, it's easy to generate a subpackage, but you still need all the 
> source tree first. [...]

And the kernel source tree is not particularly hard to get so what's your 
point? ...

> [...]  There's a reason why things like X.org got split up (too fine 
> grained in my opinion, but that's another story).

X.org got split up for all the wrong reasons, it's still a unified project 
by and large, and the different components only work reliably when going 
in lockstep so it's not like there's a true ABI between them.

So I really hope you don't advocate for that.

perf is the exact opposite: no split-up the development culture because 
they are closely related, yet a relatively disciplined ABI between the 
components. In fact the ABI is higher quality exactly because development 
is more integrated and allows for ABI problems to be resolved before they 
leak out. It also allows for faster iteration of development, without 
nonsensical ABI steps pulluting the way.

> As said I very much disagree with having the userspace perf tree in the 
> kernel still, but I've also given up on the fight as I have more 
> important things to do.
> 
> And as said before it has nothing to do with the issue discussed here 
> right now.

My point is that it's a very similar meta argument: splitting up perf 
usage space would be nonsensical and harmful in a similar fashion as it 
would be harmful to split up the perf development space.

Put differently: there's strong benefits to having a unified perf 
development environment and there's equally strong benefits on the user 
side to have a unified perf usage environment that a single command 
represents.

The benefits are not absolute and not unconditional, and any costs of 
integration should be minimized to the best of our ability, but I hope you 
get the drift of my argument ...

Thanks,

	Ingo

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Vince Weaver]

On Mon, 12 Aug 2013, Ingo Molnar wrote:

> perf is the exact opposite: no split-up the development culture because 
> they are closely related, yet a relatively disciplined ABI between the 
> components. In fact the ABI is higher quality exactly because development 
> is more integrated and allows for ABI problems to be resolved before they 
> leak out. It also allows for faster iteration of development, without 
> nonsensical ABI steps pulluting the way.

I don't know if I'd use "quality" and "perf ABI" in the same sentence.
It's a horrible ABI; it has the honor of having the longest syscall
manpage, beating out even ptrace.

It also really isn't that stable; I've had perf ABI changes break programs 
I maintain at least three times in the last 2 kernel releases.  Part of 
this is due to the tight coupling into the kernel, in fact the only ABI 
anyone seems to care about is that presented by the perf-tool CLI 
interface; the _actual_ kernel ABI seems like an afterthought.

Vince

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Ingo Molnar]

* Vince Weaver <vince@deater.net> wrote:

> On Mon, 12 Aug 2013, Ingo Molnar wrote:
> 
> > perf is the exact opposite: no split-up the development culture 
> > because they are closely related, yet a relatively disciplined ABI 
> > between the components. In fact the ABI is higher quality exactly 
> > because development is more integrated and allows for ABI problems to 
> > be resolved before they leak out. It also allows for faster iteration 
> > of development, without nonsensical ABI steps pulluting the way.
> 
> I don't know if I'd use "quality" and "perf ABI" in the same sentence. 
> It's a horrible ABI; it has the honor of having the longest syscall 
> manpage, beating out even ptrace.

The functionality it provides is useful, and comprehensive documentation 
of it is useful as well.

> It also really isn't that stable; I've had perf ABI changes break 
> programs I maintain at least three times in the last 2 kernel releases.  
> Part of this is due to the tight coupling into the kernel, in fact the 
> only ABI anyone seems to care about is that presented by the perf-tool 
> CLI interface; the _actual_ kernel ABI seems like an afterthought.

It's certainly complex, but the main root cause for your problems is what 
I pointed out to you in previous, similar discussions: I'm not aware of 
*any* tester using your library on devel kernels, so regressions in seldom 
used functionality that you rely on simply doesn't get reported.

In the past you used to only test your library once the -stable kernel was 
released - has this changed recently by any chance? I remember that in one 
particular case I got a regression bugreport from you essentially on the 
day of a -stable release.

If you tested -rc2 or so that would give us a much larger window to fix 
any breakages that affect your library. (I'm not even asking for 
linux-next testing.)

tools/perf is used much more prominently and breakages do get reported 
reasonably early, typically before the merge window even opens.

Once we receive a report we do fix your regressions as well and mark them 
for -stable.

To resolve this situation you could help us out by doing either of these:

 1) integrate your tests into tools/, there's 'perf test' that has a ton
    of testcases already

 2) run your testsuite more frequently - instead of waiting for a stable
    kernel to be released and then complain about breakage.

So far you refused to do any of this and blamed others for non-reported 
breakages :-/

Thanks,

	Ingo

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Arnaldo Carvalho de Melo]

Em Tue, Aug 13, 2013 at 12:48:21PM +0200, Ingo Molnar escreveu:
> To resolve this situation you could help us out by doing either of these:
 
>  1) integrate your tests into tools/, there's 'perf test' that has a ton
>     of testcases already

I think Jiri is working on merging some of those tests, Jiri?

Yeah, these at least:

commit 06933e3a732bb305b0721f1051a45264588e0519
Author: Jiri Olsa <jolsa@redhat.com>
Date:   Sun Mar 10 19:41:11 2013 +0100

    perf tests: Test breakpoint overflow signal handler counts

commit 5a6bef47b418676546ab86d25631c3cfb9ffaf2a
Author: Jiri Olsa <jolsa@redhat.com>
Date:   Sun Mar 10 19:41:10 2013 +0100

    perf tests: Test breakpoint overflow signal handler
 
>  2) run your testsuite more frequently - instead of waiting for a stable
>     kernel to be released and then complain about breakage.

My feeling is that he increased the frequency of such tests.

> So far you refused to do any of this and blamed others for non-reported 
> breakages :-/

He has been using the perf trinity fuzzer and reporting problems, thanks
Vince!

- Arnaldo

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Vince Weaver]

On Tue, 13 Aug 2013, Ingo Molnar wrote:
> * Vince Weaver <vince@deater.net> wrote:

> In the past you used to only test your library once the -stable kernel was 
> released - has this changed recently by any chance? I remember that in one 
> particular case I got a regression bugreport from you essentially on the 
> day of a -stable release.
> 
> If you tested -rc2 or so that would give us a much larger window to fix 
> any breakages that affect your library. (I'm not even asking for 
> linux-next testing.)

I wasn't aware that the Linux "no-ABI breakage guarantee" only applied to 
people who ran -rc kernels.

I've spent a lot of time enhancing trinity and writing my own perf syscall 
fuzzer just to try to keep on top of ABI breaks as per your rules and they 
still slip in and they still don't get reverted.

In any case, a list of perf-related ABI breakages is here.
 http://web.eece.maine.edu/~vweaver/projects/perf_events/abi_breakage.html
I guess whether it's a lot or just a few depends on your perspective.

> To resolve this situation you could help us out by doing either of these:
> 
>  1) integrate your tests into tools/, there's 'perf test' that has a ton
>     of testcases already

I have enough trouble keeping my code up to date let alone wasting weeks 
of time re-sending patches.  Some things are just simpler to maintain 
outside the kernel.

>  2) run your testsuite more frequently - instead of waiting for a stable
>     kernel to be released and then complain about breakage.

I only have so many machines and so much time, and no one is funding me 
for this work.  I run -rc kernels when I can, but perf can be very 
architecture and even CPU dependent so it is hard to get full coverage.  
For example it's hard to test full SNB-EP uncore support w/o such a 
machine.

Vince

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> Em Tue, Aug 13, 2013 at 12:48:21PM +0200, Ingo Molnar escreveu:
> > To resolve this situation you could help us out by doing either of these:
>  
> >  1) integrate your tests into tools/, there's 'perf test' that has a ton
> >     of testcases already
> 
> I think Jiri is working on merging some of those tests, Jiri?
> 
> Yeah, these at least:
> 
> commit 06933e3a732bb305b0721f1051a45264588e0519
> Author: Jiri Olsa <jolsa@redhat.com>
> Date:   Sun Mar 10 19:41:11 2013 +0100
> 
>     perf tests: Test breakpoint overflow signal handler counts
> 
> commit 5a6bef47b418676546ab86d25631c3cfb9ffaf2a
> Author: Jiri Olsa <jolsa@redhat.com>
> Date:   Sun Mar 10 19:41:10 2013 +0100
> 
>     perf tests: Test breakpoint overflow signal handler
>  
> >  2) run your testsuite more frequently - instead of waiting for a stable
> >     kernel to be released and then complain about breakage.
> 
> My feeling is that he increased the frequency of such tests.
> 
> > So far you refused to do any of this and blamed others for non-reported 
> > breakages :-/
> 
> He has been using the perf trinity fuzzer and reporting problems, thanks
> Vince!

Absolutely, it's much appreciated, thanks Vince!

I was reacting to this claim:

> > I've had perf ABI changes break programs I maintain at least three 
> > times in the last 2 kernel releases.

that can only be addressed by either extending 'perf test' or by testing 
libpfm et al sooner. The upstream kernel can only address regressions that 
get reported.

Thanks,

	Ingo

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Ingo Molnar]

* Vince Weaver <vince@deater.net> wrote:

> On Tue, 13 Aug 2013, Ingo Molnar wrote:
> > * Vince Weaver <vince@deater.net> wrote:
> 
> > In the past you used to only test your library once the -stable kernel was 
> > released - has this changed recently by any chance? I remember that in one 
> > particular case I got a regression bugreport from you essentially on the 
> > day of a -stable release.
> > 
> > If you tested -rc2 or so that would give us a much larger window to 
> > fix any breakages that affect your library. (I'm not even asking for 
> > linux-next testing.)
> 
> I wasn't aware that the Linux "no-ABI breakage guarantee" only applied 
> to people who ran -rc kernels.

It obviously does not, and nowhere did I claim the opposite.

> I've spent a lot of time enhancing trinity and writing my own perf 
> syscall fuzzer just to try to keep on top of ABI breaks as per your 
> rules and they still slip in and they still don't get reverted.

Your fuzzing effort is much appreciated, but please don't mix this up with 
the other ABI bugs you were talking about:

> > > > I've had perf ABI changes break programs I maintain at least three 
> > > > times in the last 2 kernel releases.

We can only fix regressions after they get reported.

Thanks,

	Ingo

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Vince Weaver]

On Tue, 13 Aug 2013, Ingo Molnar wrote:

> 
> that can only be addressed by either extending 'perf test' or by testing 
> libpfm et al sooner. The upstream kernel can only address regressions that 
> get reported.

Most of the tests in my test-suite are reactive.  Meaning, I wrote them 
after an ABI-breaking change was reported elsewhere, and I needed a small 
test case for bisection purposes.  Thus they are good for finding if a 
corner of the perf ABI re-breaks but they're not great at spotting new 
breakages.

Writing a complete test suite for something as complicated as the 
perf-event ABI is impractical.  One thing you can do is require anyone 
submitting new functionality also provide a regression test, but
I don't see that happening.

Another issue is that despite having some ABI definitions for files in 
/sys, these are broken with impunity.  And I've yet to have an 
ABI-breaking changeset reverted based on my bug reports.  So you can see 
why I'm not really motivated to even bother trying, as it seems like it 
would be pointless busy work at this point.

It would just be nice if we just straight out say "the ABI is whatever 
lets the perf tool run.  Anything else is undefined behavior and shouldn't 
be relied on".

Vince

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Andi Kleen]

On Tue, Aug 13, 2013 at 05:57:16PM -0400, Vince Weaver wrote:
> On Tue, 13 Aug 2013, Ingo Molnar wrote:
> 
> > 
> > that can only be addressed by either extending 'perf test' or by testing 
> > libpfm et al sooner. The upstream kernel can only address regressions that 
> > get reported.
> 
> Most of the tests in my test-suite are reactive.  Meaning, I wrote them 
> after an ABI-breaking change was reported elsewhere, and I needed a small 
> test case for bisection purposes.  Thus they are good for finding if a 
> corner of the perf ABI re-breaks but they're not great at spotting new 
> breakages.

I guess best would be to just run the major other users (PAPI, perf, trinity, 
numatop, ...?) once around rc1 time.

If it's reported at rc1 things can usually be fixed.

So would just need a easy script to do a quick test of all of these.

-Andi

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Arnaldo Carvalho de Melo]

Em Tue, Aug 13, 2013 at 05:57:16PM -0400, Vince Weaver escreveu:
> On Tue, 13 Aug 2013, Ingo Molnar wrote:
> > that can only be addressed by either extending 'perf test' or by testing 
> > libpfm et al sooner. The upstream kernel can only address regressions that 
> > get reported.
 
> Most of the tests in my test-suite are reactive.  Meaning, I wrote them 
> after an ABI-breaking change was reported elsewhere, and I needed a small 
> test case for bisection purposes.  Thus they are good for finding if a 
> corner of the perf ABI re-breaks but they're not great at spotting new 
> breakages.
 
> Writing a complete test suite for something as complicated as the 
> perf-event ABI is impractical.  One thing you can do is require anyone 
> submitting new functionality also provide a regression test, but

Agreed.

> I don't see that happening.

See some of Namhyung, Adrian and Jiri recent patchsets, they came with
'perf test' regression tests.
 
- ARnaldo

Re: [PATCH] RFC: perf, tools: Move gtk browser into separate perfgtk executable

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> Em Tue, Aug 13, 2013 at 05:57:16PM -0400, Vince Weaver escreveu:
> > On Tue, 13 Aug 2013, Ingo Molnar wrote:
> >
> > > that can only be addressed by either extending 'perf test' or by 
> > > testing libpfm et al sooner. The upstream kernel can only address 
> > > regressions that get reported.
>  
> > Most of the tests in my test-suite are reactive.  Meaning, I wrote 
> > them after an ABI-breaking change was reported elsewhere, and I needed 
> > a small test case for bisection purposes.  Thus they are good for 
> > finding if a corner of the perf ABI re-breaks but they're not great at 
> > spotting new breakages.

Would be nice to merge those in into 'perf test' - reactive tests are 
useful as well IMO.

> > Writing a complete test suite for something as complicated as the 
> > perf-event ABI is impractical.  One thing you can do is require anyone 
> > submitting new functionality also provide a regression test, but
> 
> Agreed.
> 
> > I don't see that happening.
> 
> See some of Namhyung, Adrian and Jiri recent patchsets, they came with 
> 'perf test' regression tests.

That's good progress indeed! If we merge in the cases that Vince found 
then we'd have good practical coverage and we could also start requiring 
good testcases for every new ABI extension.

That method distributes the overhead of having to write it to those who 
want to extend (and, statistically speaking, inadvertantlybreak!) the ABI.

Thanks,

	Ingo