Re: [PATCH 04/17] tools/arch/x86/pmtctl: Add libpmtctl metric definition database

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 25 May 2026, David E. Box wrote:

> Add a growable container for PMT metric definitions to support loading and
> querying hardware metric descriptors at runtime.
> 
> PMT metrics vary by platform and are discovered dynamically from sysfs. A
> block-based database allows metric definitions to be appended as they are
> parsed, without requiring the final size to be known in advance, while
> bounding individual memory allocations.
> 
> Assisted-by: GitHub-Copilot:claude-sonnet-4.6
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  .../arch/x86/pmtctl/include/lib/metrics_db.h  | 69 +++++++++++++++++++
>  tools/arch/x86/pmtctl/lib/metrics_db.c        | 62 +++++++++++++++++
>  2 files changed, 131 insertions(+)
>  create mode 100644 tools/arch/x86/pmtctl/include/lib/metrics_db.h
>  create mode 100644 tools/arch/x86/pmtctl/lib/metrics_db.c
> 
> diff --git a/tools/arch/x86/pmtctl/include/lib/metrics_db.h b/tools/arch/x86/pmtctl/include/lib/metrics_db.h
> new file mode 100644
> index 000000000000..9dff27d8785a
> --- /dev/null
> +++ b/tools/arch/x86/pmtctl/include/lib/metrics_db.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef PMTCTL_METRICS_DB_H
> +#define PMTCTL_METRICS_DB_H
> +
> +#include <stdbool.h>
> +#include <stddef.h>
> +#include <stdint.h>
> +
> +#include "lib/pmt_guid.h"
> +
> +/* Metric definition from builtin or JSON */
> +struct pmt_metric_def {
> +	const char *event_name;
> +	const char *description;
> +	const char *group;
> +	const char *platform_group;
> +
> +	const struct pmt_guid *guid;
> +	uint32_t sample_id;
> +	uint8_t  lsb;
> +	uint8_t  msb;
> +};
> +
> +struct pmt_metrics_block {
> +	const struct pmt_metric_def *defs;
> +	int count;
> +	bool is_builtin;
> +};
> +
> +struct pmt_metrics_db {
> +	struct pmt_metrics_block *blocks;
> +	int nblocks;
> +	int total;   /* sum of all block->count */
> +};
> +
> +/* Treat DB as a flat array [0..total-1] */
> +static inline const struct pmt_metric_def *pmt_metrics_at(const struct pmt_metrics_db *db, int idx)

inline???

> +{
> +	if (!db || idx < 0 || idx >= db->total)
> +		return NULL;
> +
> +	for (int i = 0; i < db->nblocks; i++) {
> +		if (idx < db->blocks[i].count)
> +			return &db->blocks[i].defs[idx];
> +		idx -= db->blocks[i].count;
> +	}
> +	return NULL;
> +}
> +
> +static inline const struct pmt_metrics_block *
> +pmt_metrics_block_for(const struct pmt_metrics_db *db, int idx)
> +{
> +	if (!db || idx < 0 || idx >= db->total)
> +		return NULL;
> +
> +	for (int i = 0; i < db->nblocks; i++) {
> +		if (idx < db->blocks[i].count)
> +			return &db->blocks[i];
> +		idx -= db->blocks[i].count;
> +	}
> +
> +	return NULL;
> +}

Code duplication.

> +
> +int pmt_metrics_add_block(struct pmt_metrics_db *db, const struct pmt_metric_def *defs,
> +			  int count, bool is_builtin);
> +void pmt_metrics_free(struct pmt_metrics_db *db);
> +
> +#endif
> diff --git a/tools/arch/x86/pmtctl/lib/metrics_db.c b/tools/arch/x86/pmtctl/lib/metrics_db.c
> new file mode 100644
> index 000000000000..82e8121a1b98
> --- /dev/null
> +++ b/tools/arch/x86/pmtctl/lib/metrics_db.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <errno.h>
> +#include <limits.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "lib/metrics_db.h"
> +
> +int pmt_metrics_add_block(struct pmt_metrics_db *db, const struct pmt_metric_def *defs,
> +			  int count, bool is_builtin)
> +{
> +	struct pmt_metrics_block *blocks;
> +	size_t alloc_size;
> +	int new_cap;
> +
> +	if (!db || !defs || count <= 0)
> +		return -EINVAL;
> +
> +	if (db->nblocks > INT_MAX - 1)
> +		return -EOVERFLOW;
> +
> +	if (db->total > INT_MAX - count)
> +		return -EOVERFLOW;
> +
> +	new_cap = db->nblocks + 1;
> +
> +	if ((size_t)new_cap > SIZE_MAX / sizeof(*blocks))

Casts like this likely indicates some typing issue in your code.

> +		return -EOVERFLOW;
> +
> +	alloc_size = (size_t)new_cap * sizeof(*blocks);
> +
> +	blocks = realloc(db->blocks, alloc_size);

So this reallocs + 1 every time??

> +	if (!blocks)
> +		return -ENOMEM;
> +
> +	db->blocks = blocks;
> +
> +	db->blocks[db->nblocks].defs       = defs;
> +	db->blocks[db->nblocks].count      = count;
> +	db->blocks[db->nblocks].is_builtin = is_builtin;
> +
> +	db->nblocks++;
> +	db->total += count;
> +
> +	return 0;
> +}
> +
> +void pmt_metrics_free(struct pmt_metrics_db *db)
> +{
> +	if (!db)
> +		return;
> +
> +	if (db->blocks) {
> +		for (int i = 0; i < db->nblocks; i++) {
> +			if (!db->blocks[i].is_builtin && db->blocks[i].defs)
> +				free((void *)db->blocks[i].defs);
> +		}
> +		free(db->blocks);
> +	}
> +
> +	memset(db, 0, sizeof(*db));

-- 
 i.