From: Dario Faggioli <dario.faggioli@citrix.com>
To: Chong Li <lichong659@gmail.com>, xen-devel@lists.xen.org
Cc: Chong Li <chong.li@wustl.edu>, Sisu Xi <xisisu@gmail.com>,
george.dunlap@eu.citrix.com, Meng Xu <mengxu@cis.upenn.edu>,
jbeulich@suse.com, dgolomb@seas.upenn.edu
Subject: Re: [PATCH v7 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
Date: Thu, 17 Mar 2016 11:03:30 +0100 [thread overview]
Message-ID: <1458209010.15374.20.camel@citrix.com> (raw)
In-Reply-To: <1458146871-2813-2-git-send-email-lichong659@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 23793 bytes --]
On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1054,15 +1054,16 @@ csched_dom_cntl(
> * lock. Runq lock not needed anywhere in here. */
> spin_lock_irqsave(&prv->lock, flags);
>
> - if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
> + switch ( op->cmd )
> {
> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + return -EINVAL;
> + case XEN_DOMCTL_SCHEDOP_getinfo:
> op->u.credit.weight = sdom->weight;
> op->u.credit.cap = sdom->cap;
>
Not feeling to strong about it, but I think
switch ( op->cmd )
{
case XEN_DOMCTL_SCHEDOP_getinfo:
op->u.credit.weight = sdom->weight;
op->u.credit.cap = sdom->cap;
break;
case XEN_DOMCTL_SCHEDOP_putinfo:
if ( op->u.credit.weight != 0 )
{
if ( !list_empty(&sdom->active_sdom_elem) )
{
prv->weight -= sdom->weight * sdom->active_vcpu_count;
prv->weight += op->u.credit.weight * sdom->active_vcpu_count;
}
sdom->weight = op->u.credit.weight;
}
if ( op->u.credit.cap != (uint16_t)~0U )
sdom->cap = op->u.credit.cap;
break;
case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
default:
return -EINVAL;
}
(i.e., grouping the cases that needs only returning -EINVAL) is better,
the final result, more than the patch itself.
And the same for Credit2, of course.
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -1129,24 +1145,22 @@ rt_dom_cntl(
> struct vcpu *v;
> unsigned long flags;
> int rc = 0;
> + xen_domctl_schedparam_vcpu_t local_sched;
> + s_time_t period, budget;
> + uint32_t index = 0;
>
> switch ( op->cmd )
> {
> case XEN_DOMCTL_SCHEDOP_getinfo:
> - if ( d->max_vcpus > 0 )
> - {
> - spin_lock_irqsave(&prv->lock, flags);
> - svc = rt_vcpu(d->vcpu[0]);
> - op->u.rtds.period = svc->period / MICROSECS(1);
> - op->u.rtds.budget = svc->budget / MICROSECS(1);
> - spin_unlock_irqrestore(&prv->lock, flags);
> - }
> - else
> - {
> - /* If we don't have vcpus yet, let's just return the
> defaults. */
> - op->u.rtds.period = RTDS_DEFAULT_PERIOD;
> - op->u.rtds.budget = RTDS_DEFAULT_BUDGET;
> - }
> + /* Return the default parameters.
> + * A previous bug fixed here:
> + * The default PERIOD or BUDGET should be divided by
> MICROSECS(1),
> + * before returned to upper caller.
> + */
Comment style (missing opening '/*').
Also, putting this kind of things in comments is not at all ideal. If
doing this in this patch, you should mention it in the changelog. Or
you do it in a separate patch (that you put before this one in the
series).
I'd say that, in this specific case, is not a big deal which one of the
two approaches you take (mentioning or separate patch), but the having
a separate one is indeed almost always preferable (e.g., the fix can be
backported).
> + spin_lock_irqsave(&prv->lock, flags);
> + op->u.rtds.period = RTDS_DEFAULT_PERIOD / MICROSECS(1);
> + op->u.rtds.budget = RTDS_DEFAULT_BUDGET / MICROSECS(1);
> + spin_unlock_irqrestore(&prv->lock, flags);
>
I don't think we need to take the lock here... RTDS_DEFAULT_PERIOD is
not going to change under our feet. :-)
> @@ -1163,6 +1177,78 @@ rt_dom_cntl(
> }
> spin_unlock_irqrestore(&prv->lock, flags);
> break;
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + while ( index < op->u.v.nr_vcpus )
> + {
> + if ( copy_from_guest_offset(&local_sched,
> + op->u.v.vcpus, index, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + if ( local_sched.vcpuid >= d->max_vcpus ||
> + d->vcpu[local_sched.vcpuid] == NULL )
> + {
> + rc = -EINVAL;
> + break;
> + }
> +
> + spin_lock_irqsave(&prv->lock, flags);
> + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> + local_sched.u.rtds.budget = svc->budget / MICROSECS(1);
> + local_sched.u.rtds.period = svc->period / MICROSECS(1);
> + spin_unlock_irqrestore(&prv->lock, flags);
> +
> + if ( copy_to_guest_offset(op->u.v.vcpus, index,
> + &local_sched, 1) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> + if ( (++index > 0x3f) && hypercall_preempt_check() )
> + break;
>
So, I know it's 0x3f in XEN_SYSCTL_pcitopoinfo, but unless I'm missing
something, I don't see why this can't be "63".
I'd also put a short comment right above, something like:
/* Process a most 64 vCPUs without checking for preemptions. */
> + }
> + if ( !rc ) /* notify upper caller how many vcpus have been
> processed */
>
Move the comment to the line above (and terminate it with a full stop).
And by the way, there's a lot of code repetition here. What about
handling get and set together, sort of like this:
case XEN_DOMCTL_SCHEDOP_getvcpuinfo: |
case XEN_DOMCTL_SCHEDOP_putvcpuinfo: |#ifdef CONFIG_HAS_PCI
while ( index < op->u.v.nr_vcpus ) | case XEN_SYSCTL_pcitopoinfo:
{ | {
if ( copy_from_guest_offset(&local_sched, | xen_sysctl_pcitopoinfo_t *ti = &op->u.pcitopoinfo;
op->u.v.vcpus, index, 1) ) | unsigned int i = 0;
{ |
rc = -EFAULT; | if ( guest_handle_is_null(ti->devs) ||
break; | guest_handle_is_null(ti->nodes) )
} | {
| ret = -EINVAL;
if ( local_sched.vcpuid >= d->max_vcpus || | break;
d->vcpu[local_sched.vcpuid] == NULL ) | }
{ |
rc = -EINVAL; | while ( i < ti->num_devs )
break; | {
} | physdev_pci_device_t dev;
| uint32_t node;
if ( op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo ) | const struct pci_dev *pdev;
{ |
spin_lock_irqsave(&prv->lock, flags); | if ( copy_from_guest_offset(&dev, ti->devs, i, 1) )
svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); | {
local_sched.u.rtds.budget = svc->budget / MICROSECS(1); | ret = -EFAULT;
local_sched.u.rtds.period = svc->period / MICROSECS(1); | break;
spin_unlock_irqrestore(&prv->lock, flags); | }
|
if ( copy_to_guest_offset(op->u.v.vcpus, index, | pcidevs_lock();
&local_sched, 1) ) | pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn);
{ | if ( !pdev )
rc = -EFAULT; | node = XEN_INVALID_DEV;
break; | else if ( pdev->node == NUMA_NO_NODE )
} | node = XEN_INVALID_NODE_ID;
} | else
else | node = pdev->node;
{ | pcidevs_unlock();
period = MICROSECS(local_sched.u.rtds.period); |
budget = MICROSECS(local_sched.u.rtds.budget); | if ( copy_to_guest_offset(ti->nodes, i, &node, 1) )
if ( period > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET || | {
budget > period || period < RTDS_MIN_PERIOD ) | ret = -EFAULT;
{ | break;
rc = -EINVAL; | }
break; |
} | /* Process a most 64 vCPUs without checking for preemptions. */
| if ( (++i > 0x3f) && hypercall_preempt_check() )
spin_lock_irqsave(&prv->lock, flags); | break;
svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); | }
svc->period = period; |
svc->budget = budget; | if ( !ret && (ti->num_devs != i) )
spin_unlock_irqrestore(&prv->lock, flags); | {
| ti->num_devs = i;
} | if ( __copy_field_to_guest(u_sysctl, op, u.pcitopoinfo.num_devs) )
/* Process a most 64 vCPUs without checking for preemptions. */ | ret = -EFAULT;
if ( (++index > 63) && hypercall_preempt_check() ) | }
break; | break;
} | }
|#endif
/* Notify upper caller how many vcpus have been processed. */ |
if ( !rc ) | case XEN_SYSCTL_tmem_op:
op->u.v.nr_vcpus = index; | ret = tmem_control(&op->u.tmem_op);
break;
I have only compile tested this, but it looks to me that it can fly...
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> + * Set or get info?
> + * For schedulers supporting per-vcpu settings (e.g., RTDS):
> + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
> + * XEN_DOMCTL_SCHEDOP_getinfo gets default params;
> + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo sets (gets) params of vcpus;
> + *
> + * For schedulers not supporting per-vcpu settings:
> + * XEN_DOMCTL_SCHEDOP_putinfo sets params for all vcpus;
> + * XEN_DOMCTL_SCHEDOP_getinfo gets domain-wise params;
> + * XEN_DOMCTL_SCHEDOP_put(get)vcpuinfo returns error;
> + */
> #define XEN_DOMCTL_SCHEDOP_putinfo 0
> #define XEN_DOMCTL_SCHEDOP_getinfo 1
> +#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2
> +#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3
> struct xen_domctl_scheduler_op {
> uint32_t sched_id; /* XEN_SCHEDULER_* */
> uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */
/* IN/OUT */
> union {
> - struct xen_domctl_sched_credit {
> - uint16_t weight;
> - uint16_t cap;
> - } credit;
> - struct xen_domctl_sched_credit2 {
> - uint16_t weight;
> - } credit2;
> - struct xen_domctl_sched_rtds {
> - uint32_t period;
> - uint32_t budget;
> - } rtds;
> + xen_domctl_sched_credit_t credit;
> + xen_domctl_sched_credit2_t credit2;
> + xen_domctl_sched_rtds_t rtds;
> + struct {
> + XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
> + /*
> + * IN: Number of elements in vcpus array.
> + * OUT: Number of processed elements of vcpus array.
> + */
> + uint32_t nr_vcpus;
> + uint32_t padding;
> + } v;
> } u;
> };
>
That is: make it even more clear that the whole union is used as
IN/OUT.
Then, indeed, inside v, what is the meaning of the nr_vcpus field in
each direction, as you're doing already.
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-03-17 10:03 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-16 16:47 [PATCH v7 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 1/4] xen: enable " Chong Li
2016-03-17 10:03 ` Dario Faggioli [this message]
2016-03-17 20:42 ` Chong Li
2016-03-18 7:39 ` Jan Beulich
2016-03-18 10:47 ` Dario Faggioli
2016-03-18 20:22 ` Chong Li
2016-03-18 7:48 ` Jan Beulich
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 2/4] libxc: " Chong Li
2016-03-16 19:24 ` Wei Liu
2016-03-17 2:28 ` Dario Faggioli
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 3/4] libxl: " Chong Li
2016-03-16 19:24 ` Wei Liu
2016-03-17 4:05 ` Dario Faggioli
2016-03-17 19:50 ` Chong Li
2016-03-18 9:17 ` Dario Faggioli
2016-03-17 4:29 ` Dario Faggioli
2016-03-16 16:47 ` [PATCH v7 for Xen 4.7 4/4] xl: " Chong Li
2016-03-16 19:24 ` Wei Liu
2016-03-16 19:29 ` Wei Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1458209010.15374.20.camel@citrix.com \
--to=dario.faggioli@citrix.com \
--cc=chong.li@wustl.edu \
--cc=dgolomb@seas.upenn.edu \
--cc=george.dunlap@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=lichong659@gmail.com \
--cc=mengxu@cis.upenn.edu \
--cc=xen-devel@lists.xen.org \
--cc=xisisu@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).