Re: SMP per-CPU statistics under FreeBSD.

Marco van Tol <[email protected]>
Newsgroups gmane.comp.gnome.apps.gkrellm
Message-ID <[email protected]>
On Tue, Apr 25, 2006 at 05:41:11PM +0000, Marco van Tol wrote:
> On Tue, Apr 25, 2006 at 05:31:34PM +0900, Hajimu UMEMOTO wrote:
> > Hi,
> > 
> > >>>>> On Sun, 23 Apr 2006 01:10:00 +0000
> > >>>>> Marco van Tol <[email protected]> said:
> > 
> > > > 2) You have the choice of reporting composite CPU data yourself or letting gkrellm
> > > > calculate the composite data.  Just never call gkrellm_cpu_assign_composite_data()
> > > > and gkrellm will do it for you.
> > 
> > The netbsd.c calls gkrellm_cpu_assign_composite_data().  As far as I
> > looked the jhb's patch briefly, it seems kern.cp_time returns
> > composite values.  I'm not sure which is better to call
> > gkrellm_cpu_assign_composite_data() or not, but I attches the patch to
> > call gkrellm_cpu_assign_composite_data().  Sorry but I don't test it
> > actually even on single-CPU machine, yet.
> > 
> > Your patch has following line:
> > 
> > 	+static size_t	oid_pcpu_time_len = sizeof(oid_cp_time);
> > 
> > It should be:
> > 
> > 	+static size_t	oid_pcpu_time_len = sizeof(oid_pcpu_time);
> > 
> > This fix is also inclueded in my patch.
> 
> I just checked out your patch (unfortunately I don't have an SMP box at
> work ;), but it doesn't display the data for the CPU's.  It does setup 2
> CPU's for my box, which is the correct count, but they both stick at 0%,
> even if I start a buildworld on my box.
> 
> I'll take a quick look to see if I can find why and will let you know.

Ha! Found it. :)

(Linenumbers are after applying your patch)

The sysctl on line 127 stores the number of bytes stored in cp_time in len.
Lateron on line 149, the modified len is used to indicate the space
available in cp_time, which is not enough to store the data returned by
oid_pcpu_time, even though cp_time does have enough space.
But len indicates the wrong count because it was modified by the sysctl on
line 127.

I fixed it by putting 'len = sizeof(cp_time);' on line 149, right before
doing the sysctl for oid_pcpu_time, at which point it does indicate the
correct space available in cp_time.  This may be crude, and you may want to
fix it more elegantly, but it does fix it.

I attached a modified version of your patch to this email.  The only
difference is the added 'len = sizeof(cp_time);' line.

Let me know if you like it. :)
It works both in non-client/server and in client/server mode.

Thanks,

Marco

-- 
Morgen stop ik.

_______________________________________________
Gkrellm mailing list
[email protected]
http://ninja.linux-phreak.biz/mailman/listinfo/gkrellm
freebsd.c.patch.2 (text/plain, 2.5 KB)
--- freebsd.c.orig	Wed Apr 26 00:28:41 2006
+++ freebsd.c	Wed Apr 26 00:29:51 2006
@@ -86,18 +86,36 @@ gk_sysctlnametomib(const char *name, int
 #endif
 #include <kvm.h>
 
-
 extern	kvm_t	*kvmd;
 
 static int	oid_cp_time[CTL_MAXNAME + 2];
 static size_t	oid_cp_time_len = sizeof(oid_cp_time);
 static gint	have_cp_time;
 
+static int	oid_pcpu_time[CTL_MAXNAME + 2];
+static size_t	oid_pcpu_time_len = sizeof(oid_pcpu_time);
+static gint	have_pcpu_time;
+
+static gint	ncpus;
+
+static gint
+get_ncpus(void)
+{
+	int ncpu;
+	size_t len = sizeof(ncpu);
+
+	if (sysctlbyname("hw.ncpu", &ncpu, &len, NULL, 0) < 0)
+		return 1;
+	else
+		return ncpu;
+}
+
 void
 gkrellm_sys_cpu_read_data(void)
 	{
-	long		cp_time[CPUSTATES];
-	int		len = sizeof(cp_time);
+	long		cp_time[ncpus][CPUSTATES];
+	size_t		len = sizeof(cp_time);
+	int		i;
 	static struct nlist nl[] = {
 #define N_CP_TIME	0
 		{ "_cp_time" },
@@ -122,21 +140,36 @@ gkrellm_sys_cpu_read_data(void)
 			return;
 		}
 
-	/* Currently, SMP is not supported */
-	gkrellm_cpu_assign_data(0, cp_time[CP_USER], cp_time[CP_NICE],
-				cp_time[CP_SYS], cp_time[CP_IDLE]);
+	if (ncpus > 1 && have_pcpu_time)
+		{
+		gkrellm_cpu_assign_composite_data(cp_time[0][CP_USER],
+						  cp_time[0][CP_NICE],
+						  cp_time[0][CP_SYS],
+						  cp_time[0][CP_IDLE]);
+		len = sizeof(cp_time);
+		if (sysctl(oid_pcpu_time, oid_pcpu_time_len,
+			   cp_time, &len, NULL, 0) < 0)
+			return;
+		}
+
+	for (i = 0 ; i < ncpus ; i++)
+		gkrellm_cpu_assign_data(i, cp_time[i][CP_USER],
+					cp_time[i][CP_NICE],
+					cp_time[i][CP_SYS],
+					cp_time[i][CP_IDLE]);
 	}
 
 gboolean
 gkrellm_sys_cpu_init(void)
 	{
-	static char	*name = "kern.cp_time";
-
-	gkrellm_cpu_set_number_of_cpus(1);
-
-	if (gk_sysctlnametomib(name, oid_cp_time, &oid_cp_time_len) < 0)
-		return TRUE;
-	++have_cp_time;
+	ncpus = get_ncpus();
+	gkrellm_cpu_set_number_of_cpus(ncpus);
+	if (gk_sysctlnametomib("kern.cp_time", oid_cp_time,
+			       &oid_cp_time_len) == 0)
+		++have_cp_time;
+	if (gk_sysctlnametomib("kern.pcpu_time", oid_pcpu_time,
+			       &oid_pcpu_time_len) == 0)
+		++have_pcpu_time;
 	return TRUE;
 	}
 
@@ -206,7 +239,7 @@ gkrellm_sys_proc_read_data(void)
 	static u_int	n_processes, n_forks = 0, curpid = -1;
 	u_int		n_vforks, n_rforks;
 	gint		r_forks, r_vforks, r_rforks;
-	gint		len;
+	size_t		len;
 	gint		nextpid, nforked;
 	static struct nlist nl[] = {
 #define N_NEXTPID	0
@@ -570,7 +603,7 @@ gkrellm_sys_inet_read_tcp_data(void)
 	gint		tcp_status;
 	struct xinpgen	*xig, *oxig;
 	gchar		*buf;
-	gint		len = 0;
+	size_t		len = 0;
 
 	if (!initialized)
 		{
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.