Re: [Xen-devel] [PATCH for Xen 4.5 v2 1/2] xen: sanity check input and serialization in sched_rt.c

[Meng Xu <xumengpanda@xxxxxxxxx>]

2014-10-29 5:41 GMT-04:00 Dario Faggioli <dario.faggioli@xxxxxxxxxx>:
> [Ditto, about the Cc list]
>
> On Sat, 2014-10-25 at 10:16 -0400, Meng Xu wrote:
>> Sanity check input params in rt_dom_cntl();
>> Serialize rt_dom_cntl() against the global lock.
>>
>> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
>>
> Reviewed-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
>
> Just a note. See below.
>
>> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
>> index 6c8faeb..b87c95b 100644
>> --- a/xen/common/sched_rt.c
>> +++ b/xen/common/sched_rt.c
>> @@ -1042,9 +1042,11 @@ 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;
>>
>>      switch ( op->cmd )
>> @@ -1055,12 +1057,19 @@ 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;
>> +        }
>> +        spin_lock_irqsave(&prv->lock, flags);
>>          list_for_each( iter, &sdom->vcpu )
>>          {
>>              struct rt_vcpu * svc = list_entry(iter, struct rt_vcpu, 
>> sdom_elem);
>>              svc->period = MICROSECS(op->u.rtds.period); /* transfer to 
>> nanosec */
>>              svc->budget = MICROSECS(op->u.rtds.budget);
>>          }
>> +        spin_unlock_irqrestore(&prv->lock, flags);
>>
> Thinking more about this, I actually prefer the approach where both the
> get and set side are properly serialized. Races are very unlikely, and
> probably not that harmful, but I don't see why leaving room for them,
> and it looks more consistent to me that way.
>
> I appreciate why you've done it this way in this patch, and I am fine
> with it. As said, it's not a huge deal, and this code is experimental,
> so we can always change it later.
>
> I just wanted to let you know that, if you end up resending the patches
> for some reasons, I'd personally go for the fully locked approach.
>

Sure! I can do that if no other objection to that approach of
serializing both the get and set method. :-)


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