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

On 12/16/12 15:47, Roman Bogorodskiy wrote:

--- src/util/processinfo.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)

diff --git a/src/util/processinfo.c b/src/util/processinfo.c index b1db049..4822bcc 100644 --- a/src/util/processinfo.c +++ b/src/util/processinfo.c @@ -168,6 +168,28 @@ realloc: return 0; }

+#elif defined(__FreeBSD__) + +int virProcessInfoSetAffinity(pid_t pid ATTRIBUTE_UNUSED, + virBitmapPtr map ATTRIBUTE_UNUSED) +{ + return 0;

Hmm, if the platform doesn't support setting of cpu affinity you should report an error here and return failure. Otherwise the user will think that the affinity was set successfully and when he tries to read it (code below) he/she gets incorrect value.

+} + +int virProcessInfoGetAffinity(pid_t pid ATTRIBUTE_UNUSED, + virBitmapPtr *map, + int maxcpu) +{ + *map = virBitmapNew(maxcpu); + if (!map) { + virReportOOMError(); + return -1; + } + virBitmapSetAll(*map);

Okay, this is fair enough, but you have to fail in the setting call so that this function doesn't lie to the user.

+ + return 0; +} + #else /* HAVE_SCHED_GETAFFINITY */

int virProcessInfoSetAffinity(pid_t pid ATTRIBUTE_UNUSED,

Peter

On Sun, Dec 16, 2012 at 06:47:56PM +0400, Roman Bogorodskiy wrote:

--- src/util/processinfo.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)

diff --git a/src/util/processinfo.c b/src/util/processinfo.c index b1db049..4822bcc 100644 --- a/src/util/processinfo.c +++ b/src/util/processinfo.c @@ -168,6 +168,28 @@ realloc: return 0; }

+#elif defined(__FreeBSD__) + +int virProcessInfoSetAffinity(pid_t pid ATTRIBUTE_UNUSED, + virBitmapPtr map ATTRIBUTE_UNUSED) +{ + return 0; +}

Hmm, I'm somewhat loathe to be pretending that this works. IMHO, we should look at 'map' and if it is all-1s then we should accept it trivially. If it is not all-1s then we should raise an error so the user knows their requested config was not honoured

+ +int virProcessInfoGetAffinity(pid_t pid ATTRIBUTE_UNUSED, + virBitmapPtr *map, + int maxcpu) +{ + *map = virBitmapNew(maxcpu); + if (!map) { + virReportOOMError(); + return -1; + } + virBitmapSetAll(*map); + + return 0; +} + #else /* HAVE_SCHED_GETAFFINITY */

int virProcessInfoSetAffinity(pid_t pid ATTRIBUTE_UNUSED,

Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Mon, Dec 17, 2012 at 09:09:52PM +0400, Roman Bogorodskiy wrote:

Daniel P. Berrange wrote:

...

On Sun, Dec 16, 2012 at 06:47:56PM +0400, Roman Bogorodskiy wrote:

...

--- src/util/processinfo.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)

diff --git a/src/util/processinfo.c b/src/util/processinfo.c index b1db049..4822bcc 100644 --- a/src/util/processinfo.c +++ b/src/util/processinfo.c @@ -168,6 +168,28 @@ realloc: return 0; }

+#elif defined(__FreeBSD__) + +int virProcessInfoSetAffinity(pid_t pid ATTRIBUTE_UNUSED, + virBitmapPtr map ATTRIBUTE_UNUSED) +{ + return 0; +}

Hmm, I'm somewhat loathe to be pretending that this works.

IMHO, we should look at 'map' and if it is all-1s then we should accept it trivially. If it is not all-1s then we should raise an error so the user knows their requested config was not honoured

The reasoning behind this change is, as subject says, provide a stub while there's no complete implementation available. I've been using it for a couple of weeks and didn't notice any problems.

Would not it make sense to just throw a warning or not critical error to user so he knows there's no affinity? I was planning to create a full-feature implementation of that after I'm done with the networking support.

Ok, since you're planning on implementing this properly, I'll accept this patch as is for now Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

From: Roman Bogorodskiy <bogorodskiy@gmail.com> --- This version incorporates DanPB's idea of checking if setting affinity for all processors and failing otherwise. --- src/util/processinfo.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/util/processinfo.c b/src/util/processinfo.c index b1db049..6cca426 100644 --- a/src/util/processinfo.c +++ b/src/util/processinfo.c @@ -168,6 +168,34 @@ realloc: return 0; } +#elif defined(__FreeBSD__) + +int virProcessInfoSetAffinity(pid_t pid ATTRIBUTE_UNUSED, + virBitmapPtr map) +{ + if (!virBitmapIsAllSet(map)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("setting process affinity isn't supported " + "on FreeBSD yet")); + return -1; + } + + return 0; +} + +int virProcessInfoGetAffinity(pid_t pid ATTRIBUTE_UNUSED, + virBitmapPtr *map, + int maxcpu) +{ + if (!(*map = virBitmapNew(maxcpu))) { + virReportOOMError(); + return -1; + } + virBitmapSetAll(*map); + + return 0; +} + #else /* HAVE_SCHED_GETAFFINITY */ int virProcessInfoSetAffinity(pid_t pid ATTRIBUTE_UNUSED, -- 1.8.0.2