From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chong Li Subject: Re: [PATCH v4 for Xen 4.6 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler Date: Sun, 9 Aug 2015 11:08:45 -0500 Message-ID: References: <1436590356-3706-1-git-send-email-chong.li@wustl.edu> <1436590356-3706-2-git-send-email-chong.li@wustl.edu> <1438012315.5036.210.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1438012315.5036.210.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli Cc: Chong Li , Sisu Xi , George Dunlap , xen-devel , Meng Xu , Jan Beulich , Dagaen Golomb List-Id: xen-devel@lists.xenproject.org On Mon, Jul 27, 2015 at 10:51 AM, Dario Faggioli wrote: > On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote: >> Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls >> to independently get and set the scheduling parameters of each >> vCPU of a domain >> > I'd add a note about the fact that, for now, this is only supported and > being implemented for RTDs. > >> Signed-off-by: Chong Li >> Signed-off-by: Meng Xu >> Signed-off-by: Sisu Xi >> >> --- >> Changes on PATCH v3: >> >> 1) Remove struct xen_domctl_schedparam_t. >> >> 2) Change struct xen_domctl_scheduler_op. >> >> 3) Check if period/budget is within a validated range >> > Don't drop the 'Changes on PATCH v2' part, please. That helps having an > idea of the full history, reminding about previous comments being done, > etc. > >> diff --git a/xen/common/domctl.c b/xen/common/domctl.c >> index 2a2d203..349f68b 100644 >> --- a/xen/common/domctl.c >> +++ b/xen/common/domctl.c > >> @@ -1162,8 +1176,82 @@ rt_dom_cntl( > >> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: >> + spin_lock_irqsave(&prv->lock, flags); >> + for( index = 0; index < op->u.v.nr_vcpus; index++ ) >> + { >> + 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; >> + } >> + svc = rt_vcpu(d->vcpu[local_sched.vcpuid]); >> + period = MICROSECS(local_sched.s.rtds.period); >> + budget = MICROSECS(local_sched.s.rtds.budget); >> + if ( period < MICROSECS(10) || period > RTDS_MAX_PERIOD || >> + budget < MICROSECS(10) || budget > period ) >> + { >> + rc = -EINVAL; >> + break; >> + } >> + if ( period < RTDS_MIN_PERIOD || budget < RTDS_MIN_BUDGET ) >> + warn = 1; >> + >> + svc->period = period; >> + svc->budget = budget; >> + if( hypercall_preempt_check() ) >> + { >> + rc = -ERESTART; >> + break; >> + } >> + } >> + spin_unlock_irqrestore(&prv->lock, flags); >> + break; >> } >> - >> + if ( rc == 0 && warn == 1 ) /* print warning in libxl */ >> + rc = 1; >> return rc; >> } >> > Mmm.. And where is the documentation about the fact that return value of > 1 means that the call succeeded, but some upper layer should print a > warning? > > Remember that, when working in Xen, libxl is not the only toolstack one > should bear in mind. > > In any case, even if documented, I don't like it that much... Why can't > we print a warning here, and just return success? Do I just use printk here? Is there any other more appropriate print function in xen? > >> diff --git a/xen/common/schedule.c b/xen/common/schedule.c >> index ecf1545..886e1b5 100644 >> --- a/xen/common/schedule.c >> +++ b/xen/common/schedule.c >> @@ -1052,10 +1052,22 @@ 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 ) >> + { >> + case XEN_DOMCTL_SCHEDOP_putinfo: >> + break; >> + case XEN_DOMCTL_SCHEDOP_getinfo: >> + break; >> + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: >> + break; >> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: >> + break; >> + default: >> + return -EINVAL; >> + } >> >> /* NB: the pluggable scheduler code needs to take care >> * of locking by itself. */ >> > So, what happens if someone calls XEN_DOMCTL_SCHEDOP_putvcpuinfo or > XEN_DOMCTL_SCHEDOP_getvcpuinfo on Credit or Credit2? The call reports > being successful, but no date is returned (by quickly inspecting > csched_dom_cntl() and csched2_dom_cntl())? > > I think you should go there and add something like: > > if ( op->cmd == XEN_DOMCTL_SCHEDOP_putvcpuinfo || > op->cmd == XEN_DOMCTL_SCHEDOP_getvcpuinfo ) > return -ENOTSUP; > > or something like that. I see. Will do. Chong > Regards, > Dario > > -- > <> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) -- Chong Li Department of Computer Science and Engineering Washington University in St.louis