[collectd] [PATCH] lpar plugin update

[Florian Forster]

Hey Aurélien,

On Sat, Sep 11, 2010 at 03:00:28PM +0200, Aurélien Reynaud wrote:
> No problem, I've been throught this myself! Your will to thoroughly
> understand every part of the code is IMHO the main reason behind
> collectd's excellent quality.

yeah, and now I'm craving for a POWER7-based system and am beginning to
think that 10,000 EUR for a server is "cheap" and it's all your fault! ;)

> > > - the "consumed" metric might seem superfluous at first sight […]
> > 
> > Would it make sense to activate this metric only if the partition is
> > a dedicated partition and donations have been enabled? Likewise,
> > would it make sense to submit "entitled" capacity only if the
> > partition is a shared partition?
> […]
> 'consumed' is a generic concept, materializing the hardware processing
> power really allocated to the partition by the hypervisor. 
> 
> Be it a shared or dedicated partition, comparing this value to the
> hard/soft limits you configured (entitlement, capping, donation) is
> possibly what admins care the most about. Many will be satisfied with
> just having the 'entitled' and 'consumed' metrics.

You're probably right. I'm sure "my admins" would threaten me in the
most dreadful ways [*] if I was about to remove anything I/O specific
from a plugin such as this, though ;)

> So maybe we could collect only entitled and consumed by default, and
> provide some 'Details = true/false' option to enable the collection of
> the different processor states.

Would be okay for me. Power to the user ;)

> > > I posted a fix ("Fix errno thread-safety under AIX") [...]
> > I applied the fix to the collectd-4.9 branch and will merge it to
> > master eventually.
> Thanks.

No, thank you for figuring this out :) I didn't find anything useful on
"_THREAD_SAFE", by the way, other than something along the lines of
"your thread provider should set this". So I'm assuming that an equally
well solution would be to include <pthread.h> before <errno.h>. Just a
guess, though.

> > I'd prefer to account "busy" and "used" (non-busy) rather than
> > "busy" and "total". Do you see any problem with changing that?
> 
> None. I can provide a patch, unless you want to code it yourself. I
> just find a little confusing to have busy/used as metrics. Maybe
> busy/idle or used/unused would be more straightforward?

Oh, "used" was a typo. I meant "idle", of course. The reason for this
(and to some degree, the "consumed" / "entitled" business above) is that
I prefer each tick to be accounted for *once*. So the busy ticks would
be included in both, the "busy" metric and the "total" metric. When both
are equally available (or trivially computable) I prefer used/unused
over used/total.

I'll probably implement this tonight, unless you beat me to it. It
should basically be one subtraction and a changed string .. ;)

> >   memcpy (&lparstats_old, lparstats_new, sizeof (lparstats_old));
> 
> Fine with me. Just need to make sure no struct members are pointers to
> other structs.

Currently only the counters and the time_base (or base_time?), which is
another counter, are required. I think a comment along the lines of
"don't use pointers from this struct" should be sufficient. But I doubt
that there are pointers in the struct -- we never need to free it, so
the called code wouldn't know when it was safe to free the pointed to
data.

> IMHO the implementation requiring the fewest lines of code will be the
> best here...

My thinking .. We save ~10 global variables and updating the state
becomes a trivial "memcpy".

Regards,
--octo

[*] Like cutting off my caffeine supply.
-- 
Florian octo Forster
Hacker in training
GnuPG: 0x0C705A15
http://octo.it/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: Digital signature
URL: <http://mailman.verplant.org/pipermail/collectd/attachments/20100911/a15b2a5e/attachment.pgp>