Hi Stan,
----- Original Message -----
> This patch adds infrastructure for dtrace style static probes. These
> are low overhead probes that can be used by, for example, systemtap to
> provide runtime tracing information. A systemtap script which provides
> functionality similar to the current pcp tracing facility is included
> below (pcpprobes.stp).
>
> The patch consists of three parts: configuration support to enable pcp
> to be built with dtrace/systemtap, changes to the tracing call sequence
> to integrate it with dtrace style probing, and runtime support for
> probing.
>
> The configuration changes follow the pattern used by other packages,
> which consists of adding the --enable-dtrace configuration option which
> results in invoking dtrace to build the probes header and object file.
> The nature of these generated files depends on whether dtrace or
> systemtap are being used.
>
Should we expect the patch to build on Mac OS X? OOC, do you know if the
format of the dtrace-generated probes.o is documented someplace? (just
for my own curiousity, not related to this patch).
Misc comments follow...
diff --git a/configure.in b/configure.in
--- a/configure.in
+++ b/configure.in
@@ -210,6 +210,21 @@ AC_ARG_ENABLE([shared],
[PACKAGE_CONFIGURE="$PACKAGE_CONFIGURE --disable-shared=$withval"])
AC_SUBST(enable_shared)
+#
+# DTrace
+#
+AC_ARG_ENABLE([dtrace],
+ [AS_HELP_STRING([--enable-dtrace], [enable DTrace support.])],
+ [PACKAGE_CONFIGURE="$PACKAGE_CONFIGURE --enable-dtrace"])
+#PGAC_ARG_BOOL(enable, dtrace, no,
+# [build with DTrace support],
+AC_CHECK_PROGS(DTRACE, dtrace)
+if test -z "$DTRACE"; then
+ echo "FATAL ERROR: dtrace missing."
+fi
That echo doesn't look very fatal ... and is this doing what it kinda looks
like it might be doing (failing a build with no dtrace?) Shouldn't life /
the build just continue on without dtrace configured in?
diff --git a/src/pmlogconf/tools/localdefs b/src/pmlogconf/tools/localdefs
index 7a6ca09..9c8a7eb 100644
--- a/src/pmlogconf/tools/localdefs
+++ b/src/pmlogconf/tools/localdefs
@@ -1 +1 @@
-FILES = pcp-summary pmclient pmclient-summary pmstat iostat ip mpstat vmstat
+FILES = pcp-summary pmclient pmclient-summary pmstat iostat ip mpstat vmstat
sar
This is unrelated, I'll fix it up in dev - I notice the atop files are missing
as well. And test 366 fails as a result of all this, fixed up as well now in
my local tree, will push it out tomorrow.
diff --git a/src/libpcp_pmcd/src/trace.c b/src/libpcp_pmcd/src/trace.c
index 1631573..01b018f 100644
--- a/src/libpcp_pmcd/src/trace.c
+++ b/src/libpcp_pmcd/src/trace.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1995-2000,2003 Silicon Graphics, Inc. All Rights Reserved.
+ * Copyright (c) 1995-2000,2003,2013 Silicon Graphics, Inc. All Rights
Reserved.
*
Heh - very generous of you. ;)
@@ -65,7 +66,7 @@ pmcd_init_trace(int n)
-pmcd_trace(int type, int who, int p1, int p2)
+pmcd_trace(int trace_p, int type, int who, int p1, int p2)
The name "trace_p" doesn't tell me much - "mask" maybe? Something else?
@@ -76,6 +77,73 @@ pmcd_trace(int type, int who, int p1, int p2)
}
Er, wait - we just allocated the trace buffer unconditionally - that's
not right (line 72, pmcd_trace, trace.c). Thats possibly an unintended
change from current behaviour?, doesn't seem right either way.
switch (type) {
+ case TR_ADD_CLIENT:
+ PROBE_ADD_CLIENT (who, p1, p2);
+ break;
+ case TR_DEL_CLIENT:
+ PROBE_DEL_CLIENT (who, p1, p2);
+ break;
+ case TR_ADD_AGENT:
+ PROBE_ADD_AGENT (who, p1, p2);
+ break;
+ case TR_DEL_AGENT:
+ PROBE_DEL_AGENT (who, p1, p2);
+ break;
+ case TR_EOF:
+ PROBE_AT_EOF (who, p1, p2);
+ break;
+ case TR_WRONG_PDU:
+ PROBE_WRONG_PDU (who, p1, p2);
+ break;
+ case TR_XMIT_ERR:
+ PROBE_XMIT_ERR (who, p1, p2);
+ break;
+ case TR_RECV_TIMEOUT:
+ PROBE_RECV_TIMEOUT (who, p1, p2);
+ break;
+ case TR_RECV_ERR:
+ PROBE_RECV_ERR (who, p1, p2);
+ break;
Hmm, not a big fan of this. I like the one-line pmcd_trace macro instead
of two at each callsite, but this is way more code than I was expecting :(
... isn't this dtrace-fu meant to equate to a noop or two at the callsite?
Can we try TR_-specific macros at the call sites, which expand out to be
the dtrace-macro-or-nothing-if-build-disabled plus the old pmcd_trace call?
(IOW, make this go away using cpp expansion rather than runtime switch)
+ case TR_XMIT_PDU:
+ {
+ __pmID_int *p = (__pmID_int*)&pp2;
+ __pmInDom_int *p = (__pmInDom_int*)&pp2;
All of this decoding should be offloaded to the event handler, not done at the
trace site (pmID and pmInDom are unsigned ints - pass 'em straight through and
decode in stap-land - its trivial bitshifting/masking, copy from pcp headers?).
[ IOW, the above code shouldn't be in pmcd at all. ]
+ case TR_RECV_PDU:
+ PROBE_RECV_PDU (who, p1, p2);
+ }
+
+ if (! trace_p)
+ return;
(having the above if here has changed the old behaviour, as mentioned above).
+ switch (type) {
case TR_XMIT_PDU:
case TR_RECV_PDU:
if ((_pmcd_trace_mask & TR_MASK_PDU) == 0)
diff --git a/src/pmcd/src/GNUmakefile b/src/pmcd/src/GNUmakefile
index 61da1d8..09cd6a6 100644
--- a/src/pmcd/src/GNUmakefile
+++ b/src/pmcd/src/GNUmakefile
@@ -17,9 +17,12 @@ TOPDIR = ../../..
include $(TOPDIR)/src/include/builddefs
CMDTARGET = pmcd$(EXECSUFFIX)
-HFILES = client.h pmcd.h
+HFILES = client.h pmcd.h probes.h
Isn't probes.h generated? - needs to be in LSRCFILES not HFILES if so.
CFILES = pmcd.c config.c dofetch.c dopdus.c dostore.c client.c agent.c util.c
LLDLIBS = $(PCPLIB) $(LIB_FOR_DLOPEN) -lpcp_pmcd
+ifeq ($(enable_dtrace), yes)
+OBJECTS += probes.o
+endif
PCPLIB_LDFLAGS += -L$(TOPDIR)/src/libpcp_pmcd/$(LIBPCP_ABIDIR)
LLDFLAGS += $(RDYNAMIC_FLAG) $(PIELDFLAGS)
@@ -27,6 +30,20 @@ LCFLAGS += $(PIECFLAGS)
default: $(CMDTARGET)
+DIRT += probes.h
+pmcd.c: probes.h
+probes.h: probes.d
+ifeq ($(enable_dtrace), yes)
+ $(DTRACE) -h -s $< -o $@
+else
+ awk '/probe [A-Z]/ {gsub("probe ","PROBE_");gsub(";","");gsub("
*\\(int, int, int.","(i,j,k)");print "#define " $$0}' $< >$@
Hmm, whats this? Do we really need it, given its in the !dtrace path?
Its pretty cryptic whatever it is. If we do need it, PCP_AWK_PROG is
needed here over awk, plus a comment as to what this is all about.
(would an #ifdef DTRACE_ENABLED ... #else ... #endif in a header be an
alternative here?)
diff --git a/src/pmcd/src/config.c b/src/pmcd/src/config.c
index c666889..7a21141 100644
--- a/src/pmcd/src/config.c
+++ b/src/pmcd/src/config.c
@@ -1448,9 +1449,8 @@ DoAgentCreds(AgentInfo* aPtr, __pmPDU *pb)
if ((sts = __pmDecodeCreds(pb, &sender, &credcount, &credlist)) < 0)
return sts;
- else if (_pmcd_trace_mask)
- pmcd_trace(TR_RECV_PDU, aPtr->outFd, sts, (int)((__psint_t)pb &
0xffffffff));
-
+ else
+ pmcd_trace(_pmcd_trace_mask, TR_RECV_PDU, aPtr->outFd, sts,
(int)((__psint_t)pb & 0xffffffff));
No need for an "else"?
for (i = 0; i < credcount; i++) {
switch (credlist[i].c_type) {
case CVERSION:
@@ -1483,8 +1483,7 @@ DoAgentCreds(AgentInfo* aPtr, __pmPDU *pb)
handshake.c_flags = (flags & PDU_FLAG_AUTH);
if ((sts = __pmSendCreds(aPtr->inFd, (int)getpid(), 1, cp)) < 0)
return sts;
- else if (_pmcd_trace_mask)
- pmcd_trace(TR_XMIT_PDU, aPtr->inFd, PDU_CREDS, credcount);
+ else pmcd_trace(_pmcd_trace_mask, TR_XMIT_PDU, aPtr->inFd, PDU_CREDS,
credcount);
No need for an "else"?
@@ -467,8 +466,9 @@ DoFetch(ClientInfo *cip, __pmPDU* pb)
__pmFD_CLR(ap->outFd, &waitFds);
nWait--;
pinpdu = sts = __pmGetPDU(ap->outFd, ANY_SIZE, _pmcd_timeout, &pb);
- if (sts > 0 && _pmcd_trace_mask)
- pmcd_trace(TR_RECV_PDU, ap->outFd, sts, (int)((__psint_t)pb &
0xffffffff));
+ PROBE_RECV_PDU (ap->outFd, sts, (int)((__psint_t)pb & 0xffffffff));
+ if (sts > 0)
+ pmcd_trace(_pmcd_trace_mask, TR_RECV_PDU, ap->outFd, sts,
(int)((__psint_t)pb & 0xffffffff));
Eh? Looks like double tracing. Definitely only want one set of trace macros
at the end of this exercise.
cheers.
--
Nathan
|