On Thu, Jul 05, 2012 at 09:49:40AM -0700, Chegu Vinod wrote:
> Changes since v2:
> - Using "unsigned long *" for the node_cpumask[].
> - Use bitmap_new() instead of g_malloc0() for allocation.
> - Don't rely on "max_cpus" since it may not be initialized
> before the numa related qemu options are parsed & processed.
>
> Note: Continuing to use a new constant for allocation of
> the mask (This constant is currently set to 255 since
> with an 8bit APIC ID VCPUs can range from 0-254 in a
> guest. The APIC ID 255 (0xFF) is reserved for broadcast).
That means that depending on core/thread topology, we may have an even
smaller limit.
(But that will be a problem only after we fix the APIC-ID CPU topology
bug we have)
>
>
[...]
> Signed-off-by: Chegu Vinod <chegu_vinod@xxxxxx>, Jim Hull <jim.hull@xxxxxx>, Craig Hada <craig.hada@xxxxxx>
> Tested-by: Eduardo Habkost <ehabkost <at> redhat.com>
Actually I had not tested this version yet, when you submitted it. But
now I have tested it.
Tested-by: Eduardo Habkost <ehabkost@xxxxxxxxxx>
Other minor comments below:
> ---
> cpus.c | 3 ++-
> hw/pc.c | 3 ++-
> sysemu.h | 3 ++-
> vl.c | 48 ++++++++++++++++++++++++++----------------------
> 4 files changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index b182b3d..acccd08 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -36,6 +36,7 @@
> #include "cpus.h"
> #include "qtest.h"
> #include "main-loop.h"
> +#include "bitmap.h"
>
> #ifndef _WIN32
> #include "compatfd.h"
> @@ -1145,7 +1146,7 @@ void set_numa_modes(void)
>
> for (env = first_cpu; env != NULL; env = env->next_cpu) {
> for (i = 0; i < nb_numa_nodes; i++) {
> - if (node_cpumask[i] & (1 << env->cpu_index)) {
> + if (test_bit(env->cpu_index, node_cpumask[i])) {
> env->numa_node = i;
> }
> }
> diff --git a/hw/pc.c b/hw/pc.c
> index c7e9ab3..2edcc07 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -48,6 +48,7 @@
> #include "memory.h"
> #include "exec-memory.h"
> #include "arch_init.h"
> +#include "bitmap.h"
>
> /* output Bochs bios info messages */
> //#define DEBUG_BIOS
> @@ -639,7 +640,7 @@ static void *bochs_bios_init(void)
> numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
> for (i = 0; i < max_cpus; i++) {
> for (j = 0; j < nb_numa_nodes; j++) {
> - if (node_cpumask[j] & (1 << i)) {
> + if (test_bit(i, node_cpumask[j])) {
> numa_fw_cfg[i + 1] = cpu_to_le64(j);
> break;
> }
> diff --git a/sysemu.h b/sysemu.h
> index bc2c788..2ce63fc 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -133,9 +133,10 @@ extern uint8_t qemu_extra_params_fw[2];
> extern QEMUClock *rtc_clock;
>
> #define MAX_NODES 64
> +#define MAX_CPUMASK_BITS 255
> extern int nb_numa_nodes;
> extern uint64_t node_mem[MAX_NODES];
> -extern uint64_t node_cpumask[MAX_NODES];
> +extern unsigned long *node_cpumask[MAX_NODES];
>
> #define MAX_OPTION_ROMS 16
> typedef struct QEMUOptionRom {
> diff --git a/vl.c b/vl.c
> index 1329c30..fdd7b74 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -28,6 +28,7 @@
> #include <errno.h>
> #include <sys/time.h>
> #include <zlib.h>
> +#include "bitmap.h"
>
> /* Needed early for CONFIG_BSD etc. */
> #include "config-host.h"
> @@ -240,7 +241,7 @@ QTAILQ_HEAD(, FWBootEntry) fw_boot_order = QTAILQ_HEAD_INITIALIZER(fw_boot_order
>
> int nb_numa_nodes;
> uint64_t node_mem[MAX_NODES];
> -uint64_t node_cpumask[MAX_NODES];
> +unsigned long *node_cpumask[MAX_NODES];
>
> uint8_t qemu_uuid[16];
>
> @@ -950,6 +951,9 @@ static void numa_add(const char *optarg)
> char *endptr;
> unsigned long long value, endvalue;
> int nodenr;
> + int i;
> +
> + value = endvalue = 0ULL;
>
> optarg = get_opt_name(option, 128, optarg, ',') + 1;
> if (!strcmp(option, "node")) {
> @@ -970,27 +974,25 @@ static void numa_add(const char *optarg)
> }
> node_mem[nodenr] = sval;
> }
> - if (get_param_value(option, 128, "cpus", optarg) == 0) {
> - node_cpumask[nodenr] = 0;
> - } else {
> + if (get_param_value(option, 128, "cpus", optarg) != 0) {
> value = strtoull(option, &endptr, 10);
> - if (value >= 64) {
> - value = 63;
> - fprintf(stderr, "only 64 CPUs in NUMA mode supported.\n");
> + if (*endptr == '-') {
> + endvalue = strtoull(endptr+1, &endptr, 10);
> } else {
> - if (*endptr == '-') {
> - endvalue = strtoull(endptr+1, &endptr, 10);
> - if (endvalue >= 63) {
> - endvalue = 62;
> - fprintf(stderr,
> - "only 63 CPUs in NUMA mode supported.\n");
> - }
> - value = (2ULL << endvalue) - (1ULL << value);
> - } else {
> - value = 1ULL << value;
> - }
> + endvalue = value;
> + }
> +
> +
> + if (!(endvalue < MAX_CPUMASK_BITS)) {
> + endvalue = MAX_CPUMASK_BITS - 1;
> + fprintf(stderr,
> + "A max of %d CPUs are supported in a guest\n",
> + MAX_CPUMASK_BITS);
> + }
> +
> + for (i = value; i <= endvalue; ++i) {
> + set_bit(i, node_cpumask[nodenr]);
> }
Nit: you could optimize this using bitmap_set(), but it's not a big
deal.
> - node_cpumask[nodenr] = value;
> }
> nb_numa_nodes++;
> }
> @@ -2331,7 +2333,8 @@ int main(int argc, char **argv, char **envp)
>
> for (i = 0; i < MAX_NODES; i++) {
> node_mem[i] = 0;
> - node_cpumask[i] = 0;
> + node_cpumask[i] = bitmap_new(MAX_CPUMASK_BITS);
> + bitmap_zero(node_cpumask[i], MAX_CPUMASK_BITS);
bitmap_new() already returns a zeroed bitmap, it seems.
> }
>
> nb_numa_nodes = 0;
> @@ -3469,8 +3472,9 @@ int main(int argc, char **argv, char **envp)
> }
>
> for (i = 0; i < nb_numa_nodes; i++) {
> - if (node_cpumask[i] != 0)
> + if (!bitmap_empty(node_cpumask[i], MAX_CPUMASK_BITS)) {
> break;
> + }
> }
> /* assigning the VCPUs round-robin is easier to implement, guest OSes
> * must cope with this anyway, because there are BIOSes out there in
> @@ -3478,7 +3482,7 @@ int main(int argc, char **argv, char **envp)
> */
> if (i == nb_numa_nodes) {
> for (i = 0; i < max_cpus; i++) {
> - node_cpumask[i % nb_numa_nodes] |= 1 << i;
> + set_bit(i, node_cpumask[i % nb_numa_nodes]);
> }
> }
> }
Reviewed-by: Eduardo Habkost <ehabkost@xxxxxxxxxx>
--
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
