From: "Jan Beulich" <JBeulich@suse.com>
To: Chong Li <lichong659@gmail.com>
Cc: Chong Li <chong.li@wustl.edu>, Sisu Xi <xisisu@gmail.com>,
george.dunlap@eu.citrix.com, dario.faggioli@citrix.com,
xen-devel@lists.xen.org, Meng Xu <mengxu@cis.upenn.edu>,
dgolomb@seas.upenn.edu
Subject: Re: [PATCH v6 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
Date: Mon, 07 Mar 2016 05:59:36 -0700 [thread overview]
Message-ID: <56DD894802000078000D9F84@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1457286958-5427-2-git-send-email-lichong659@gmail.com>
>>> On 06.03.16 at 18:55, <lichong659@gmail.com> wrote:
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1054,6 +1054,10 @@ csched_dom_cntl(
> * lock. Runq lock not needed anywhere in here. */
> spin_lock_irqsave(&prv->lock, flags);
>
> + if ( op->cmd == XEN_DOMCTL_SCHEDOP_putvcpuinfo ||
> + op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo )
> + return -EINVAL;
> +
> if ( op->cmd == XEN_DOMCTL_SCHEDOP_getinfo )
> {
> op->u.credit.weight = sdom->weight;
Considering the rest of the code following where, I would - albeit
I'm not maintainer of this code - strongly suggest moving to
switch() in such cases, with the default case returning -EINVAL (or
maybe better -EOPNOTSUPP).
> @@ -1130,23 +1146,17 @@ rt_dom_cntl(
> unsigned long flags;
> int rc = 0;
>
> + xen_domctl_schedparam_vcpu_t local_sched;
> + s_time_t period, budget;
> + uint32_t index = 0;
> +
There's a stray blank line left ahead of this addition.
> 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;
> - }
> + case XEN_DOMCTL_SCHEDOP_getinfo: /* return the default parameters */
> + 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);
> break;
This alters the values returned when d->max_vcpus == 0 - while
this looks to be intentional, I think calling out such a bug fix in the
description is a must.
> @@ -1163,6 +1173,96 @@ rt_dom_cntl(
> }
> spin_unlock_irqrestore(&prv->lock, flags);
> break;
> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> + if ( guest_handle_is_null(op->u.v.vcpus) )
> + {
> + rc = -EINVAL;
Perhaps rather -EFAULT? But then again - what is this check good for
(considering that it doesn't cover other obviously bad handle values)?
> + break;
> + }
> + while ( index < op->u.v.nr_vcpus )
> + {
> + if ( copy_from_guest_offset(&local_sched,
> + op->u.v.vcpus, index, 1) )
Indentation.
> + {
> + rc = -EFAULT;
> + break;
> + }
> + if ( local_sched.vcpuid >= d->max_vcpus ||
> + d->vcpu[local_sched.vcpuid] == NULL )
Again. And more below.
> + {
> + rc = -EINVAL;
> + break;
> + }
> +
> + spin_lock_irqsave(&prv->lock, flags);
> + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
> + local_sched.s.rtds.budget = svc->budget / MICROSECS(1);
> + local_sched.s.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 how is the caller going to be able to reliably read all vCPU-s'
information for a guest with more than 64 vCPU-s?
> + }
> +
> + if ( !rc && (op->u.v.nr_vcpus != index) )
> + op->u.v.nr_vcpus = index;
I don't think the right side of the && is really necessary / useful.
> + break;
> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
When switch statements get large, please put blank lines between
individual case blocks.
> + if ( guest_handle_is_null(op->u.v.vcpus) )
> + {
> + rc = -EINVAL;
> + break;
> + }
> + 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;
> + }
> +
> + period = MICROSECS(local_sched.s.rtds.period);
> + budget = MICROSECS(local_sched.s.rtds.budget);
> + if ( period > RTDS_MAX_PERIOD || budget < RTDS_MIN_BUDGET ||
> + budget > period || period < RTDS_MIN_PERIOD )
> + {
> + rc = -EINVAL;
> + break;
> + }
> +
> + /*
> + * We accept period/budget less than 100 us, but will warn users about
> + * the large scheduling overhead due to it
> + */
> + if ( period < MICROSECS(100) || budget < MICROSECS(100) )
> + printk("Warning: period or budget set to less than 100us.\n"
> + "This may result in high scheduling overhead.\n");
This should use a log level which is rate limited by default. Quite
likely that would be one of the guest log levels.
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1148,10 +1148,19 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op)
> if ( ret )
> return ret;
>
> - if ( (op->sched_id != DOM2OP(d)->sched_id) ||
> - ((op->cmd != XEN_DOMCTL_SCHEDOP_putinfo) &&
> - (op->cmd != XEN_DOMCTL_SCHEDOP_getinfo)) )
> + if ( op->sched_id != DOM2OP(d)->sched_id )
> return -EINVAL;
> + else
> + switch ( op->cmd )
Pointless else, pointlessly increasing the necessary indentation
for the entire switch().
> +typedef struct xen_domctl_schedparam_vcpu {
> + union {
> + xen_domctl_sched_credit_t credit;
> + xen_domctl_sched_credit2_t credit2;
> + xen_domctl_sched_rtds_t rtds;
> + } s;
Please call such unions "u", as done everywhere else.
> + uint16_t vcpuid;
Any particular reason to limit this to 16 bits, when elsewhere
we commonly use 32 bits for vCPU IDs?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-03-07 12:59 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-06 17:55 [PATCH v6 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2016-03-06 17:55 ` [PATCH v6 for Xen 4.7 1/4] xen: enable " Chong Li
2016-03-07 12:59 ` Jan Beulich [this message]
2016-03-07 16:28 ` Chong Li
2016-03-07 16:40 ` Jan Beulich
2016-03-07 17:53 ` Dario Faggioli
2016-03-07 22:16 ` Chong Li
2016-03-08 9:10 ` Jan Beulich
2016-03-08 10:34 ` Dario Faggioli
2016-03-08 11:47 ` Jan Beulich
2016-03-08 19:09 ` Wei Liu
2016-03-09 16:10 ` Dario Faggioli
2016-03-09 16:38 ` Jan Beulich
2016-03-13 17:05 ` Chong Li
2016-03-14 8:37 ` Jan Beulich
2016-03-14 9:10 ` Dario Faggioli
2016-03-14 9:15 ` Jan Beulich
2016-03-14 10:05 ` Dario Faggioli
2016-03-15 16:22 ` Chong Li
2016-03-15 16:41 ` Dario Faggioli
2016-03-15 17:22 ` Chong Li
2016-03-16 3:14 ` Meng Xu
2016-03-16 3:32 ` Chong Li
2016-03-16 3:43 ` Meng Xu
2016-03-16 8:23 ` Dario Faggioli
2016-03-16 14:37 ` Meng Xu
2016-03-16 14:46 ` Chong Li
2016-03-16 14:53 ` Dario Faggioli
2016-03-16 14:46 ` Chong Li
2016-03-16 14:54 ` Dario Faggioli
2016-03-16 10:48 ` Jan Beulich
2016-03-10 22:35 ` Chong Li
2016-03-10 22:50 ` Wei Liu
2016-03-14 9:07 ` Dario Faggioli
2016-03-06 17:55 ` [PATCH v6 for Xen 4.7 2/4] libxc: " Chong Li
2016-03-08 19:09 ` Wei Liu
2016-03-08 19:32 ` Chong Li
2016-03-08 19:36 ` Wei Liu
2016-03-06 17:55 ` [PATCH v6 for Xen 4.7 3/4] libxl: " Chong Li
2016-03-08 19:12 ` Wei Liu
2016-03-09 0:38 ` Chong Li
2016-03-09 14:01 ` Wei Liu
2016-03-09 17:28 ` Dario Faggioli
2016-03-09 21:57 ` Chong Li
2016-03-09 17:09 ` Dario Faggioli
2016-03-09 17:28 ` Dario Faggioli
2016-03-06 17:55 ` [PATCH v6 for Xen 4.7 4/4] xl: " Chong Li
2016-03-08 19:12 ` Wei Liu
2016-03-08 21:24 ` Chong Li
2016-03-09 14:01 ` Wei Liu
2016-03-09 14:09 ` 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=56DD894802000078000D9F84@prv-mh.provo.novell.com \
--to=jbeulich@suse.com \
--cc=chong.li@wustl.edu \
--cc=dario.faggioli@citrix.com \
--cc=dgolomb@seas.upenn.edu \
--cc=george.dunlap@eu.citrix.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).