Re: [Xen-devel] [PATCH] xen: sanity check input and serialize vcpu data in sched_rt.c

[Meng Xu <xumengpanda@xxxxxxxxx>]

2014-09-29 4:09 GMT-04:00 Jan Beulich <JBeulich@xxxxxxxx>:

>>> On 26.09.14 at 20:29, <mengxu@xxxxxxxxxxxxx> wrote:
> Sanity check input params in rt_dom_cntl();
> Serialize rt_dom_cntl() against the global lock;
> Move the call to rt_update_deadline() from _alloc to _insert.

Regarding this last sentence - the patch clearly does more in this
regard than just move the call from _alloc or _insert. Please make
sure your description matches the patch (which doesn't necessarily
mean enumerating all places it gets moved from - all I'd like to see
is that description and implementation don't disagree).

And aren't the two changes here independent anyway? In which
case submitting them as two patches might be better?

> @@ -577,7 +579,12 @@ rt_vcpu_remove(const struct scheduler *ops, struct vcpu *vc)
>Â Â Â BUG_ON( sdom == NULL );
>
>Â Â Â if ( __vcpu_on_q(svc) )
> +Â Â {
> +Â Â Â Â spinlock_t *lock;
> +Â Â Â Â lock = vcpu_schedule_lock_irq(vc);
>Â Â Â Â Â __q_remove(svc);
> +Â Â Â Â vcpu_schedule_unlock_irq(lock, vc);
> +Â Â }

While Andrew already pointed out that this needs to change anyway,
I'd still like to point out that declarations should be separated from
statements by a blank line.

> @@ -1042,11 +1044,14 @@ rt_dom_cntl(
>Â Â Â struct domain *d,
>Â Â Â struct xen_domctl_scheduler_op *op)
>Â {
> +Â Â struct rt_private *prv = rt_priv(ops);
>Â Â Â struct rt_dom * const sdom = rt_dom(d);
>Â Â Â struct rt_vcpu *svc;
>Â Â Â struct list_head *iter;
> +Â Â unsigned long flags;
>Â Â Â int rc = 0;
>
> +Â Â spin_lock_irqsave(&prv->lock, flags);
>Â Â Â switch ( op->cmd )
>Â Â Â {
>Â Â Â case XEN_DOMCTL_SCHEDOP_getinfo:
> @@ -1055,6 +1060,12 @@ rt_dom_cntl(
>Â Â Â Â Â op->u.rtds.budget = svc->budget / MICROSECS(1);
>Â Â Â Â Â break;
>Â Â Â case XEN_DOMCTL_SCHEDOP_putinfo:
> +Â Â Â Â if ( op->u.rtds.period == 0 || op->u.rtds.budget == 0 )
> +Â Â Â Â {
> +Â Â Â Â Â Â rc = -EINVAL;
> +Â Â Â Â Â Â break;
> +Â Â Â Â }
> +
>Â Â Â Â Â list_for_each( iter, &sdom->vcpu )
>Â Â Â Â Â {
>Â Â Â Â Â Â Â struct rt_vcpu * svc = list_entry(iter, struct rt_vcpu, sdom_elem);
> @@ -1064,6 +1075,8 @@ rt_dom_cntl(
>Â Â Â Â Â break;
>Â Â Â }
>
> +Â Â spin_unlock_irqrestore(&prv->lock, flags);

Do you really need the lock to be held for the "get" side too?

âNot really. The period and budget field of each rt_vcpu is not changed until users use xl command to change it, although cur_deadline and cur_budget, which keep track of the current deadline and remaining budget , is changing in scheduling.

So I will use lock for set operation only.

Mengâ

--

-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

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