[PATCH v4 0/4] xentop: add physical CPU usage support

[PATCH v4 0/4] xentop: add physical CPU usage support

[Jahan Murudi]

This is v4 of the patch series to add physical CPU monitoring to xentop.

Changes since v3:
-   Split the single large patch into a logical series of 3 smaller patches
    for easier review.
-   Fixed the memory allocation error handling in pcpu.c as pointed out by
    Anthony PERARD. The realloc() result is now assigned immediately to avoid
    invalid pointers on failure.
-   Simplified the time calculation by using a single global timestamp instead
    of a per-CPU array, as the time difference is the same for all cores.
-   Removed the unnecessary check for small time intervals which could lead to
    incorrect usage calculations.
-   Integrated the PCPU display with the existing print() function to ensure
    correct behavior in both interactive and batch modes.

The series adds a new '-p'/'--pcpus' flag to xentop. When enabled, it displays
a table showing the usage percentage of each physical CPU core in the system.

Jahan Murudi (4):
  xentop: add pcpu header and basic infrastructure
  xentop: add pcpu implementation with proper error handling
  xentop: update Makefile to link against libxenctrl
  xentop: integrate pcpu support into main program


 tools/xentop/Makefile |   5 +-
 tools/xentop/pcpu.c   | 141 ++++++++++++++++++++++++++++++++++++++++++
 tools/xentop/pcpu.h   |  17 +++++
 tools/xentop/xentop.c |  79 +++++++++++++++++------
 4 files changed, 222 insertions(+), 20 deletions(-)
 create mode 100644 tools/xentop/pcpu.c
 create mode 100644 tools/xentop/pcpu.h

-- 
2.34.1

[PATCH v4 1/4] xentop: add pcpu header and basic infrastructure

[Jahan Murudi]

Introduce a new header (pcpu.h) which defines the interfaces
for physical CPU statistics collection. This provides the
basic infrastructure needed to track per-CPU usage and will
be used in subsequent patches.

Signed-off-by: Jahan Murudi <jahan.murudi.zg@renesas.com>
---
 tools/xentop/pcpu.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 tools/xentop/pcpu.h

diff --git a/tools/xentop/pcpu.h b/tools/xentop/pcpu.h
new file mode 100644
index 0000000000..a528177476
--- /dev/null
+++ b/tools/xentop/pcpu.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2025 Renesas Electronics Corporation
+ */
+
+#ifndef __XENTOP_PCPU_H__
+#define __XENTOP_PCPU_H__
+
+#include <sys/time.h>
+
+int update_pcpu_stats(const struct timeval *now, unsigned int delay);
+int get_pcpu_count(void);
+float get_pcpu_usage(int cpu_index);
+int has_pcpu_data(void);
+void free_pcpu_stats(void);
+
+#endif /* __XENTOP_PCPU_H__ */
-- 
2.34.1

[PATCH v4 2/4] xentop: add pcpu implementation with proper error handling

[Jahan Murudi]

Introduce pcpu.c which implements physical CPU statistics
collection using xc_getcpuinfo(). The code handles allocation
and reallocation of statistics buffers, maintains previous
idle times, and calculates usage percentages.

Proper error handling and cleanup are provided to ensure
robustness in the face of allocation or hypervisor API
failures.

Signed-off-by: Jahan Murudi <jahan.murudi.zg@renesas.com>
---
 tools/xentop/pcpu.c | 141 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 141 insertions(+)
 create mode 100644 tools/xentop/pcpu.c

diff --git a/tools/xentop/pcpu.c b/tools/xentop/pcpu.c
new file mode 100644
index 0000000000..cdb4cb2131
--- /dev/null
+++ b/tools/xentop/pcpu.c
@@ -0,0 +1,141 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2025 Renesas Electronics Corporation
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+#include <sys/time.h>
+#include <xenctrl.h>
+#include "pcpu.h"
+
+#define MAX_PCPUS 128
+
+typedef struct {
+    float usage_pct;
+} pcpu_stat_t;
+
+static pcpu_stat_t *pcpu_stats = NULL;
+static uint64_t *prev_idle = NULL;
+static int allocated_pcpus = 0;
+static xc_interface *xc_handle = NULL;
+static uint64_t prev_global_time = 0;
+
+static void report_pcpu_error(const char *context)
+{
+    fprintf(stderr, "PCPU error: %s (%s)\n", context, strerror(errno));
+}
+
+int update_pcpu_stats(const struct timeval *now, unsigned int delay_sec)
+{
+    struct xen_sysctl_cpuinfo info[MAX_PCPUS];
+    int detected_cpus = 0;
+    int ret, i;
+    uint64_t current_time = (uint64_t)now->tv_sec * 1000000ULL + now->tv_usec;
+    uint64_t time_diff;
+
+    if (!xc_handle) {
+        xc_handle = xc_interface_open(NULL, NULL, 0);
+        if (!xc_handle) {
+            report_pcpu_error("xc_interface_open failed");
+            return -1;
+        }
+    }
+
+    ret = xc_getcpuinfo(xc_handle, MAX_PCPUS, info, &detected_cpus);
+    if (ret < 0) {
+        report_pcpu_error("xc_getcpuinfo failed");
+        return -1;
+    }
+
+    /* Allocate/reallocate memory if needed */
+    if (!pcpu_stats || detected_cpus > allocated_pcpus) {
+        pcpu_stat_t *new_stats = realloc(pcpu_stats,
+                        detected_cpus * sizeof(*pcpu_stats));
+        if (!new_stats) goto alloc_error;
+
+        pcpu_stats = new_stats;
+
+        uint64_t *new_prev_idle = realloc(prev_idle,
+                        detected_cpus * sizeof(*prev_idle));
+        if (!new_prev_idle) goto alloc_error;
+
+        prev_idle = new_prev_idle;
+        allocated_pcpus = detected_cpus;
+
+        /* Initialize new entries */
+        for (i = 0; i < detected_cpus; i++) {
+            prev_idle[i] = info[i].idletime / 1000; /* ns->us */
+            pcpu_stats[i].usage_pct = 0.0;
+        }
+
+        /* Set initial global time reference */
+        prev_global_time = current_time;
+        return 0;
+    }
+
+    /* Calculate time difference since last update */
+    time_diff = current_time - prev_global_time;
+
+    /* Calculate CPU usage for each core */
+    for (i = 0; i < detected_cpus; i++) {
+        uint64_t current_idle = info[i].idletime / 1000;
+        uint64_t idle_diff = current_idle - prev_idle[i];
+
+        if (time_diff > 0) {
+            double usage = 100.0 * (1.0 - ((double)idle_diff / time_diff));
+            /* Clamp between 0-100% */
+            pcpu_stats[i].usage_pct = (usage < 0) ? 0.0 :
+                                     (usage > 100) ? 100.0 : usage;
+        } else {
+            pcpu_stats[i].usage_pct = 0.0;
+        }
+
+        prev_idle[i] = current_idle;
+    }
+
+    /* Update global time reference for next calculation */
+    prev_global_time = current_time;
+
+    return 0;
+
+alloc_error:
+    free_pcpu_stats();
+    errno = ENOMEM;
+    report_pcpu_error("memory allocation failed");
+    return -1;
+}
+
+/* Accessor functions for xentop.c */
+int get_pcpu_count(void)
+{
+    return allocated_pcpus;
+}
+
+float get_pcpu_usage(int cpu_index)
+{
+    if (!pcpu_stats || cpu_index < 0 || cpu_index >= allocated_pcpus) {
+        return 0.0;
+    }
+    return pcpu_stats[cpu_index].usage_pct;
+}
+
+int has_pcpu_data(void)
+{
+    return (pcpu_stats != NULL && allocated_pcpus > 0);
+}
+
+void free_pcpu_stats(void)
+{
+    if (xc_handle) {
+        xc_interface_close(xc_handle);
+        xc_handle = NULL;
+    }
+    free(pcpu_stats);
+    pcpu_stats = NULL;
+    free(prev_idle);
+    prev_idle = NULL;
+    allocated_pcpus = 0;
+}
-- 
2.34.1

Re: [PATCH v4 2/4] xentop: add pcpu implementation with proper error handling

[Anthony PERARD]

On Wed, Sep 03, 2025 at 03:53:21PM +0530, Jahan Murudi wrote:
> diff --git a/tools/xentop/pcpu.c b/tools/xentop/pcpu.c
> new file mode 100644
> index 0000000000..cdb4cb2131
> --- /dev/null
> +++ b/tools/xentop/pcpu.c
[..]
> +/* Accessor functions for xentop.c */
> +int get_pcpu_count(void)
> +{
> +    return allocated_pcpus;
> +}
> +
> +float get_pcpu_usage(int cpu_index)

You could use `size_t` instead of `int` for the `cpu_index`, and then,
no need to check for negatives. `allocated_pcpus` could also be `size_t`
or `unsigned int`, since "0" mean not available, we don't need "-1" or
other negative numbers.

> +{
> +    if (!pcpu_stats || cpu_index < 0 || cpu_index >= allocated_pcpus) {
> +        return 0.0;
> +    }
> +    return pcpu_stats[cpu_index].usage_pct;
> +}

Thanks,

-- 
Anthony PERARD

[PATCH v4 3/4] xentop: update Makefile to link against libxenctrl

[Jahan Murudi]

Update the build system so that xentop links against
libxenctrl, which is required for retrieving per-CPU
statistics via the hypervisor API.

Also update the build rule to include the new pcpu.o
object file.

Signed-off-by: Jahan Murudi <jahan.murudi.zg@renesas.com>
---
 tools/xentop/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/xentop/Makefile b/tools/xentop/Makefile
index 70cc2211c5..f514a6f7a8 100644
--- a/tools/xentop/Makefile
+++ b/tools/xentop/Makefile
@@ -15,6 +15,7 @@ include $(XEN_ROOT)/tools/Rules.mk
 
 CFLAGS += -DGCC_PRINTF $(CFLAGS_libxenstat)
 LDLIBS += $(LDLIBS_libxenstat) $(CURSES_LIBS) $(TINFO_LIBS) $(SOCKET_LIBS) -lm
+LDLIBS += $(LDLIBS_libxenctrl)
 CFLAGS += -DHOST_$(XEN_OS)
 
 # Include configure output (config.h)
@@ -25,8 +26,8 @@ TARGETS := xentop
 .PHONY: all
 all: $(TARGETS)
 
-xentop: xentop.o
-	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS) $(APPEND_LDFLAGS)
+xentop: xentop.o pcpu.o
+	$(CC) $(LDFLAGS) -o $@ $^ $(LDLIBS) $(APPEND_LDFLAGS)
 
 .PHONY: install
 install: all
-- 
2.34.1

[PATCH v4 4/4] xentop: integrate pcpu support into main program

[Jahan Murudi]

Integrate physical CPU usage statistics into xentop.
This patch adds:
 - a new command-line option (-p, --pcpus)
 - display of per-CPU usage statistics
 - cleanup of pcpu resources on exit

This allows users to monitor host physical CPU utilization
alongside existing domain metrics.

Signed-off-by: Jahan Murudi <jahan.murudi.zg@renesas.com>
---
 tools/xentop/xentop.c | 79 +++++++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 18 deletions(-)

diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
index f5d6c19cf9..addb1c70c9 100644
--- a/tools/xentop/xentop.c
+++ b/tools/xentop/xentop.c
@@ -37,6 +37,7 @@
 #endif
 
 #include <xenstat.h>
+#include "pcpu.h"
 
 #define XENTOP_VERSION "1.0"
 
@@ -205,6 +206,7 @@ field_id sort_field = FIELD_DOMID;
 unsigned int first_domain_index = 0;
 unsigned int delay = 3;
 unsigned int batch = 0;
+static unsigned int show_pcpus = 0;
 unsigned int loop = 1;
 unsigned int iterations = 0;
 int show_vcpus = 0;
@@ -230,22 +232,23 @@ static WINDOW *cwin;
 /* Print usage message, using given program name */
 static void usage(const char *program)
 {
-	printf("Usage: %s [OPTION]\n"
-	       "Displays ongoing information about xen vm resources \n\n"
-	       "-h, --help           display this help and exit\n"
-	       "-V, --version        output version information and exit\n"
-	       "-d, --delay=SECONDS  seconds between updates (default 3)\n"
-	       "-n, --networks       output vif network data\n"
-	       "-x, --vbds           output vbd block device data\n"
-	       "-r, --repeat-header  repeat table header before each domain\n"
-	       "-v, --vcpus          output vcpu data\n"
-	       "-b, --batch	     output in batch mode, no user input accepted\n"
-	       "-i, --iterations     number of iterations before exiting\n"
-	       "-f, --full-name      output the full domain name (not truncated)\n"
-	       "-z, --dom0-first     display dom0 first (ignore sorting)\n"
-	       "\n" XENTOP_BUGSTO,
-	       program);
-	return;
+    printf("Usage: %s [OPTION]\n"
+           "Displays ongoing information about xen vm resources \n\n"
+           "-h, --help           display this help and exit\n"
+           "-V, --version        output version information and exit\n"
+           "-d, --delay=SECONDS  seconds between updates (default 3)\n"
+           "-n, --networks       output vif network data\n"
+           "-x, --vbds           output vbd block device data\n"
+           "-r, --repeat-header  repeat table header before each domain\n"
+           "-v, --vcpus          output vcpu data\n"
+           "-b, --batch          output in batch mode, no user input accepted\n"
+           "-p, --pcpus          show physical CPU stats\n"
+           "-i, --iterations     number of iterations before exiting\n"
+           "-f, --full-name      output the full domain name (not truncated)\n"
+           "-z, --dom0-first     display dom0 first (ignore sorting)\n"
+           "\n" XENTOP_BUGSTO,
+           program);
+    return;
 }
 
 /* Print program version information */
@@ -267,6 +270,8 @@ static void cleanup(void)
 		xenstat_free_node(cur_node);
 	if(xhandle != NULL)
 		xenstat_uninit(xhandle);
+
+	free_pcpu_stats();
 }
 
 /* Display the given message and gracefully exit */
@@ -313,6 +318,32 @@ static void print(const char *fmt, ...)
 	}
 }
 
+/* PCPU statistics display function */
+static void print_pcpu_stats_display(void)
+{
+    int i;
+    int num_cpus;
+
+    if (!has_pcpu_data()) {
+        print("\nNo PCPU data available\n");
+        return;
+    }
+
+    num_cpus = get_pcpu_count();
+
+    /* Use the existing print() function which handles cursor bounds */
+    print("\nPhysical CPU Usage:\n");
+    print("+-------+--------+\n");
+    print("| Core  | Usage  |\n");
+    print("+-------+--------+\n");
+
+    for (i = 0; i < num_cpus; i++) {
+        print("| %-5d | %5.1f%% |\n", i, get_pcpu_usage(i));
+    }
+
+    print("+-------+--------+\n");
+}
+
 static void xentop_attron(int attr)
 {
 	if (!batch)
@@ -1245,6 +1276,14 @@ static void top(void)
 			do_vbd(domains[i]);
 	}
 
+    if (show_pcpus) {
+        if (update_pcpu_stats(&curtime, delay) == 0) {
+            print_pcpu_stats_display();
+        } else {
+            fail("Error getting PCPU stats\n");
+        }
+    }
+
 	if (!batch)
 		do_bottom_line();
 
@@ -1271,13 +1310,14 @@ int main(int argc, char **argv)
 		{ "repeat-header", no_argument,       NULL, 'r' },
 		{ "vcpus",         no_argument,       NULL, 'v' },
 		{ "delay",         required_argument, NULL, 'd' },
-		{ "batch",	   no_argument,	      NULL, 'b' },
+		{ "batch",         no_argument,	      NULL, 'b' },
+		{ "pcpus",         no_argument,       NULL, 'p' },
 		{ "iterations",	   required_argument, NULL, 'i' },
 		{ "full-name",     no_argument,       NULL, 'f' },
 		{ "dom0-first",    no_argument,       NULL, 'z' },
 		{ 0, 0, 0, 0 },
 	};
-	const char *sopts = "hVnxrvd:bi:fz";
+	const char *sopts = "hVnxrvd:bpi:fz";
 
 	if (atexit(cleanup) != 0)
 		fail("Failed to install cleanup handler.\n");
@@ -1312,6 +1352,9 @@ int main(int argc, char **argv)
 		case 'b':
 			batch = 1;
 			break;
+		case 'p':
+			show_pcpus = 1;
+			break;
 		case 'i':
 			iterations = atoi(optarg);
 			loop = 0;
-- 
2.34.1

Re: [PATCH v4 0/4] xentop: add physical CPU usage support

[Anthony PERARD]

Hi Jahan,

On Wed, Sep 03, 2025 at 03:53:19PM +0530, Jahan Murudi wrote:
> This is v4 of the patch series to add physical CPU monitoring to xentop.
> 
> Changes since v3:
> -   Split the single large patch into a logical series of 3 smaller patches
>     for easier review.

The single patch in v3 was fine to review. It didn't really need to be
cut into several patches. Having one file change per patch is certainly
the worse possible way to cut one patch into several.

It might have been possible to separate into several patch in another
way, but it's a bit too late for that, there's already been several
reviews. What I like to do when I review a patch series, is to look at
the difference since the last review I gave, tools like
`git range-diff` and https://patchew.org/Xen/ can help with that.

Anyway, squashing back all the patch is the way to go I think.

I'll have a look at the changes.

-- 
Anthony PERARD

Re: [PATCH v4 0/4] xentop: add physical CPU usage support

[Anthony PERARD]

On Wed, Sep 03, 2025 at 06:06:01PM +0200, Anthony PERARD wrote:
> Hi Jahan,
> 
> On Wed, Sep 03, 2025 at 03:53:19PM +0530, Jahan Murudi wrote:
> > This is v4 of the patch series to add physical CPU monitoring to xentop.
> > 
> > Changes since v3:
> > -   Split the single large patch into a logical series of 3 smaller patches
> >     for easier review.
> 
> The single patch in v3 was fine to review. It didn't really need to be
> cut into several patches. Having one file change per patch is certainly
> the worse possible way to cut one patch into several.
> 
> It might have been possible to separate into several patch in another
> way, but it's a bit too late for that, there's already been several
> reviews. What I like to do when I review a patch series, is to look at
> the difference since the last review I gave, tools like
> `git range-diff` and https://patchew.org/Xen/ can help with that.
> 
> Anyway, squashing back all the patch is the way to go I think.
> 
> I'll have a look at the changes.

So the code looks good, but only if it is a single patch like on v3.
Would you be fine if I merge the change, but squash all patch of v4
together and take the patch description from v3? (or the patch
description of the last patch with "integrate" replace by "introduce".)

If I do the squashing, I'll just ignore the remark to use "unsigned int"
instead of "int".

With all patch squash together, more or less: Reviewed-by: Anthony
PERARD <anthony.perard@vates.tech>

Cheers,

-- 
Anthony PERARD

RE: [PATCH v4 0/4] xentop: add physical CPU usage support

[Jahan Murudi]

Hi Anthony,

>> Anyway, squashing back all the patch is the way to go I think.
>> 
>> I'll have a look at the changes.

> So the code looks good, but only if it is a single patch like on v3.
> Would you be fine if I merge the change, but squash all patch of v4 together and take the patch description from v3? (or the patch description of the last patch with "integrate" replace by "introduce".)

> If I do the squashing, I'll just ignore the remark to use "unsigned int"
> instead of "int".

> With all patch squash together, more or less: Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thank you for the review and feedback.
I completely understand. Please feel free to squash the v4 series back into a single patch for merging, using the patch description from v3 as you suggested.

Thanks again for your time and guidance.

Best regards,
Jahan