Date: Thu, 1 May 2014 18:30:06 +0400 From: Roman Bogorodskiy <novel@FreeBSD.org> To: Neel Natu <neelnatu@gmail.com> Cc: "freebsd-virtualization@freebsd.org" <freebsd-virtualization@freebsd.org> Subject: Re: [PATCH] Flexible vcpu pinning configuration Message-ID: <20140501143005.GA91029@kloomba> In-Reply-To: <CAFgRE9FUHEXbnkTSVmpyOP7EMzQh7QEFeNJikVWw6xHRWbvQSw@mail.gmail.com> References: <20140427104511.GA7804@kloomba> <CAFgRE9FUHEXbnkTSVmpyOP7EMzQh7QEFeNJikVWw6xHRWbvQSw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--tjCHc7DPkfUGtrlw
Content-Type: multipart/mixed; boundary="YiEDa0DAkWCtVeE4"
Content-Disposition: inline
--YiEDa0DAkWCtVeE4
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Neel Natu wrote:
> Hi Roman,
>=20
> On Sun, Apr 27, 2014 at 3:45 AM, Roman Bogorodskiy <novel@freebsd.org> wr=
ote:
> > 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?
> >
>=20
> I think we can reuse the "-p" option unless anybody objects vociferously.
>=20
> > - Is the syntax OK (currently: 'vcpu:pcpu', later
> > 'vcpu:pcpuN-pcpuM,pcpuX")?
>=20
> Yup, I think that works fine.
>=20
> The patch looks good in general but I have a few comments:
>=20
> - Scope of 'vcpupmap[]' should be restricted to 'static'.
>=20
> - usage() and man page need to be updated.
>=20
> - pincpu_parse():
> The option parsing can be made much easier by using:
>=20
> if (sscanf(str, "%d:%d", &vcpu, &pcpu) =3D=3D 2) {
> /* success */
> } else {
> return (-1);
> }
>=20
> If the same vcpu is specified multiple times then we should
> malloc(sizeof(cpuset_t)) only the first time:
>=20
> if (vcpumap[vcpu] !=3D NULL)
> mask =3D vcpumap[vcpu];
> else
> mask =3D malloc(sizeof(cpuset_t));
>=20
> We need to range-check 'vcpu' before using it as an index into the
> 'vcpumap[]' array.
>=20
> 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
--YiEDa0DAkWCtVeE4
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="bhyve_vcpupin2.diff"
Content-Transfer-Encoding: quoted-printable
Index: bhyve.8
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- 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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- bhyverun.c (revision 264921)
+++ bhyverun.c (working copy)
@@ -83,7 +83,6 @@
=20
int guest_ncpus;
=20
-static int pincpu =3D -1;
static int guest_vmexit_on_hlt, guest_vmexit_on_pause, disable_x2apic;
static int virtio_msix =3D 1;
=20
@@ -119,6 +118,8 @@
int mt_vcpu;=09
} mt_vmm_info[VM_MAXCPU];
=20
+static cpuset_t *vcpumap[VM_MAXCPU] =3D { NULL };
+
static void
usage(int code)
{
@@ -125,12 +126,12 @@
=20
fprintf(stderr,
"Usage: %s [-aehwAHIPW] [-g <gdb port>] [-s <pci>] [-S <pc=
i>]\n"
- " %*s [-c vcpus] [-p pincpu] [-m mem] [-l <lpc>] <vm>\n"
+ " %*s [-c vcpus] [-p vcpu:hostcpu] [-m mem] [-l <lpc>] <vm>\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: <n:m> 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);
}
=20
+static int
+pincpu_parse(const char *cpupin)
+{
+ char *string;
+ int vcpu =3D -1, pcpu =3D -1, ret =3D -1;
+
+ if ((string =3D strdup(cpupin)) =3D=3D NULL) {
+ fprintf(stderr, "strdup failed: %d\n", errno);
+ return -1;
+ }
+
+ if (sscanf(string, "%d:%d", &vcpu, &pcpu) !=3D 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;
+ }
+
+ if (vcpumap[vcpu] =3D=3D NULL) {
+ if ((vcpumap[vcpu] =3D malloc(sizeof(cpuset_t))) =3D=3D NULL) {
+ fprintf(stderr, "malloc failed: %d\n", errno);
+ goto cleanup;
+ }
+ CPU_ZERO(vcpumap[vcpu]);
+ }
+ CPU_SET(pcpu, vcpumap[vcpu]);
+
+ ret =3D 0;
+
+cleanup:
+ free(string);
+
+ return ret;
+}
+
void *
paddr_guest2host(struct vmctx *ctx, uintptr_t gaddr, size_t len)
{
@@ -479,15 +527,13 @@
static void
vm_loop(struct vmctx *ctx, int vcpu, uint64_t rip)
{
- cpuset_t mask;
int error, rc, prevcpu;
enum vm_exitcode exitcode;
=20
- if (pincpu >=3D 0) {
- CPU_ZERO(&mask);
- CPU_SET(pincpu + vcpu, &mask);
+ if (vcpumap[vcpu] !=3D NULL) {
error =3D pthread_setaffinity_np(pthread_self(),
- sizeof(mask), &mask);
+ sizeof(cpuset_t),
+ vcpumap[vcpu]);
assert(error =3D=3D 0);
}
=20
@@ -622,7 +668,10 @@
bvmcons =3D 1;
break;
case 'p':
- pincpu =3D atoi(optarg);
+ if (pincpu_parse(optarg) < 0) {
+ errx(EX_USAGE, "invalid cpu pin "
+ "configuration '%s'", optarg);
+ }
break;
case 'c':
guest_ncpus =3D atoi(optarg);
--YiEDa0DAkWCtVeE4--
--tjCHc7DPkfUGtrlw
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (FreeBSD)
iQEcBAEBAgAGBQJTYlptAAoJEMltX/4IwiJqEkoH/ixfHJaNREQrMECNKv+B7R1i
YJSS3TesSLCiCzKUA887+i+DMgXfbvKu/xMciS6Vllj6ejIONMlrEQgRF9tUO23J
p1GRcQRzNXsmhSLlBhZL/HA2LoZN8UStj2vScKYMtAYotFyoXwc024mVy8t6q4wU
vllCl21Pa3TAjVYPpzlrKQAadhZXRTIr2U0BDmUWFeMl553U/ZOlT/d/TBDhSeYG
4VVS4eTK7oDkK8/XRPq4bQvswx5KmFGp1Sy0uoVSbUCVYQ6oNg2TEWJtpa+spPl8
SCgiK15s2lHaB/UeAQkSre/sJ/I0Gy5lZ5R/Ncn12tfu0OrCO4YSvZuGPKwKFAA=
=Ix2V
-----END PGP SIGNATURE-----
--tjCHc7DPkfUGtrlw--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140501143005.GA91029>