On (03/06/16 12:44), Jakub Hrozek wrote: >On Mon, May 09, 2016 at 10:04:37AM +0200, Jakub Hrozek wrote: >> Hi, >> >> the attached patches are the first self-contained part of my performance >> work. Using them, I analyzed the performance of 'id' as the worst-case, >> then realized most of the issues are around processing and storing large >> groups. The results gathered using these scripts are in fact the base of >> the improvements documented at: >> >> https://fedorahosted.org/sssd/wiki/DesignDocs/OneFourteenPerformanceImprovements >> >> I tried to add descriptive commit messages to the patches so that hopefully >> all the info is in those commit messages. We will probably add more probes >> into different areas of code, but these are the ones that I already haven't >> modified for weeks, so I think it should be safe to merge them. >> >> The probes are enabled in our RPMs and I would encourage downstreams to >> enable them as well. There is virtually no cost of having the probes >> compiled in by default and the benefit would be that admins can run them >> to see what are the bottlenecks in their environment. I was also >> wondering if it makes sense to add a generic back-end-request tracking >> probes and an associated script, but since the data provider is being >> re-factored at the moment, I think it would be better to land the DP >> changes first. >> >> The reason I split the sysdb probes into several commits (unlike the LDAP >> probes) is that I think in future, we can use these commits as a >> reference for adding more probes to different areas of SSSD. >> >> CI: http://sssd-ci.duckdns.org/logs/job/42/99/summary.html >> >> (BTW, thanks to Lukas for helping me a lot with the build failures on >> #sssd last week) > >Hi, > >the attached patches just fix a Coverity issue in one of the patches.
>From 2e6de9cf0278525e61af915220b8bdcf667abb13 Mon Sep 17 00:00:00 2001 >From: Jakub Hrozek <[email protected]> >Date: Mon, 29 Feb 2016 13:05:59 +0100 >Subject: [PATCH 02/10] BUILD: Add build infrastructure for systemtap scripts > >Adds infrastructure that generatest the probes.h and probes.o from the >dtrace probes.d file. The probes.d file is empty except for the provider >name in this commit, its content will be added with later commits that >actually add some content. The probes.d file is always distributed in >the tarball so that distributions can optionally enable systemtap >support. > >The generation is done using the "dtrace" command because the probes.d file >is compatible with the Solaris dtrace format. Please see "man 1 dtrace" >for more information on the dtrace format and the command line tool. > >In order to make libtool happy, a fake libtool object is generated. This >hunk was taken from the libvirt code. > >The AM_V_GEN macro is used to make the build compatible with the silent >build configuration. > >To enable systemtap probing, configure sssd with: > --enable-systemtap >In order to do so, the 'dtrace' command-line utility must be installed. >On Fedora and RHEL, this package is installed as part of the >"systemtap-sdt-devel" package. > >You'll also want the 'systemtap' package installed as well as the matching >versions of kernel-devel and kernel-debuginfo on your machine. >--- > Makefile.am | 43 +++++++++++++++++++++++++++++++++++++++++-- > configure.ac | 4 ++++ > src/external/systemtap.m4 | 35 +++++++++++++++++++++++++++++++++++ > src/systemtap/sssd_probes.d | 2 ++ > src/tests/cwrap/Makefile.am | 6 ++++++ > src/util/probes.h | 2 ++ > 6 files changed, 90 insertions(+), 2 deletions(-) > create mode 100644 src/external/systemtap.m4 > create mode 100644 src/systemtap/sssd_probes.d > //snip >diff --git a/src/external/systemtap.m4 b/src/external/systemtap.m4 >new file mode 100644 >index >0000000000000000000000000000000000000000..d1caa2017f0730394339f0a439046df6b56cb2ba >--- /dev/null >+++ b/src/external/systemtap.m4 >@@ -0,0 +1,35 @@ >+dnl A macro to check the availability of systemtap user-space probes >+AC_DEFUN([AM_CHECK_SYSTEMTAP], >+[ >+ AC_ARG_ENABLE([systemtap], >+ [AS_HELP_STRING([--enable-systemtap], >+ [Enable inclusion of systemtap trace >support])], >+ [ENABLE_SYSTEMTAP="${enableval}"], [ENABLE_SYSTEMTAP='no']) >+ >+ if test "x${ENABLE_SYSTEMTAP}" = xyes; then >+ AC_CHECK_PROGS(DTRACE, dtrace) >+ if test -z "$DTRACE"; then >+ AC_MSG_ERROR([dtrace not found]) >+ fi >+ >+ AC_CHECK_HEADER([sys/sdt.h], [SDT_H_FOUND='yes'], ^^^^^^^^^^^ this variable is not used anywhere. it would be better to use default action "[]" >+ [SDT_H_FOUND='no'; >+ AC_MSG_ERROR([systemtap support needs sys/sdt.h >header])]) e.g. AC_CHECK_HEADERS([check.h],,AC_MSG_ERROR([Could not find CHECK headers])) >+ >+ AC_DEFINE([HAVE_SYSTEMTAP], [1], [Define to 1 if systemtap is >enabled]) >+ HAVE_SYSTEMTAP=1 ^^^^^^^^^^^^^^ it should already be set to 1 by AC_DEFINE. >+ >+ AC_ARG_WITH([tapset-install-dir], >+ [AS_HELP_STRING([--with-tapset-install-dir], ^ It would be good to append here string "=DIR" So it will be clear that there is expected argument We already have it for "--with-systemdunitdir" >+ [The absolute path where the tapset dir >will be installed])], >+ [if test "x${withval}" = x; then >+ tapset_dir="\$(datadir)/systemtap/tapset" >+ else >+ tapset_dir="${withval}" >+ fi], It can be simplified to 'tapset_dir="${withval}"' after updating help string. >+ [tapset_dir="\$(datadir)/systemtap/tapset"]) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this default value could be mentioned in help string as well. @see output of ./configure --help (with-pubconf-path, with-pipe-path ...) >+ AC_SUBST(tapset_dir) >+ fi >+ >+ AM_CONDITIONAL([BUILD_SYSTEMTAP], [test x$HAVE_SYSTEMTAP = x1]) >+]) >diff --git a/src/systemtap/sssd_probes.d b/src/systemtap/sssd_probes.d >new file mode 100644 >index >0000000000000000000000000000000000000000..7579577c8661e862d83b9fc7c9fe205d25be30ed >--- /dev/null >+++ b/src/systemtap/sssd_probes.d >@@ -0,0 +1,2 @@ >+provider sssd { >+} >diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am >index >a5afb8c020ee00b680aac0680bcf6f6495dd8222..abf50a56f3a9b5e24759e819b24ba01d65192f37 > 100644 >--- a/src/tests/cwrap/Makefile.am >+++ b/src/tests/cwrap/Makefile.am >@@ -88,6 +88,9 @@ server_tests_LDADD = \ > $(abs_top_builddir)/libsss_debug.la \ > $(abs_top_builddir)/libsss_test_common.la \ > $(NULL) >+if BUILD_SYSTEMTAP >+server_tests_LDADD += $(abs_top_builddir)/stap_generated_probes.lo >+endif > > usertools_tests_SOURCES = \ > test_usertools.c \ >@@ -103,6 +106,9 @@ usertools_tests_LDADD = \ > $(abs_top_builddir)/libsss_debug.la \ > $(abs_top_builddir)/libsss_test_common.la \ > $(NULL) >+if BUILD_SYSTEMTAP >+usertools_tests_LDADD += $(abs_top_builddir)/stap_generated_probes.lo >+endif > > responder_common_tests_SOURCES =\ > test_responder_common.c \ >diff --git a/src/util/probes.h b/src/util/probes.h >index >349f29086e86a51b4551f09f542d0f45bf6f1aae..effce492d0a6cbcfe5b4c347b8931e102b853db2 > 100644 >--- a/src/util/probes.h >+++ b/src/util/probes.h >@@ -20,6 +20,8 @@ > > #ifdef HAVE_SYSTEMTAP > >+#include "stap_generated_probes.h" >+ > /* Probe expansion inspired by libvirt */ > #define PROBE_EXPAND(NAME, ...) NAME(__VA_ARGS__) > >-- >2.4.11 > >From c1a4fba84679bb4ce10badcc9458088e4a94d281 Mon Sep 17 00:00:00 2001 >From: Jakub Hrozek <[email protected]> >Date: Mon, 29 Feb 2016 13:20:28 +0100 >Subject: [PATCH 04/10] SYSDB: Add systemtap probes to track sysdb transactions > >Actually adds marks for sysdb transactions that receive the transaction >nesting level as an argument. The nesting is passed on from probes to >marks along with a human-friendly description. > >The transaction commit is decorated with two probes, before and after. >This would allow the caller to distinguish between the time we spend in >the transaction (which might be important, because if a transaction is >active on an ldb context, even the readers are blocked before the >transaction completes) and the time we spend commiting the transaction >(which is important because that's when the disk writes occur) > >The probes would be installed into /usr/share/systemtap/tapset on RHEL >and Fedora. This is in line with systemtap's paths which are described >in detail in "man 7 stappaths". >--- > Makefile.am | 4 +++- > configure.ac | 1 + > src/db/sysdb.c | 8 ++++++++ > src/systemtap/sssd.stp.in | 32 ++++++++++++++++++++++++++++++++ > src/systemtap/sssd_probes.d | 4 ++++ > 5 files changed, 48 insertions(+), 1 deletion(-) > create mode 100644 src/systemtap/sssd.stp.in > >diff --git a/Makefile.am b/Makefile.am >index >33930759ab82b65643a9a0a071fd92b025dab145..1b4ba3cc651b29c55f7b43c90bc479f584a911e2 > 100644 >--- a/Makefile.am >+++ b/Makefile.am >@@ -81,6 +81,7 @@ krb5rcachedir = @krb5rcachedir@ > sudolibdir = @sudolibpath@ > polkitdir = @polkitdir@ > pamconfdir = $(sysconfdir)/pam.d >+systemtap_tapdir = @tapset_dir@ > > UNICODE_LIBS=@UNICODE_LIBS@ > >@@ -1081,6 +1082,8 @@ SYSTEMTAP_PROBES = \ > $(srcdir)/src/systemtap/sssd_probes.d \ > $(NULL) > >+dist_systemtap_tap_DATA = $(builddir)/src/systemtap/sssd.stp >+ Do we need to install this file if sssd is not build with systemtap support? The same applies to other patches and automake variables dist_systemtap_tap_DATA, dist_sssdtapscript_DATA It caused mock failures RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_client.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_tools.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ldap.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_krb5.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ipa.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ad.lang Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp Lang files might be unrelated but there are *.stp files in output. http://sssd-ci.duckdns.org/logs/job/44/50/summary.html It works after applying the last patch from patchset. > int sysdb_transaction_commit(struct sysdb_ctx *sysdb) > { > int ret; >+#ifdef HAVE_SYSTEMTAP >+ int commit_nesting = sysdb->transaction_nesting-1; ^^ missing spaces :-) >+#endif >From def7601820c687c55db87136f77ce1cd06affa29 Mon Sep 17 00:00:00 2001 >From: Jakub Hrozek <[email protected]> >Date: Fri, 6 May 2016 15:51:12 +0200 >Subject: [PATCH 10/10] BUILD: Enable systemtap during RPM build and CI > >So far, systemtap is only enabled for Red Hat distributions. >--- > contrib/ci/configure.sh | 6 ++++++ > contrib/sssd.spec.in | 19 +++++++++++++++++++ > 2 files changed, 25 insertions(+) > >diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh >index >c850eb9ce9a4228c1a89b8b2b49311e1c748b7de..823d3277c0467d1abf0e746c910d2a0e16783693 > 100644 >--- a/contrib/ci/configure.sh >+++ b/contrib/ci/configure.sh >@@ -47,6 +47,12 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || > ) > fi > >+if [[ "$DISTRO_FAMILY" == "redhat" ]]; then >+ CONFIGURE_ARG_LIST+=( >+ "--enable-systemtap" >+ ) >+fi >+ Maybe we can enable a build also on debian with installed package systemtap-sdt-dev (I didn't try) I briefly look at system tap script and they looks good to me. some lines are longer than 80 characters :-) and there could be more comments but it's not a blocker. LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
