Hi -
Looking at lberk's fine commit #7e9b87e49b18 in pcpfans.git
lberk/papi, I was reminded of an earlier idea to make the pmda desc
callback function fully dynamic. This would avoid having to have a
materialized metrictab[] within the pmda code at all, which would
simplify the code, reduce data size, basically sounds like a win/win.
I even prototyped it (patch below), but it doesn't quite work! It
supplies "pminfo -d" and such all right, so clients get nice pmDesc
data, but it kills the ability to do fetches! And that is because
the way pmdaFetch() is implemented in libpcp_pmda: it mandates that
the metric table be materialized into a pmdaMetric table, which it
searches for a pmid->pmDesc entry. All it wants is the pmDesc, but
instead of calling the pmda->desc callback function to get it, it does
a private lookup on a potentially empty or stale table.
I started prototyping a change to pmdaFetch() (patch below) to fall
back to a call into the pmda desc callback function pointer, but
unfortunately pmdaFetch() is not given enough parameters to find it:
only a pmdaExt*, from which it appears impossible to find the
associated pmdaInterface*.
What shall we do about this? It seems an unsatisfactory state of
affairs, to both allow pmdas to provide a custom pmid->pmDesc lookup
function, but then ignore it in the fetch path and instead mandate
a previously-supplied lookup table.
- FChE
--- a/src/libpcp_pmda/src/callback.c
+++ b/src/libpcp_pmda/src/callback.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2013 Red Hat.
+ * Copyright (c) 2013-2014 Red Hat.
* Copyright (c) 1995-2000 Silicon Graphics, Inc. All Rights Reserved.
*
* This library is free software; you can redistribute it and/or modify it
@@ -449,6 +449,7 @@ pmdaFetch(int numpmid, pmID pmidlist[], pmResult **resp,
pmdaExt *pmda)
int inst;
int numval;
pmValueSet *vset;
+ pmDesc dpBuf;
pmDesc *dp;
pmdaMetric *metap;
pmAtomValue atom;
@@ -472,7 +473,12 @@ pmdaFetch(int numpmid, pmID pmidlist[], pmResult **resp,
pmdaExt *pmda)
extp->res->timestamp.tv_usec = 0;
extp->res->numpmid = numpmid;
+ /* Look up the pmDesc for the incoming pmids in our pmdaMetrics tables,
+ if present. Fall back to .desc callback if not found (for highly
+ dynamic pmdas). */
for (i = 0; i < numpmid; i++) {
+ dp = NULL;
+
if (pmda->e_flags & PMDA_EXT_FLAG_HASHED)
metap = __pmdaHashedSearch(pmidlist[i], &extp->hashpmids);
else if (pmda->e_direct)
@@ -480,8 +486,17 @@ pmdaFetch(int numpmid, pmID pmidlist[], pmResult **resp,
pmdaExt *pmda)
else
metap = __pmdaLinearSearch(pmidlist[i], pmda);
- if (metap) {
+ if (metap != NULL)
dp = &(metap->m_desc);
+ else {
+ if (extp->version.any.desc != NULL) { // <--- XXX: inaccessible!!
+ sts = (*(extp->version.any.desc))(pmidlist[i], &dpBuf, pmda);
+ if (sts >= 0)
+ dp = &dpBuf;
+ }
+ }
+
+ if (dp != NULL) {
if (dp->indom != PM_INDOM_NULL) {
/* count instances in the profile */
numval = 0;
@@ -497,7 +512,6 @@ pmdaFetch(int numpmid, pmID pmidlist[], pmResult **resp,
pmdaExt *pmda)
}
}
else {
- dp = NULL;
/* dynamic name metrics may often vanish, avoid log spam */
if (extp->pmda_interface < PMDA_INTERFACE_4) {
char strbuf[20];
--- a/src/pmdas/papi/papi.c
+++ b/src/pmdas/papi/papi.c
@@ -66,8 +66,6 @@ static int refresh_metrics();
static void auto_enable_expiry_cb (int, void *);
static char helppath[MAXPATHLEN];
-static pmdaMetric *metrictab;
-static int nummetrics;
static int
permission_check(int context)
@@ -123,21 +121,6 @@ enlarge_ctxtab(int context)
}
}
-static void
-expand_metric_tab(int size)
-{
- pmUnits units = PMDA_PMUNITS(0,0,0,0, PM_TIME_NSEC, 0);
- size_t need = sizeof(pmdaMetric)*(nummetrics+1);
- metrictab = realloc(metrictab, need);
- if (metrictab == NULL)
- __pmNoMem("metrictab expansion", need, PM_FATAL_ERR);
-
- metrictab[nummetrics-1].m_desc.pmid = papi_info[size].pmid;
- metrictab[nummetrics-1].m_desc.type = PM_TYPE_U64;
- metrictab[nummetrics-1].m_desc.indom = PM_INDOM_NULL;
- metrictab[nummetrics-1].m_desc.sem = PM_SEM_COUNTER;
- metrictab[nummetrics-1].m_desc.units = units;
-}
static int
check_papi_state()
@@ -188,7 +171,7 @@ papi_fetchCallBack(pmdaMetric *mdesc, unsigned int inst,
pmAtomValue *atom)
if (idp->item >= 0 && idp->item <= number_of_events) {
// the 'case' && 'idp->item' value we get is the pmns_position
if (papi_info[idp->item].position >= 0) {
- atom->ull = papi_info[idp->item].prev_value +
values[papi_info[idp->item].position];
+ atom->ll = papi_info[idp->item].prev_value +
values[papi_info[idp->item].position];
// if previously auto-enabled, extend the timeout
if (papi_info[idp->item].metric_enabled !=
METRIC_ENABLED_FOREVER &&
auto_enable_time)
@@ -543,9 +526,60 @@ papi_store(pmResult *result, pmdaExt *pmda)
static int
papi_desc(pmID pmid, pmDesc *desc, pmdaExt *pmda)
{
- return pmdaDesc(pmid, desc, pmda);
+ int sts = PM_ERR_PMID; // presume fail; switch statements fall through to
this
+ __pmID_int *idp = (__pmID_int *)&(pmid);
+
+ switch (idp->cluster) {
+ case CLUSTER_PAPI:
+ desc->pmid = pmid; // needed?
+ desc->type = PM_TYPE_64;
+ desc->indom = PM_INDOM_NULL;
+ desc->sem = PM_SEM_COUNTER;
+ desc->units = (pmUnits) PMDA_PMUNITS(0,0,1,0,0,PM_COUNT_ONE);
+ sts = 0;
+ break;
+
+ case CLUSTER_CONTROL:
+ switch (idp->item) {
+ case 0: // papi.control.enable
+ case 1: // papi.control.reset
+ case 2: // papi.control.disable
+ case 3: // papi.control.status
+ desc->pmid = pmid; // needed?
+ desc->type = PM_TYPE_STRING;
+ desc->indom = PM_INDOM_NULL;
+ desc->sem = PM_SEM_DISCRETE;
+ desc->units = (pmUnits) PMDA_PMUNITS(0,0,0,0,0,0);
+ sts = 0;
+ break;
+ case 4: // papi.control.auto_enable
+ desc->pmid = pmid; // needed?
+ desc->type = PM_TYPE_U32;
+ desc->indom = PM_INDOM_NULL;
+ desc->sem = PM_SEM_DISCRETE;
+ desc->units = (pmUnits) PMDA_PMUNITS(0,1,0,0,PM_TIME_SEC,0);
+ sts = 0;
+ break;
+ }
+ break;
+
+ case CLUSTER_AVAILABLE:
+ switch (idp->item) {
+ case 0: // .available.num_counters
+ desc->pmid = pmid; // needed?
+ desc->type = PM_TYPE_U32;
+ desc->indom = PM_INDOM_NULL;
+ desc->sem = PM_SEM_DISCRETE;
+ desc->units = (pmUnits) PMDA_PMUNITS(0,0,1,0,0,PM_COUNT_ONE);
+ sts = 0;
+ }
+ break;
+ }
+
+ return sts;
}
+
static int
papi_text(int ident, int type, char **buffer, pmdaExt *ep)
{
@@ -563,9 +597,11 @@ papi_text(int ident, int type, char **buffer, pmdaExt *ep)
*buffer = papi_info[pmidp->item].info.long_descr;
return 0;
}
+ /* delegate to "help" file */
return pmdaText(ident, type, buffer, ep);
}
else
+ /* delegate to "help" file */
return pmdaText(ident, type, buffer, ep);
}
@@ -625,8 +661,6 @@ papi_internal_init(pmdaInterface *dp)
memset(&entry[0], 0, sizeof(entry));
papi_info[i].position = -1;
papi_info[i].metric_enabled = 0;
- nummetrics++;
- expand_metric_tab(i);
expand_values(i);
i++;
}
@@ -687,39 +721,7 @@ void
__PMDA_INIT_CALL
papi_init(pmdaInterface *dp)
{
- int i;
int retval;
- pmUnits auto_enable_units = PMDA_PMUNITS(0,1,0,0, PM_TIME_SEC, 0);
- pmUnits other_units = PMDA_PMUNITS(0,0,0,0, PM_TIME_NSEC, 0);
- nummetrics = 6;
- metrictab = malloc(nummetrics*sizeof(pmdaMetric));
- if (metrictab == NULL)
- __pmNoMem("initial metrictab allocation",
(nummetrics*sizeof(pmdaMetric)), PM_FATAL_ERR);
- for(i = 0; i < 6; i++) {
- switch (i) {
- case 4:
- metrictab[i].m_desc.pmid = pmid_build(dp->domain, CLUSTER_CONTROL,
i);
- metrictab[i].m_desc.type = PM_TYPE_U32;
- metrictab[i].m_desc.indom = PM_INDOM_NULL;
- metrictab[i].m_desc.sem = PM_SEM_DISCRETE;
- metrictab[i].m_desc.units = auto_enable_units;
- break;
- case 5:
- metrictab[i].m_desc.pmid = pmid_build(dp->domain,
CLUSTER_AVAILABLE, 0);
- metrictab[i].m_desc.type = PM_TYPE_U32;
- metrictab[i].m_desc.indom = PM_INDOM_NULL;
- metrictab[i].m_desc.sem = PM_SEM_DISCRETE;
- metrictab[i].m_desc.units = other_units;
- break;
- default:
- metrictab[i].m_desc.pmid = pmid_build(dp->domain, CLUSTER_CONTROL,
i);
- metrictab[i].m_desc.type = PM_TYPE_STRING;
- metrictab[i].m_desc.indom = PM_INDOM_NULL;
- metrictab[i].m_desc.sem = PM_SEM_INSTANT;
- metrictab[i].m_desc.units = other_units;
- break;
- }
- }
if (isDSO) {
int sep = __pmPathSeparator();
@@ -755,7 +757,7 @@ papi_init(pmdaInterface *dp)
dp->version.four.children = papi_children;
pmdaSetFetchCallBack(dp, papi_fetchCallBack);
pmdaSetEndContextCallBack(dp, papi_endContextCallBack);
- pmdaInit(dp, NULL, 0, metrictab, nummetrics);
+ pmdaInit(dp, NULL, 0, NULL, 0);
}
static pmLongOptions longopts[] = {
@@ -801,7 +803,6 @@ main(int argc, char **argv)
free(ctxtab);
free(papi_info);
free(values);
- free(metrictab);
exit(0);
}
|