[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] FreeBSD: add nodeinfo support.



On 12/16/12 15:47, Roman Bogorodskiy wrote:
Uses sysctl(3) interface to obtain CPU and memory information.
---
  src/nodeinfo.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 74 insertions(+), 1 deletion(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 096000b..f6fdf90 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -38,6 +38,11 @@
  # include <numa.h>
  #endif

+#ifdef __FreeBSD__
+# include <sys/types.h>
+# include <sys/sysctl.h>
+#endif
+
  #include "c-ctype.h"
  #include "memory.h"
  #include "nodeinfo.h"
@@ -53,6 +58,24 @@

  #define VIR_FROM_THIS VIR_FROM_NONE

+#ifdef __FreeBSD__
+static int freebsdNodeGetCPUCount(void);

We usually don't forward-declare functions if they are defined right after.

+
+static int
+freebsdNodeGetCPUCount(void)
+{
+    int ncpu_mib[2] = { CTL_HW, HW_NCPU };
+    unsigned long ncpu;
+    size_t ncpu_len = sizeof(ncpu);
+
+    if (sysctl(ncpu_mib, 2, &ncpu, &ncpu_len, NULL, 0) == -1) {
+        return -1;
+    }
+
+    return ncpu;
+}
+#endif
+
  #ifdef __linux__
  # define CPUINFO_PATH "/proc/cpuinfo"
  # define SYSFS_SYSTEM_PATH "/sys/devices/system"
@@ -871,6 +894,46 @@ cleanup:
      VIR_FORCE_FCLOSE(cpuinfo);
      return ret;
      }
+#elif defined(__FreeBSD__)
+    {
+    nodeinfo->nodes = 1;
+    nodeinfo->sockets = 1;
+    nodeinfo->cores = 1;
+    nodeinfo->threads = 1;
+
+    nodeinfo->cpus = freebsdNodeGetCPUCount();

Documentation to the nodeinfo structure says that if the correct topology cannot be enumerated, the actual count of processors should be returned in the "cores" field instead of cpus. On linux this kind of "lie" is used when the host is a strange NUMA architecture, where we can't successfully determine the actual count of NUMA nodes and stuff.

+
+    if (nodeinfo->cpus == -1) {
+        virReportError(VIR_ERR_NO_SUPPORT, "%s",
+                     _("cannot obtain cpu count"));

Hmm, it would be better (as I see this error reporting code twice in this patch) for the freebsdNodeGetCPUCount to report the error. Also I suppose that the sysctl call sets the "errno" variable where it would be better to use the virReportSystemError function that takes errno as a param and prints the system error. Also I think that the VIR_ERR_NO_SUPPORT isn't really appropriate for this kind of errors as this error code is mainly used when the remote daemon doesn't support the method that was called.

+    }
+
+    unsigned long cpu_freq;
+    size_t cpu_freq_len = sizeof(cpu_freq);
+
+    if (sysctlbyname("dev.cpu.0.freq", &cpu_freq, &cpu_freq_len, NULL, 0) < 0) {
+        virReportError(VIR_ERR_NO_SUPPORT, "%s",
+                    _("cannot obtain cpu freq"));

This also seems as a candidate for virReportSystemError()

+        return -1;
+    }
+
+    nodeinfo->mhz = cpu_freq;
+
+    /* get memory information */
+    int mib[2] = { CTL_HW, HW_PHYSMEM };
+    unsigned long physmem;
+    size_t len = sizeof(physmem);
+
+    if (sysctl(mib, 2, &physmem, &len, NULL, 0) == -1) {
+        virReportError(VIR_ERR_NO_SUPPORT, "%s",
+                    _("cannot obtain memory count"));

And here too. Also "memory count" is awkward. Please use "memory size"

+        return -1;
+    }
+
+    nodeinfo->memory = (unsigned long)(physmem / 1024);
+
+    return 0;
+    }
  #else
      /* XXX Solaris will need an impl later if they port QEMU driver */
      virReportError(VIR_ERR_NO_SUPPORT, "%s",
@@ -978,7 +1041,7 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED,
  int
  nodeGetCPUCount(void)
  {
-#ifdef __linux__
+#if defined(__linux__)
      /* To support older kernels that lack cpu/present, such as 2.6.18
       * in RHEL5, we fall back to count cpu/cpuNN entries; this assumes
       * that such kernels also lack hotplug, and therefore cpu/cpuNN
@@ -1008,6 +1071,16 @@ nodeGetCPUCount(void)

      VIR_FREE(cpupath);
      return i;
+#elif defined(__FreeBSD__)
+    int cpu_count = -1;
+
+    cpu_count = freebsdNodeGetCPUCount();
+    if (cpu_count == -1) {
+        virReportError(VIR_ERR_NO_SUPPORT, "%s",
+                       _("cannot obtain cpu count"));

Here's the duplicate code as noted above.

+    }
+
+    return cpu_count;
  #else
      virReportError(VIR_ERR_NO_SUPPORT, "%s",
                     _("host cpu counting not implemented on this platform"));


I unfortunately don't have FreeBSD at hand to actually test the system calls so I didn't check if this works :(

Peter


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]