Date: Mon, 5 May 2014 11:14:00 -0700 From: Neel Natu <neelnatu@gmail.com> To: Roman Bogorodskiy <novel@freebsd.org> Cc: "freebsd-virtualization@freebsd.org" <freebsd-virtualization@freebsd.org> Subject: Re: [PATCH] Flexible vcpu pinning configuration Message-ID: <CAFgRE9HxZQBwMTgPPNYJ%2Bi0KQmuSKqzz2MAyu0GEsQtLNMed6Q@mail.gmail.com> In-Reply-To: <20140501143005.GA91029@kloomba> References: <20140427104511.GA7804@kloomba> <CAFgRE9FUHEXbnkTSVmpyOP7EMzQh7QEFeNJikVWw6xHRWbvQSw@mail.gmail.com> <20140501143005.GA91029@kloomba>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Roman,
On Thu, May 1, 2014 at 7:30 AM, Roman Bogorodskiy <novel@freebsd.org> wrote=
:
> Neel Natu wrote:
>
>> Hi Roman,
>>
>> On Sun, Apr 27, 2014 at 3:45 AM, Roman Bogorodskiy <novel@freebsd.org> w=
rote:
>> > I've created an initial version of the patch which allows more flexibl=
e
>> > 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, becaus=
e
>> > 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 ne=
w
>> > 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) =3D=3D 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] !=3D NULL)
>> mask =3D vcpumap[vcpu];
>> else
>> mask =3D 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=3Drevision&revision=3D265376
>
> 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 =E2=80=A6 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_deletec=
pu()
> error message says fprintf(stderr, "addcpu: .... Should it be "deletecpu:=
" instead?
Thanks. This is fixed now:
http://svnweb.freebsd.org/base?view=3Drevision&revision=3D265366
best
Neel
>
> Roman Bogorodskiy
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFgRE9HxZQBwMTgPPNYJ%2Bi0KQmuSKqzz2MAyu0GEsQtLNMed6Q>