On 08/03/2012 12:36 AM, Hu Tao wrote:
> From: Tang Chen <tangchen@xxxxxxxxxxxxxx>
>
> Modify vcpupin command to support hypervisor threads pin.
> 1) add "--hypervisor" option to get hypervisor threads info.
> 2) add "--hypervisor cpuset" to set hypervisor threads to specified cpuset.
Dan's comment of s/hypervisor/emulator/ applies here as well.
>
> Signed-off-by: Tang Chen <tangchen@xxxxxxxxxxxxxx>
> Signed-off-by: Hu Tao <hutao@xxxxxxxxxxxxxx>
> ---
> tests/vcpupin | 6 +--
> tools/virsh-domain.c | 147 +++++++++++++++++++++++++++++++++-----------------
> tools/virsh.pod | 12 +++--
> 3 files changed, 109 insertions(+), 56 deletions(-)
>
> diff --git a/tests/vcpupin b/tests/vcpupin
> index f1fb038..e55806e 100755
> --- a/tests/vcpupin
> +++ b/tests/vcpupin
> @@ -31,16 +31,16 @@ fi
> fail=0
>
> # Invalid syntax.
> -$abs_top_builddir/tools/virsh --connect test:///default vcpupin test a 0,1 > out 2>&1
> +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test a --vcpu 0,1 > out 2>&1
Why did you have to change this example invocation? If you broke
back-compat with existing clients, then we need to rethink how to add
the new functionality into virsh. Since this was a negative test (where
the command used to fail), if the command now succeeds where it used to
fail, and adding '--vcpu' preserves the negative test aspect, I could
understand it; but that doesn't appear to be the case here.
> test $? = 1 || fail=1
> cat <<\EOF > exp || fail=1
> -error: vcpupin: Invalid or missing vCPU number.
> +error: vcpupin: Invalid or missing vCPU number, or missing --hypervisor option.
Changing an error message is okay, though.
>
> EOF
> compare exp out || fail=1
>
> # An out-of-range vCPU number deserves a diagnostic, too.
> -$abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 0,1 > out 2>&1
> +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test --vcpu 100 0,1 > out 2>&1
Again, you shouldn't be needing to modify this test.
> test $? = 1 || fail=1
> cat <<\EOF > exp || fail=1
> error: vcpupin: Invalid vCPU number.
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 33b1727..74603e8 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -4373,14 +4373,15 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd)
> * "vcpupin" command
> */
> static const vshCmdInfo info_vcpupin[] = {
> - {"help", N_("control or query domain vcpu affinity")},
> - {"desc", N_("Pin domain VCPUs to host physical CPUs.")},
> + {"help", N_("control or query domain vcpu or hypervisor threads affinities")},
> + {"desc", N_("Pin domain VCPUs or hypervisor threads to host physical CPUs.")},
> {NULL, NULL}
> };
>
> static const vshCmdOptDef opts_vcpupin[] = {
> {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> - {"vcpu", VSH_OT_INT, 0, N_("vcpu number")},
> + {"vcpu", VSH_OT_INT, VSH_OFLAG_REQ_OPT, N_("vcpu number")},
This is the broken change - you can't require existing users to start
typing --vcpu.
> + {"hypervisor", VSH_OT_BOOL, VSH_OFLAG_REQ_OPT, N_("pin hypervisor threads")},
bool options cannot require an option, so using VSH_OFLAG_REQ_OPT is
wrong here (hmm, virsh.c's self-check validation should probably be
erroring out on this combination).
I think what you want to end up with is:
vcpupin dom
=> query ALL pinnings
vcpupin dom --emulator
=> query just the emulator pinning
vcpupin dom [--vcpu] number
=> query just vcpuN pinning
vcpupin dom --emulator cpulist
=> set the emulator pinning to cpulist
vcpupin dom [--vcpu] number cpulist
=> set the vcpuN pinning to cpulist
Or in virsh.pod syntax:
vcpupin <domain> [ { [--vcpu] <number> | <--emulator> } [<cpulist>] ]
[--config] [--live] [--current]
such that either --emulator or an integer but not both must be present
if cpulist is also present; and that an integer can optionally be
prefixed with --vcpu. There's no way to write that directly in
opts_vcpupin, so it requires listing both options as optional, then
manual checks in the function body that exactly one of the two options
was present.
Hmm, I see a potential gotcha: the parser would treat:
vcpupin dom --emulator 0
as assigning '0' to --vcpu, instead of to --cpulist, so users of
--emulator would always have to spell out:
vcpupin dom --emulator --cpulist 0
but since that's a new command, at least we wouldn't be breaking
back-compat.
Maybe a solution would be to take --vcpu -1 as the special case of
requesting the emulator pinning, instead of adding a new flag
--emulator. That makes sense if we do the same overloading at the
libvirt API level.
Or maybe the solution is to add a new command 'virsh emulatorpin' for
emulator pinning, instead of trying to overload 'virsh vcpupin',
especially, since you have proposed separate libvirt APIs. That does
mean you can't list all pinnings in one command, but that's not too
tough of a restriction.
I'm still thinking about the best way to resolve this UI issue, and I
think part of the answer is determined by the API answer in 9/17.
> +/*
> + * Helper function to print vcpupin and hypervisorpin info.
> + */
> +static bool
> +printPinInfo(unsigned char *cpumaps, size_t cpumaplen,
> + int maxcpu, int vcpuindex)
We should probably name this vshPrintPinInfo, to keep our helper
functions in the virsh namespace.
For patch review purposes, it would help to split this into two patches
- one that refactors code to introduce this new function and call it
from the old location but has no semantic changes, and then one to add
the new options and/or command and becomes a second client of the helper
function.
> static bool
> cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
> {
> @@ -4401,13 +4441,13 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
> unsigned char *cpumap = NULL;
> unsigned char *cpumaps = NULL;
> size_t cpumaplen;
> - bool bit, lastbit, isInvert;
> - int i, cpu, lastcpu, maxcpu, ncpus;
> + int i, cpu, lastcpu, maxcpu, ncpus, nhyper;
Why nhyper? There is only one emulator pinning value.
> + if (hypervisor && vcpu != -1) {
> + vshError(ctl, "%s", _("vcpupin: --hypervisor must be specified "
> + "exclusively to --vcpu."));
reads better as:
--hypervisor and --vcpu cannot both be specified
> + if (vcpu == -1) {
> + cpumaps = vshMalloc(ctl, cpumaplen);
> + if ((nhyper = virDomainGetHypervisorPinInfo(dom, cpumaps,
> + cpumaplen, flags)) >= 0) {
Why >= 0? Either it is 0 (no pinning known) or 1 (exactly one pinning
known).
> +++ b/tools/virsh.pod
> @@ -1592,12 +1592,16 @@ Thus, this command always takes exactly zero or two flags.
> Returns basic information about the domain virtual CPUs, like the number of
> vCPUs, the running time, the affinity to physical processors.
>
> -=item B<vcpupin> I<domain> [I<vcpu>] [I<cpulist>] [[I<--live>]
> -[I<--config>] | [I<--current>]]
> +=item B<vcpupin> I<domain-id> [I<vcpu>] [I<hypervisor>] [I<cpulist>]
You are introducing a regression on the spelling of 'I<domain>'.
--hypervisor is a bool, so it should be listed as <--hypervisor>. See
my above attempt at describing this virsh.pod layout using {} to
indicate mutual exclusion, if we decide to overload this command rather
than adding a new one.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list