[
[ my reply was intended to go to the list, not just Max ... I've also
[ appended Max's reply and my reply to Max's reply ...
] 8^)>
On Thu, 2010-11-11 at 12:46 +1100, Max Matveev wrote:
> On Thu, 11 Nov 2010 10:49:07 +1100 (EST), nathans wrote:
>
> >> There may be just one PMID and an associated PM_TYPE_STRING value
> >> in cases where the event subsystem produces event records as
> >> structured strings, e.g. XML or JSON encodings.
>
> nathans> I think we should introduce PM_TYPE_JSON and PM_TYPE_XML so that the
> nathans> clients can differentiate these from unstructured strings...
>
> It we're going to do that can we also agree who owns memory allocated
> to hold the string and who is responsible for freeing it? Current idea
> what pmda callback owns the memory makes life harder for dynamically
> allocated strings. In fact, I think adding something
> PM_TYPE_YOUFREE_STRING would make like easier for PMDA writers.
I believe everything that is needed for the PMDA writer is already in
place ...
1. use PM_VAL_DPTR for the valfmt and pmFreeResult will clean up (PMDA
must not retain any reference to the memory once the value is returned
in the fetch callback), or
2. use PM_VAL_SPTR for the valfmt and the PMDA can retain references to
the memory, but the PMDA is responsible for any necessary clean up, when
the options are:
a) install a pmdaResultCallBack method and you'll be called when it is
safe to free the pmValueBlocks under the pmResult (this callback was
designed for _just_ this purpose), or
b) wait for the start of the next fetch when it is safe because fetches
are never nested (simple for singular metrics, else either install a PDU
received callback or have your own fetch() method)
For the event records in the sample PMDA I've used a single buffer with
high-watermark and doubling allocation, and event records are singular
so the simplest option of 2.b) above applies. This is particularly
attractive in this case because calling malloc/realloc to build a packed
array of event records could be horribly expensive.
I think Max's PM_TYPE_YOUFREE_STRING has to apply to a PM_TYPE_STRING
metric where the value has to be accessed via a pmValueBlock, so making
valfmt PM_VAL_DPTR is all that is needed.
If there are some other use cases where PMDAs have trouble freeing
dynamically allocated memory (presumably for the pmValueBlocks of a
pmResult) I'd like to hear the details.
In making the event record changes I've noticed a slight oddity in case
1. above with ... vlen and vtype need to be set outside the call to
__pmStuffValue which is a little strange, but otherwise I think it works
as designed.
Max replied ...
It's mostly happens in pmdaFetchCallback - a punter only gets a
pointer to the atom to fill, there is no way to convey the idea what
memory has been allocated dynamically, so it forces PMDA writer to
deploy your option 2b, often with dramatic consequences - I think
Nate's leak could be blamed on this - see leaks branch on
git://oss.sgi.com/makc/pcp.
And my reply ...
Not withstanding the current issues with the "supersize me" Darwin PMDA,
if the memory pointed to by vp or cp from the pmAtomValue is dynamically
allocated (as an atomic unit, so not part of some larger allocation),
then valfmt of PM_VAL_SPTR is all that is needed. If it is part of
larger allocation then strcpy or malloc/memcpy and make the pmAtomValue
point to that, at which time you can free the underlying allocation.
If it is more complicated than this, like one-shot-per-fetch allocations
that are filled and used to provide values for multiple metrics and/or
multiple instances, then I don't see how the PCP libraries can help and
it is up to the PMDA implementer to get it right (tm).
|