Hi Martins,
I've had time to go through the code in detail now - this looks like a
good step forward, and its much simpler than I expected.
----- Original Message -----
> [...]
> This version works, *but* likely has more cleanup to go if this is
> an ok way forward. This is on my hotproc branch on github. I've also
> attached a cumulative patch and git log since over the course of
> development there were a bunch of changes and refactors and the linear
> history might be confusing.
>
> *******
> github.com/ubccr/pcp/tree/hotproc
> *******
Thanks Martins. I've extracted the diff from there, some review notes
follow below. If I can help out with hacking on any of this with you,
just lemme know (especially dynamic metrics, which are a bit quirky).
> Some notes on this implementation:
>
> 1. I tried to keep as much functionality as I could from the sgi version
> but its been quite a while since we ran it and haven't had this hardware
> for quite some time! There are a few things missing that I discuss below.
OK.
> 2. If you recall, the original hotproc pmda used a series of scripts to
> duplicate the proc pmda and then overlay the hotproc functionality as a
> separate pmda. Instead of having a separate pmda, I merged it into
> linux_proc for ease of maintenance. I modified the root_proc file by
> hand for now, but that could likely be done with an awk script as before
> as part of the build process.
I think the approach of merging the two PMDAs has worked well. Since
hotproc was originally written, the dynamic metrics concept has come
along and I think it'll be a better match here than any namespace file
script approach. Some more notes in the review - but, if I can help
with coding this aspect, just let me know (I have some of this fresh
in-mind from other recent hacking).
> 3. What should the default state of hotproc be given that proc is
> usually always running? Should there be some default config file to
It should be available but "off" I think. There is already a pmStore
interface to allow dynamic changes (we should add some access controls
to those, see "have_access" in proc_store - use of that is missing for
the new metrics). In this case, checking for zero uid attribute would
be a good option, I think (in the absence of any more involved ACLs,
which could be added later).
> have some metrics available or should it be disabled (timer doesn't run,
> etc)
> if the user does not provide a config file? I have provided a
Yes, disabled and no-values by default I think.
> sample that is a simplification of what we are currently using.
That should be named sample.conf and hotproc.conf should not
exist by default, since we have no way of knowing up-front what
any individual site might want to use. pmdashping does something
like this, btw, might provide a reference - oh, the old pmdahotproc
Install script implements the same logic too actually. This could
be added into the pmdaproc Install.
> 4. Many of the changes are actually pretty self contained. The biggest
Yeah, its neat - I was a bit surprised at how easily it slotted in.
> change was in proc_pid where I needed to change the assumption that
> there was only one list of processes that we cared about. This resulted
> in having to change a bunch of the functions to take a process list to
> operate on.
*nod*
> 5. Some stats I couldn't find per process yet: syscalls (systemtap seems
> to be the only way to get these?) , and schedwait (doesn't seem to be
> available at all).
Yeah, IRIX had a fair bit more per-process instrumentation.
> 6. idletime is provided by the linux pmda. I just re-implemented this
> small part of code. Is there an alternative way to do inter pmda fetches?
Looks fine to me, other than the minor comments below.
> 7.Some cleanups and error checking still to do. The original code used
> lots of "externs" and I started to factor those out, but then saw use
> in other pmdas so maybe thats not a concern?
Its good to reduce them where it makes sense. Also, the calls to exit(1)
are a concern, from the original PMDA - we should refactor those.
> 8. I realize there is a lot to cleanup yet: commented out old code,
> debugging left on, maybe some memory leaks, etc but wanted to make sure
> there were no show stoppers before I kept going. Also, some of the data
> types might might not be correct, i.e converting some of the long and
> long long types in the original code.
Its progressing nicely, I think, please keep moving ahead with it.
> 9. I'm open to any and all suggestions as having this available makes
> logging proc much more manageable for us. I have it running in a couple
> of VMs right now and will be deploying further over the next week.
Good stuff. Other missing things would be docs and QA testing. The
original man page for pmdahotproc(1) is currently over in man/retired
in the pcp git tree - maybe go through that and pull out the relevant
bits (config format, pmstore interface, etc)?
>From a QA POV, I'm pretty sure there were some hotproc tests originally
- but, I cannot find any currently in the tree. I've sent a note to our
friends at SGI to see if they can help us out there. I'll send through
details once I hear back, but in general we should start thinking about
automated tests now that we have the basics in place, to support the next
rounds of development (and ongoing, into the future, of course).
OK, here's my review notes - let me know if/where I can help out, or if
anything makes no sense.
diff --git a/src/pmdas/linux_proc/GNUmakefile b/src/pmdas/linux_proc/GNUmakefile
index c259bda..4ec9438 100644
--- a/src/pmdas/linux_proc/GNUmakefile
+++ b/src/pmdas/linux_proc/GNUmakefile
+LSRCFILES = help root linux_proc_migrate.conf $(SCRIPTS)
(no need for LSRCFILES anymore)
+root_proc: root_proc.in root_hotproc.in
+ ./build_root_proc.sh
I think it'll be simpler to use dynamic metrics - simplifies the
above step, and...
diff --git a/src/pmdas/linux_proc/build_root_proc.sh
b/src/pmdas/linux_proc/build_root_proc.sh
new file mode 100755
(removes the need for this)
diff --git a/src/pmdas/linux_proc/create_hot_pmns.awk
b/src/pmdas/linux_proc/create_hot_pmns.awk
new file mode 100644
(and this)
diff --git a/src/pmdas/linux_proc/insert_into_root_pmns.awk
b/src/pmdas/linux_proc/insert_into_root_pmns.awk
new file mode 100644
(and this)
diff --git a/src/pmdas/linux_proc/config.c b/src/pmdas/linux_proc/config.c
new file mode 100644
index 0000000..498b5f9
--- /dev/null
+++ b/src/pmdas/linux_proc/config.c
@@ -0,0 +1,561 @@
[...]
+ (void)fprintf(stderr, "%s: Unable to open configuration file \"%s\":
%s\n",
+ pmProgname, hotproc_configfile, osstrerror());
+ exit(1);
This, and quite a few other cases of calling exit() here, should
be reworked to not fail quite so drastically.
+read_test_var(char *line, config_vars *vars)
There's helper code here for testing, we should look into how to
automate its use.
diff --git a/src/pmdas/linux_proc/pmda.c b/src/pmdas/linux_proc/pmda.c
index 1efaabd..ab3710f 100644
--- a/src/pmdas/linux_proc/pmda.c
+++ b/src/pmdas/linux_proc/pmda.c
@@ -63,6 +68,11 @@ char *proc_statspath = ""; /* optional path prefix for all
stats files */
static pmdaIndom indomtab[NUM_INDOMS];
/*
+ * Real metric tab that will be used after we add in hotprocs
+ */
+static pmdaMetric *fullmetrictab;
OK, I think the idea here (fullmetrictab, proc_init_hotproc, and
createHotprocMetric - and the namespace file scripting you talked
about earlier) are duplicating concepts from the dynamic namespace
support already used in the Linux pmdaproc. It seems like we can
fit this in better ... if the [hot]proc.{psinfo,id,memory,io,fd,
schedstat} subtrees become dynamic, they'd be able to share single
definition, help text, and so on.
+ case CLUSTER_HOTPROC_PID_STAT:
+ active_proc_pid = &hotproc_pid;
case CLUSTER_PID_STAT:
(This is cunning - I like this piggy-backing approach!)
@@ -1702,43 +1932,85 @@ proc_store(pmResult *result, pmdaExt *pmda)
[...]
+ case CLUSTER_CONTROL:
[...]
+ case CLUSTER_HOTPROC_GLOBAL:
+ switch(idp->item){
+ case 1: /* Update interval */
+ if ((sts = pmExtractValue(vsp->valfmt, &vsp->vlist[0],
+ PM_TYPE_U32, &av, PM_TYPE_U32)) >= 0) {
+ hotproc_update_interval.tv_sec = av.ul;
+ reset_hotproc_timer();
+ }
+ break;
+ case 8: /* CONFIG */
+ {
+ bool_node *tree = NULL;
+ char *savebuffer;
+ if ((sts = pmExtractValue(vsp->valfmt, &vsp->vlist[0],
+ PM_TYPE_STRING, &av, PM_TYPE_STRING)) >= 0) {
The above is the answer to one of your earlier questions about how
to enable this dynamically, when no initial configuration file exists?
It'd be good to use descriptive macros instead of "1" and "8" here.
+void
+proc_init_hotproc(){
+
+ nmetrics = sizeof(metrictab)/sizeof(metrictab[0]);
+
+ fullmetrictab = (pmdaMetric *) malloc(nmetrics * 2 *
sizeof(pmdaMetric));
+ /* memcopy, then add hotproc, then set nmetrics for use below, then
update all to use fullmetrictab */
Yeah, this is more of the code I mentioned earlier re dynamic metrics
and duplicating concepts implemented there already.
@@ -1871,6 +2249,21 @@ proc_init(pmdaInterface *dp)
indomtab[CGROUP_MOUNTS_INDOM].it_indom = CGROUP_MOUNTS_INDOM;
proc_pid.indom = &indomtab[PROC_INDOM];
+
+ indomtab[HOTPROC_INDOM].it_indom = HOTPROC_INDOM;
+ hotproc_pid.indom = &indomtab[HOTPROC_INDOM];
+
+ char h_configfile[MAXPATHLEN];
+
+ snprintf(h_configfile, sizeof(h_configfile), "%s%c" "proc" "%c"
"hotproc.conf",
+ pmGetConfig("PCP_PMDAS_DIR"), sep, sep);
+
+ //Send this all to a hotproc init config function? TODO
(yep, like cgroups_init I s'pose)
diff --git a/src/pmdas/linux_proc/proc_pid.c b/src/pmdas/linux_proc/proc_pid.c
index c0bab14..6a7e4ec 100644
--- a/src/pmdas/linux_proc/proc_pid.c
+++ b/src/pmdas/linux_proc/proc_pid.c
(there's several different coding styles in this file, makes it
a bit jarring to read for me)
+/* The idea of this is copied from linux/proc_stat.c */
+static unsigned long long
+get_idle_time(){
(this is fine - its a trivial code snippet)
+ FILE * fp = NULL; /* kept open until exit() */
(this comment is incorrect - doesn't match the code)
+ char *linux_statspath = "";
+ if ((envpath = getenv("LINUX_STATSPATH")) != NULL)
+ linux_statspath = envpath;
(this will need to be PROC_STATSPATH in this PMDA)
+ static long hz = -1;
+
+ if (hz == -1){
+ hz = sysconf(_SC_CLK_TCK);
+ num_cpus = sysconf(_SC_NPROCESSORS_ONLN);
+ }
This dup's code in proc_fetchCallBack - could move this to PMDA
init time, and share "hz" to avoid duplicating the calls.
+ /* How do we deal with errors here??? */
Issue __pmNotifyErr warnings and move on is the best option I guess.
+ /* Just copied the old Diff code. Is there a new way to do
this? */
Nope, no new way exists - this looks fine to me.
+ bzero(&newnode->preds, sizeof(newnode->preds));
We tend to use memset-to-zero everywhere else in PCP nowadays.
+ double hptime = (ts.tv_sec - p_timestamp.tv_sec) + (ts.tv_usec -
p_timestamp.tv_usec)/1000000.0;
+ //fprintf(stderr, "Hotproc Update took %f time\n", hptime);
Hide this handy info inside a pmDebug diagnostic branch (uncommented).
cheers.
--
Nathan
|