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/OneFourteenPerformanceImpro...
>
> 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 <jhrozek(a)redhat.com>
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 <jhrozek(a)redhat.com>
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 <jhrozek(a)redhat.com>
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