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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.