Re: [pcp] systemtap/pcp integration

[Nathan Scott <nathans@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2458 bytes --]

Hi David,

----- Original Message -----
> [...]
> Note that systemtap will create a file called 'mmv' in
> /proc/systemtap/{MODULE_NAME}. I've just been using pcp's 'mmvdump'
> utility to dump the contents of the /proc/systemtap/{MODULE_NAME}/mmv
> file. Currently the pcp mmv pmda only looks in one place for mmv files,
> but it might be possible to create a symbolic link to systemtap's mmv
> file to make it happy.

(OOC, what's {MODULE_NAME} in this context?)

A symlink would sorta work but it feels like a bit of a workaround - the
PMDA is written to be able to detect arrival/departure of new MMV files
based on changes in a directory (and the location of that directory is
parameterised via /etc/pcp.conf variables).  I'd not recommend trying to
find it within a stap script ... I imagine it will be cleaner if we go
for separate user/kernel locations for MMV files.

Attached patch (lightly tested) implements the PCP side of things with
that in mind - with this patch (and making the kernel code manage the
lifecycle of separate /proc/mmv/* entries), things should begin to work
out-of-the-box (the MMV PMDA is already default-enabled in the default
pmcd config file, so everything else is in place for you).

It also occurs to me that there's not really anything systemtap-specific
about the kernel MMV instrumentation you've done, or is there?  A device
driver author might want to instrument her code, for example, without 
needing a stap install/script...?  It would be worth thinking about if
the MMV bits in your script could become a more general kernel module
for others to use too (in addition to stap, of course!).  The extremely
lightweight nature of the interface is such that it can be permanently
enabled (that's certainly how the userspace MMV is used).

Nice work anyway ... I'm looking forward to seeing the code in wider use.


Oh, to use the attached patch with PCP tools - apply to a "dev" PCP tree
(git.performancecopilot.org/pcp), build, cd src/pmdas/mmv/src, and ...

$ pminfo -L -Kclear -Kadd,70,./pmda_mmv.so,mmv_init mmv

... will serve as a quick sanity test that the metrics are functioning.
You can also use gdb on /usr/bin/pminfo to inspect behaviour of the PMDA.

Once thats going, installing it properly ('make install') and restarting
the pmcd service should get you a fully-functional PCP setup with your
kernel code firing on all cylinders (pmchart, pmie, everything will work
at that point).

cheers.

--
Nathan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: proc-mmv.patch --]
[-- Type: text/x-patch; name=proc-mmv.patch, Size: 5538 bytes --]

diff --git a/src/pmdas/mmv/src/mmv.c b/src/pmdas/mmv/src/mmv.c
index f3ef26f..92cd4e1 100644
--- a/src/pmdas/mmv/src/mmv.c
+++ b/src/pmdas/mmv/src/mmv.c
@@ -42,15 +42,21 @@ static int incnt;
 
 static int reload;
 static __pmnsTree * pmns;
-static int statsdir_code;		/* last statsdir stat code */
-static time_t statsdir_ts;		/* last statsdir timestamp */
 static char * prefix = "mmv";
 
 static char * pcptmpdir;		/* probably /var/tmp */
 static char * pcpvardir;		/* probably /var/pcp */
 static char * pcppmdasdir;		/* probably /var/pcp/pmdas */
 static char pmnsdir[MAXPATHLEN];	/* pcpvardir/pmns */
-static char statsdir[MAXPATHLEN];	/* pcptmpdir/<prefix> */
+
+typedef struct {
+    char	path[MAXPATHLEN];
+    int		status;			/* last statsdir stat return code */
+    time_t	timestamp;		/* last statsdir modify timestamp */
+} statsdir_t;
+
+static statsdir_t sdirlist[2];		/* userspace and kernel MMV files */
+#define SDIRCOUNT (sizeof(sdirlist)/sizeof(sdirlist[0]))
 
 typedef struct {
     char *	name;			/* strdup client name */
@@ -364,13 +370,44 @@ create_indom(pmdaExt *pmda, stats_t *s, mmv_disk_indom_t *id, pmInDom indom)
     return 0;
 }
 
+static int
+create_client_statdir(statsdir_t *statsdir)
+{
+    struct dirent **files;
+    int need_reload = 0;
+    int i, num;
+
+    num = scandir(statsdir->path, &files, NULL, NULL);
+    for (i = 0; i < num; i++) {
+	struct stat statbuf;
+	char path[MAXPATHLEN];
+	char *client;
+
+	if (files[i]->d_name[0] == '.')
+	    continue;
+
+	client = files[i]->d_name;
+	sprintf(path, "%s%c%s", statsdir->path, __pmPathSeparator(), client);
+
+	if (stat(path, &statbuf) >= 0 && S_ISREG(statbuf.st_mode))
+	    if (create_client_stat(client, path, statbuf.st_size) == -EAGAIN)
+		need_reload = 1;
+    }
+
+    for (i = 0; i < num; i++)
+	free(files[i]);
+    if (num > 0)
+	free(files);
+
+    return need_reload;
+}
+
 static void
 map_stats(pmdaExt *pmda)
 {
-    struct dirent **files;
     char name[64];
     int need_reload = 0;
-    int i, j, k, sts, num;
+    int i, j, k, sts;
 
     if (pmns)
 	__pmFreePMNS(pmns);
@@ -407,27 +444,8 @@ map_stats(pmdaExt *pmda)
 	scnt = 0;
     }
 
-    num = scandir(statsdir, &files, NULL, NULL);
-    for (i = 0; i < num; i++) {
-	struct stat statbuf;
-	char path[MAXPATHLEN];
-	char *client;
-
-	if (files[i]->d_name[0] == '.')
-	    continue;
-
-	client = files[i]->d_name;
-	sprintf(path, "%s%c%s", statsdir, __pmPathSeparator(), client);
-
-	if (stat(path, &statbuf) >= 0 && S_ISREG(statbuf.st_mode))
-	    if (create_client_stat(client, path, statbuf.st_size) == -EAGAIN)
-		need_reload = 1;
-    }
-
-    for (i = 0; i < num; i++)
-	free(files[i]);
-    if (num > 0)
-	free(files);
+    for (i = 0; i < SDIRCOUNT; i++)
+	need_reload += create_client_statdir(&sdirlist[i]);
 
     for (i = 0; slist && i < scnt; i++) {
 	stats_t * s = slist + i;
@@ -609,11 +627,38 @@ mmv_fetchCallBack(pmdaMetric *mdesc, unsigned int inst, pmAtomValue *atom)
     return 0;
 }
 
+static int
+check_time_statsdir(statsdir_t *statsdir)
+{
+    struct stat s;
+    int need_reload = 0;
+
+    /*
+     * check if the directory has been modified, reload if so;
+     * note modification may involve removal or newly appeared,
+     * a change in permissions from accessible to not (or vice-
+     * versa), and so on.
+     */
+    if (stat(statsdir->path, &s) >= 0) {
+	if (s.st_mtime != statsdir->timestamp) {
+	    need_reload++;
+	    statsdir->status = 0;
+	    statsdir->timestamp = s.st_mtime;
+	}
+    } else {
+	if (statsdir->status != oserror()) {
+	    statsdir->status = oserror();
+	    statsdir->timestamp = 0;
+	    need_reload++;
+	}
+    }
+    return need_reload;
+}
+
 static void
 mmv_reload_maybe(pmdaExt *pmda)
 {
     int i;
-    struct stat s;
     int need_reload = reload;
 
     /* check if generation numbers changed or monitored process exited */
@@ -629,26 +674,8 @@ mmv_reload_maybe(pmdaExt *pmda)
 	}
     }
 
-    /*
-     * check if the directory has been modified, reload if so;
-     * note modification may involve removal or newly appeared,
-     * a change in permissions from accessible to not (or vice-
-     * versa), and so on.
-     */
-    if (stat(statsdir, &s) >= 0) {
-	if (s.st_mtime != statsdir_ts) {
-	    need_reload++;
-	    statsdir_code = 0;
-	    statsdir_ts = s.st_mtime;
-	}
-    } else {
-	i = oserror();
-	if (statsdir_code != i) {
-	    statsdir_code = i;
-	    statsdir_ts = 0;
-	    need_reload++;
-	}
-    }
+    for (i = 0; i < SDIRCOUNT; i++)
+	need_reload += check_time_statsdir(&sdirlist[i]);
 
     if (need_reload) {
 	if (pmDebug & DBG_TRACE_APPL0)
@@ -808,6 +835,7 @@ mmv_init(pmdaInterface *dp)
 {
     int	m;
     int sep = __pmPathSeparator();
+    char *procdir;
 
     if (isDSO) {
 	pmdaDSO(dp, PMDA_INTERFACE_4, "mmv", NULL);
@@ -818,12 +846,15 @@ mmv_init(pmdaInterface *dp)
     pcptmpdir = pmGetConfig("PCP_TMP_DIR");
     pcpvardir = pmGetConfig("PCP_VAR_DIR");
     pcppmdasdir = pmGetConfig("PCP_PMDAS_DIR");
+    if ((procdir = getenv("PROC_STATSPATH")) == NULL)
+	procdir = "/proc";
 
-    snprintf(statsdir, sizeof(statsdir), "%s%c%s", pcptmpdir, sep, prefix);
     snprintf(pmnsdir, sizeof(pmnsdir), "%s%c" "pmns", pcpvardir, sep);
-    statsdir[sizeof(statsdir)-1] = '\0';
     pmnsdir[sizeof(pmnsdir)-1] = '\0';
 
+    snprintf(sdirlist[0].path, MAXPATHLEN-1, "%s%c%s", pcptmpdir, sep, prefix);
+    snprintf(sdirlist[1].path, MAXPATHLEN-1, "%s%c%s", procdir, sep, prefix);
+
     /* Initialize internal dispatch table */
     if (dp->status == 0) {
 	/*