Re: [Xen-devel] [PATCH v4 13/15] xl: enable getting and setting soft

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-11-22 at 19:58 +0100, Dario Faggioli wrote:
> Getting happens via `xl vcpu-list', which now looks like this:
> 
>  # xl vcpu-list -s
>  Name       ID VCPU CPU State Time(s) Affinity (Hard / Soft)
>  Domain-0   0     0  11  -b-     5.4  8-15 / all
>  Domain-0   0     1  11  -b-     1.0  8-15 / all
>  Domain-0   0    14  13  -b-     1.4  8-15 / all
>  Domain-0   0    15   8  -b-     1.6  8-15 / all
>  vm-test    3     0   4  -b-     2.5  0-12 / 0-7
>  vm-test    3     1   0  -b-     3.2  0-12 / 0-7
> 
> Setting happens by adding a '-s'/'--soft' switch to `xl vcpu-pin'.
> 
> xl manual page is updated accordingly.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> ---
> Changes from v3:
>  * fix typos in doc, rephrased the help message and changed
>    the title of the column for hard/soft affinity, as suggested
>    during review.
> 
> Changes from v2:
>  * this patch folds what in v2 were patches 13 and 14;
>  * `xl vcpu-pin' always shows both had and soft affinity,
>    without the need of passing '-s'.
> ---
>  docs/man/xl.pod.1         |   24 +++++++++++++++
>  tools/libxl/xl_cmdimpl.c  |   70 
> +++++++++++++++++++++++++++------------------
>  tools/libxl/xl_cmdtable.c |    3 +-
>  3 files changed, 67 insertions(+), 30 deletions(-)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index e7b9de2..b738ca2 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -619,7 +619,7 @@ after B<vcpu-set>, go to B<SEE ALSO> section for 
> information.
>  Lists VCPU information for a specific domain.  If no domain is
>  specified, VCPU information for all domains will be provided.
>  
> -=item B<vcpu-pin> I<domain-id> I<vcpu> I<cpus>
> +=item B<vcpu-pin> [I<OPTIONS>] I<domain-id> I<vcpu> I<cpus>
>  
>  Pins the VCPU to only run on the specific CPUs.  The keyword
>  B<all> can be used to apply the I<cpus> list to all VCPUs in the
> @@ -630,6 +630,28 @@ different run state is appropriate.  Pinning can be used 
> to restrict
>  this, by ensuring certain VCPUs can only run on certain physical
>  CPUs.
>  
> +B<OPTIONS>
> +
> +=over 4
> +
> +=item B<-s>, B<--soft>
> +
> +The same as above, but affect I<soft affinity> rather than pinning
> +(also called I<hard affinity>).
> +
> +Normally, VCPUs just wander among the CPUs where it is allowed to

s/it is/they are/

I'd usually avoid terms such as "wander" in formal writing. Perhaps:
        "A VCPUs is normally scheduled among a set of PCPUs."

> +run (either all the CPUs or the ones to which it is pinned, as said
> +for B<vcpu-list>). Soft affinity offers a means to specify one or

s/, as said for B<vcpu-list>// unless you think that adds something? I
can't see why vcpu-list is relevant though.

Or just "(subject to pinning)" would be a less wordy way to get the
message across.

> +more I<preferred> CPUs. Basically, among the ones where it can run,
> +the VCPU the VCPU will greatly prefer to execute on one of these
> +CPUs, whenever that is possible.

"...one or more I<preferred> CPUs where the VCPU will prefer to run
whenever possible".

> +
> +Notice that, in order for soft affinity to actually work, it needs
> +special support in the scheduler. Right now, only credit1 provides
> +that.
> +
> +=back
> +
>  =item B<vm-list>
>  
>  Prints information about guests. This list excludes information about
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 17fffdd..d6fda26 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4630,23 +4649,29 @@ static int vcpupin(uint32_t domid, const char *vcpu, 
> char *cpu)
>      }
>  
>      if (vcpuid != -1) {
> -        if (libxl_set_vcpuaffinity(ctx, domid, vcpuid, &cpumap, NULL)) {
> -            fprintf(stderr, "Could not set affinity for vcpu `%u'.\n", 
> vcpuid);
> +        if (!soft_affinity &&
> +            libxl_set_vcpuaffinity(ctx, domid, vcpuid, &cpumap, NULL)) {
> +            fprintf(stderr, "Could not set hard affinity for vcpu `%u'.\n",
> +                    vcpuid);
> +            goto out;
> +        } else if (soft_affinity &&
> +                   libxl_set_vcpuaffinity(ctx, domid, vcpuid, NULL, 
> &cpumap)) {
> +            fprintf(stderr, "Could not set soft affinity for vcpu `%u'.\n",
> +                    vcpuid);

I think I would have written this as:
        libxl_bitmap *soft = NULL, *hard = NULL;
        if (soft_affinity)
                soft = &cpumap
        else
                hard = &cpumap

The in both of the branches "if (vcpu != -1)":
        libxl_set_vcpuaffinity(..., hard, soft)
                error handling

I think you can omit the soft/hard from the log message other logging
and/or the fact that they know what they asked for will cover the
distinction. If you really wanted the const char *what = soft_affinity ?
"soft" : "hard" to use.

Is there no option to set both?



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel