PCP: Dynamic columns

[smalinux]

Implements support for arbitrary Performance Co-Pilot
metrics with per-process instance domains to form new
htop columns. The column-to-metric mappings are setup
using configuration files which will be documented via
man pages as part of a follow-up commit.

We provide an initial set of column configurations so
as to provide new capabilities to pcp-htop: including
configs for containers, open fd counts, scheduler run
queue time, tcp/udp bytes/calls sent/recv, delay acct,
virtual machine guests, detailed virtual memory, swap.

Note there is a change to the configuration file path
resolution algorithm introduced for 'dynamic meters'.
First, look in any custom PCP_HTOP_DIR location. Then
iterate, in priority order, users home directory, then
local sysadmins files in /etc/pcp/htop, then readonly
configuration files below /usr/share/pcp/htop. This
final location becomes the preferred place for our own
shipped meter and column files.

The Settings file (htoprc) writing code is updated to
not using the numeric identifier for dynamic columns.
The same strategy used for dynamic meters is used here
where we write Dynamic(name) so the name can be setup
once more at start. Regular (static) columns writing
to htoprc - i.e. numerically indexed - is unchanged.

[lgtm-com]

This pull request introduces 12 alerts when merging 1ee1e4c into 4f3ba68 - view on LGTM.com

new alerts:

• 12 for FIXME comment

[BenBE]

Please fix all those fixme/todo comments.

Also what about having the Custom columns be handled as a sub-call to each platform's WriteColumn function? That way the overall public interface stays minimal.

[lgtm-com]

This pull request introduces 8 alerts when merging 2871e6d into 4f3ba68 - view on LGTM.com

new alerts:

• 8 for FIXME comment

[smalinux]

Just for recording, my friend @a3f gave me a code review here. thank you, @a3f

[natoscott]

Overall it looks good @smalinux - great work. I think the htoprc change mentioned elsewhere is a critical next step before this can be merged, and also the dynamic config tweaks I mention in the review.

I'll send through some more practical dynamic column configuration files shortly.

[smalinux]

@BenBE

./configure --enable-pcp=yes

[BenBE]

@BenBE

./configure --enable-pcp=yes

Exactly not. htop still needs to build and work properly without PCP being enabled. Otherwise there'd be not much sense in having that feature configurable … ;-)

[smalinux]

@natoscott not sure, but I think this related a476490, we need to do the same for columns.

[BenBE]

I'd prefer the following patch instead:

$ git diff
diff --git a/Settings.c b/Settings.c
index 07158e6f..8b366849 100644
--- a/Settings.c
+++ b/Settings.c
@@ -391,7 +391,7 @@ int Settings_write(const Settings* this, bool onCrash) {
 Settings* Settings_new(unsigned int initialCpuCount, Hashtable* dynamicColumns) {
    Settings* this = xCalloc(1, sizeof(Settings));
 
-   this->dynamicColumns = dynamicColumns;
+   this->dynamicColumns = dynamicColumns ? dynamicColumns : Hashtable_new(1, true);
    this->sortKey = PERCENT_CPU;
    this->treeSortKey = PID;
    this->direction = -1;

Much cleaner, less complexity added and offers DiD for NULL-deref issues down the line … The current code for the dynamic columns isn't generally written to cope with dc being NULL. Thus either fix it for every access or just act like an empty list of dynamic columns has been supplied.

[BenBE]

Things for follow-up cleanup:

• The column title alignment code may be updated to use a cache of previously aligned titles for columns. This should be done in conjunction with getting rid of the return of statically allocated module-global memory of those two functions.
• Check if PCPDynamicColumn and PCPDynamicColumns could utilize size_t instead of unsigned int for certain fields.
• Split the PCP Metric API (functions Metric_*) into their own module.

I'm probably missing something, but where's the Metric_enable(FOO, false) call for metrics being removed due to their column being removed in the settings?

[natoscott]

I'd prefer the following patch instead:

I think if we're going this route, the Hashtable allocation should happen earlier (up in CommandLine.c).
Putting it in Settings.c means we have other (NULL) pointers still in CommandLine and ProcessList to deal with, and also means the lifecycle of the Settings.c pointer has two different orbits and knowing the correctl place to free() the pointer is no longer possible.

The allocated-but-empty table is not my preferred solution because its just wasting memory (ok, very small amount) for platforms lacking support - the NULL pointer approach feels slightly more efficient to me. Also if we allocate an empty hash for DynamicColumns we should probably do the same for DynamicMeters, for consistency there I guess.

Much cleaner, less complexity added and offers DiD for NULL-deref issues down the line … The current code for the dynamic columns isn't generally written to cope with dc being NULL. Thus either fix it for every access or just act like an empty list of dynamic columns has been supplied.

There's not a huge number of accesses - I vote we just check for NULL and respond accordingly in each one that's missing the check.

[natoscott]

Things for follow-up cleanup:

* The column title alignment code may be updated to use a cache of previously aligned titles for columns. 

For reference, which source files/lines are you talking about there?

This should be done in conjunction with getting rid of the return of statically allocated module-global memory of those two functions.

@smalinux do you want to hack on this?

* Check if `PCPDynamicColumn` and `PCPDynamicColumns` could utilize `size_t` instead of `unsigned int` for certain fields.

* Split the PCP Metric API (functions `Metric_*`) into their own module.

I'll take these two items on. I'll do 'em as followup commits I think because they're likely to introduce patch conflicts with other PRs.

I'm probably missing something, but where's the Metric_enable(FOO, false) call for metrics being removed due to their column being removed in the settings?

There isn't one - @smalinux do you know where that call would need to be? I figured it wasn't a big deal to keep sampling one or two metrics we're not using anymore after interactive changes, but we should switch 'em off if we can.

[BenBE]

@natoscott FYI: The minimalistic patch regarding Settings.c for the Hashtable was kinda retracted and that check placed in CommandLine.c instead. Cf. https://github.com/htop-dev/htop/compare/a0fefb6d7330539c73a3b10e13dbe96a5c4b3218..cf6d0ff4dad01747f668970697d1d0ea36a816de

I'd rather not follow the NULL approach for now, as this will lead to extra places to take care of possible NULL pointers where up to now we usually don't have this expectation in the code for our central structures.

But I'm fine if you make the dynamicMeters member non-NULL too for consistency too …

k, regarding header alignment and static buffers: I referred to alignedDynamicColumnTitle in ProcessList.c.

[natoscott]

@natoscott FYI: The minimalistic patch regarding Settings.c for the Hashtable was kinda retracted and that check placed in CommandLine.c instead. Cf. https://github.com/htop-dev/htop/compare/a0fefb6d7330539c73a3b10e13dbe96a5c4b3218..cf6d0ff4dad01747f668970697d1d0ea36a816de

OK, fair enough.

I'd rather not follow the NULL approach for now, as this will lead to extra places to take care of possible NULL pointers where up to now we usually don't have this expectation in the code for our central structures.

But I'm fine if you make the dynamicMeters member non-NULL too for consistency too …

OK, let's go that route then.

k, regarding header alignment and static buffers: I referred to alignedDynamicColumnTitle in ProcessList.c.

Taa.

[smalinux]

@natoscott

I'm probably missing something, but where's the Metric_enable(FOO, false) call for metrics being removed due to their column being removed in the settings?

There isn't one - @smalinux do you know where that call would need to be? I figured it wasn't a big deal to keep sampling one or two metrics we're not using anymore after interactive changes, but we should switch 'em off if we can.

• Columns:

  htop/AvailableColumnsPanel.c

  Line 25 in 6974ce8

  static void AvailableColumnsPanel_delete(Object* object) {

• Meters:

  htop/AvailableMetersPanel.c

  Line 27 in 6974ce8

  static void AvailableMetersPanel_delete(Object* object) {

[BenBE]

LGTM. Didn't run it yet with those latest changes …

[BenBE]

We're getting there …

[BenBE]

Okay. LGTM.