Re: [PATCH] Flexible vcpu pinning configuration
Hi Roman, On Thu, May 1, 2014 at 7:30 AM, Roman Bogorodskiy wrote: > Neel Natu wrote: > >> Hi Roman, >> >> On Sun, Apr 27, 2014 at 3:45 AM, Roman Bogorodskiy wrote: >> > I've created an initial version of the patch which allows more flexible >> > vcpu pinning configuration. >> > >> > Current schema is: >> > >> > bhyve -p N >> > >> > pins vcpu i to hostcpu N + i. >> > >> > The propsed extension is: >> > >> > bhyve -p N:M -p 0:1 -p 3:5 >> > >> > which pins vcpu N to host pcpu M. Option needs to be specified >> > individually for each vcpu. >> > >> > So it works like that for me: >> > >> > sudo /usr/sbin/bhyve -p 0:0 -p 1:3 -c 2 ... >> > >> > # sudo cpuset -g -t 100262 >> > tid 100262 mask: 0 >> > # sudo cpuset -g -t 100264 >> > tid 100264 mask: 3 >> > >> > PS I used cpumat_t* array to store these values instead of int, because >> > if the idea is OK, I'll extend it to support ranges like e.g. cpuset(1) >> > supports, e.g.: "1:2-5". >> > >> > The questions are: >> > >> > - Is it OK to chance '-p' arg syntax or it's better to introduce a new >> >one? >> > >> >> I think we can reuse the "-p" option unless anybody objects vociferously. >> >> > - Is the syntax OK (currently: 'vcpu:pcpu', later >> >'vcpu:pcpuN-pcpuM,pcpuX")? >> >> Yup, I think that works fine. >> >> The patch looks good in general but I have a few comments: >> >> - Scope of 'vcpupmap[]' should be restricted to 'static'. >> >> - usage() and man page need to be updated. >> >> - pincpu_parse(): >> The option parsing can be made much easier by using: >> >> if (sscanf(str, "%d:%d", &vcpu, &pcpu) == 2) { >> /* success */ >> } else { >> return (-1); >> } >> >> If the same vcpu is specified multiple times then we should >> malloc(sizeof(cpuset_t)) only the first time: >> >> if (vcpumap[vcpu] != NULL) >> mask = vcpumap[vcpu]; >> else >> mask = malloc(sizeof(cpuset_t)); >> >> We need to range-check 'vcpu' before using it as an index into the >> 'vcpumap[]' array. >> >> best >> Neel > > Attached an updated patch. A slightly modified version was submitted: http://svnweb.freebsd.org/base?view=revision&revision=265376 > > I'm still inclined to use something like parselist() from > usr.bin/cpuset/cpuset.c, but I don't want to copy/paste and I don't know > where it'd make sense to move it so it was usable outside of cpuset? > Hmm, not sure really … but you can get started by copying it into bhyve and if there is a better place to put it we can do that too. > PS While reading bhyverun.c, I think I spotted a typo: in fbsdrun_deletecpu() > error message says fprintf(stderr, "addcpu: Should it be "deletecpu:" > instead? Thanks. This is fixed now: http://svnweb.freebsd.org/base?view=revision&revision=265366 best Neel > > Roman Bogorodskiy ___ freebsd-virtualization@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization To unsubscribe, send any mail to "freebsd-virtualization-unsubscr...@freebsd.org"
Re: [PATCH] Flexible vcpu pinning configuration
Neel Natu wrote: > Hi Roman, > > On Sun, Apr 27, 2014 at 3:45 AM, Roman Bogorodskiy wrote: > > I've created an initial version of the patch which allows more flexible > > vcpu pinning configuration. > > > > Current schema is: > > > > bhyve -p N > > > > pins vcpu i to hostcpu N + i. > > > > The propsed extension is: > > > > bhyve -p N:M -p 0:1 -p 3:5 > > > > which pins vcpu N to host pcpu M. Option needs to be specified > > individually for each vcpu. > > > > So it works like that for me: > > > > sudo /usr/sbin/bhyve -p 0:0 -p 1:3 -c 2 ... > > > > # sudo cpuset -g -t 100262 > > tid 100262 mask: 0 > > # sudo cpuset -g -t 100264 > > tid 100264 mask: 3 > > > > PS I used cpumat_t* array to store these values instead of int, because > > if the idea is OK, I'll extend it to support ranges like e.g. cpuset(1) > > supports, e.g.: "1:2-5". > > > > The questions are: > > > > - Is it OK to chance '-p' arg syntax or it's better to introduce a new > >one? > > > > I think we can reuse the "-p" option unless anybody objects vociferously. > > > - Is the syntax OK (currently: 'vcpu:pcpu', later > >'vcpu:pcpuN-pcpuM,pcpuX")? > > Yup, I think that works fine. > > The patch looks good in general but I have a few comments: > > - Scope of 'vcpupmap[]' should be restricted to 'static'. > > - usage() and man page need to be updated. > > - pincpu_parse(): > The option parsing can be made much easier by using: > > if (sscanf(str, "%d:%d", &vcpu, &pcpu) == 2) { > /* success */ > } else { > return (-1); > } > > If the same vcpu is specified multiple times then we should > malloc(sizeof(cpuset_t)) only the first time: > > if (vcpumap[vcpu] != NULL) > mask = vcpumap[vcpu]; > else > mask = malloc(sizeof(cpuset_t)); > > We need to range-check 'vcpu' before using it as an index into the > 'vcpumap[]' array. > > best > Neel Attached an updated patch. I'm still inclined to use something like parselist() from usr.bin/cpuset/cpuset.c, but I don't want to copy/paste and I don't know where it'd make sense to move it so it was usable outside of cpuset? PS While reading bhyverun.c, I think I spotted a typo: in fbsdrun_deletecpu() error message says fprintf(stderr, "addcpu: Should it be "deletecpu:" instead? Roman Bogorodskiy Index: bhyve.8 === --- bhyve.8 (revision 264921) +++ bhyve.8 (working copy) @@ -78,12 +78,14 @@ allow a remote kernel kgdb to be relayed to the guest kernel gdb stub via a local IPv4 address and this port. This option will be deprecated in a future version. -.It Fl p Ar pinnedcpu +.It Fl p Ar n:m Force guest virtual CPUs to be pinned to host CPUs. Virtual CPU .Em n is pinned to host CPU -.Em pinnedcpu+n . +.Em m . +This option could be specified multiple times to set pinning for each +Virtual CPU and to pin Virtual CPU to more than one host CPU. .It Fl P Force the guest virtual CPU to exit when a PAUSE instruction is detected. .It Fl W Index: bhyverun.c === --- bhyverun.c (revision 264921) +++ bhyverun.c (working copy) @@ -83,7 +83,6 @@ int guest_ncpus; -static int pincpu = -1; static int guest_vmexit_on_hlt, guest_vmexit_on_pause, disable_x2apic; static int virtio_msix = 1; @@ -119,6 +118,8 @@ int mt_vcpu; } mt_vmm_info[VM_MAXCPU]; +static cpuset_t *vcpumap[VM_MAXCPU] = { NULL }; + static void usage(int code) { @@ -125,12 +126,12 @@ fprintf(stderr, "Usage: %s [-aehwAHIPW] [-g ] [-s ] [-S ]\n" - " %*s [-c vcpus] [-p pincpu] [-m mem] [-l ] \n" + " %*s [-c vcpus] [-p vcpu:hostcpu] [-m mem] [-l ] \n" " -a: local apic is in XAPIC mode (default is X2APIC)\n" " -A: create an ACPI table\n" " -g: gdb port\n" " -c: # cpus (default 1)\n" - " -p: pin vcpu 'n' to host cpu 'pincpu + n'\n" + " -p: pin vcpu 'n' to host cpu 'm'\n" " -H: vmexit from the guest on hlt\n" " -P: vmexit from the guest on pause\n" " -W: force virtio to use single-vector MSI\n" @@ -146,6 +147,53 @@ exit(code); } +static int +pincpu_parse(const char *cpupin) +{ +char *string; +int vcpu = -1, pcpu = -1, ret = -1; + +if ((string = strdup(cpupin)) == NULL) { +fprintf(stderr, "strdup failed: %d\n", errno); +return -1; +} + +if (sscanf(string, "%d:%d", &vcpu, &pcpu) != 2) { +fprintf(stderr, "invalid format: %s\n", string); +goto cleanup; +} + +if (vcpu < 0 || vcpu > VM_MAXCPU - 1) { +fprintf(stderr, "invalid vcpu value '%d', " +"should be from 0 to %d\n", +vcpu, VM_MAXCPU - 1); +goto cleanup; +} + +if (pcpu < 0 || pcpu > CPU_SETSIZE) { +fprintf(stderr, "invalid pcpu value '%d', " +"should be from 0 to %d\n", +pcpu, CPU_SETSIZE); +goto cleanup; +
Re: [PATCH] Flexible vcpu pinning configuration
Neel Natu wrote: > Hi Roman, > > On Sun, Apr 27, 2014 at 3:45 AM, Roman Bogorodskiy wrote: > > I've created an initial version of the patch which allows more flexible > > vcpu pinning configuration. > > > > Current schema is: > > > > bhyve -p N > > > > pins vcpu i to hostcpu N + i. > > > > The propsed extension is: > > > > bhyve -p N:M -p 0:1 -p 3:5 > > > > which pins vcpu N to host pcpu M. Option needs to be specified > > individually for each vcpu. > > > > So it works like that for me: > > > > sudo /usr/sbin/bhyve -p 0:0 -p 1:3 -c 2 ... > > > > # sudo cpuset -g -t 100262 > > tid 100262 mask: 0 > > # sudo cpuset -g -t 100264 > > tid 100264 mask: 3 > > > > PS I used cpumat_t* array to store these values instead of int, because > > if the idea is OK, I'll extend it to support ranges like e.g. cpuset(1) > > supports, e.g.: "1:2-5". > > > > The questions are: > > > > - Is it OK to chance '-p' arg syntax or it's better to introduce a new > >one? > > > > I think we can reuse the "-p" option unless anybody objects vociferously. > > > - Is the syntax OK (currently: 'vcpu:pcpu', later > >'vcpu:pcpuN-pcpuM,pcpuX")? > > Yup, I think that works fine. > > The patch looks good in general but I have a few comments: > > - Scope of 'vcpupmap[]' should be restricted to 'static'. > > - usage() and man page need to be updated. > > - pincpu_parse(): > The option parsing can be made much easier by using: > > if (sscanf(str, "%d:%d", &vcpu, &pcpu) == 2) { > /* success */ > } else { > return (-1); > } > > If the same vcpu is specified multiple times then we should > malloc(sizeof(cpuset_t)) only the first time: > > if (vcpumap[vcpu] != NULL) > mask = vcpumap[vcpu]; > else > mask = malloc(sizeof(cpuset_t)); > > We need to range-check 'vcpu' before using it as an index into the > 'vcpumap[]' array. Thanks for the comments, I'll make the fixes you pointed and re-send the patch. Roman Bogorodskiy pgpNXJKIF45X9.pgp Description: PGP signature
Re: [PATCH] Flexible vcpu pinning configuration
Hi Roman, On Sun, Apr 27, 2014 at 3:45 AM, Roman Bogorodskiy wrote: > I've created an initial version of the patch which allows more flexible > vcpu pinning configuration. > > Current schema is: > > bhyve -p N > > pins vcpu i to hostcpu N + i. > > The propsed extension is: > > bhyve -p N:M -p 0:1 -p 3:5 > > which pins vcpu N to host pcpu M. Option needs to be specified > individually for each vcpu. > > So it works like that for me: > > sudo /usr/sbin/bhyve -p 0:0 -p 1:3 -c 2 ... > > # sudo cpuset -g -t 100262 > tid 100262 mask: 0 > # sudo cpuset -g -t 100264 > tid 100264 mask: 3 > > PS I used cpumat_t* array to store these values instead of int, because > if the idea is OK, I'll extend it to support ranges like e.g. cpuset(1) > supports, e.g.: "1:2-5". > > The questions are: > > - Is it OK to chance '-p' arg syntax or it's better to introduce a new >one? > I think we can reuse the "-p" option unless anybody objects vociferously. > - Is the syntax OK (currently: 'vcpu:pcpu', later >'vcpu:pcpuN-pcpuM,pcpuX")? Yup, I think that works fine. The patch looks good in general but I have a few comments: - Scope of 'vcpupmap[]' should be restricted to 'static'. - usage() and man page need to be updated. - pincpu_parse(): The option parsing can be made much easier by using: if (sscanf(str, "%d:%d", &vcpu, &pcpu) == 2) { /* success */ } else { return (-1); } If the same vcpu is specified multiple times then we should malloc(sizeof(cpuset_t)) only the first time: if (vcpumap[vcpu] != NULL) mask = vcpumap[vcpu]; else mask = malloc(sizeof(cpuset_t)); We need to range-check 'vcpu' before using it as an index into the 'vcpumap[]' array. best Neel > > Roman Bogorodskiy ___ freebsd-virtualization@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization To unsubscribe, send any mail to "freebsd-virtualization-unsubscr...@freebsd.org"
[PATCH] Flexible vcpu pinning configuration
I've created an initial version of the patch which allows more flexible vcpu pinning configuration. Current schema is: bhyve -p N pins vcpu i to hostcpu N + i. The propsed extension is: bhyve -p N:M -p 0:1 -p 3:5 which pins vcpu N to host pcpu M. Option needs to be specified individually for each vcpu. So it works like that for me: sudo /usr/sbin/bhyve -p 0:0 -p 1:3 -c 2 ... # sudo cpuset -g -t 100262 tid 100262 mask: 0 # sudo cpuset -g -t 100264 tid 100264 mask: 3 PS I used cpumat_t* array to store these values instead of int, because if the idea is OK, I'll extend it to support ranges like e.g. cpuset(1) supports, e.g.: "1:2-5". The questions are: - Is it OK to chance '-p' arg syntax or it's better to introduce a new one? - Is the syntax OK (currently: 'vcpu:pcpu', later 'vcpu:pcpuN-pcpuM,pcpuX")? Roman Bogorodskiy Index: bhyverun.c === --- bhyverun.c (revision 264921) +++ bhyverun.c (working copy) @@ -83,7 +83,6 @@ int guest_ncpus; -static int pincpu = -1; static int guest_vmexit_on_hlt, guest_vmexit_on_pause, disable_x2apic; static int virtio_msix = 1; @@ -119,6 +118,8 @@ int mt_vcpu; } mt_vmm_info[VM_MAXCPU]; +cpuset_t *vcpumap[VM_MAXCPU] = { NULL }; + static void usage(int code) { @@ -146,6 +147,56 @@ exit(code); } +static int +pincpu_parse(const char *cpupin) +{ +char *token, *string; +size_t i = 0; +int vcpu = -1, pcpu = -1, ret = -1; +cpuset_t *mask; + +if ((string = strdup(cpupin)) == NULL) { +fprintf(stderr, "strdup failed: %d\n", errno); +return -1; +} + +while ((token = strsep(&string, ":")) != NULL) { +switch (i) { +case 0: +vcpu = atoi(token); +break; +case 1: +pcpu = atoi(token); +break; +default: +fprintf(stderr, "invalid format: %s\n", token); +goto cleanup; +} +i++; +} + +if (vcpu == -1 || pcpu == -1) { +fprintf(stderr, "invalid format: %s\n", cpupin); +goto cleanup; +} + +if ((mask = malloc(sizeof(cpuset_t))) == NULL) { +fprintf(stderr, "malloc failed: %d\n", errno); +goto cleanup; +} +CPU_ZERO(mask); +CPU_SET(pcpu, mask); + +vcpumap[vcpu] = mask; + +ret = 0; + +cleanup: +free(string); + +return ret; +} + void * paddr_guest2host(struct vmctx *ctx, uintptr_t gaddr, size_t len) { @@ -479,15 +530,13 @@ static void vm_loop(struct vmctx *ctx, int vcpu, uint64_t rip) { - cpuset_t mask; int error, rc, prevcpu; enum vm_exitcode exitcode; - if (pincpu >= 0) { - CPU_ZERO(&mask); - CPU_SET(pincpu + vcpu, &mask); + if (vcpumap[vcpu] != NULL) { error = pthread_setaffinity_np(pthread_self(), - sizeof(mask), &mask); + sizeof(cpuset_t), + vcpumap[vcpu]); assert(error == 0); } @@ -622,7 +671,10 @@ bvmcons = 1; break; case 'p': - pincpu = atoi(optarg); +if (pincpu_parse(optarg) < 0) { +errx(EX_USAGE, "invalid cpu pin " + "configuration '%s'", optarg); +} break; case 'c': guest_ncpus = atoi(optarg); pgpxeZu8t9Fjn.pgp Description: PGP signature