Re: [Xen-devel] [PATCH for 4.5 v4 1/4] xen: add real time scheduler rtds

[Meng Xu <xumengpanda@xxxxxxxxx>]

2014-09-24 9:14 GMT-04:00 Dario Faggioli <dario.faggioli@xxxxxxxxxx>:
> On mer, 2014-09-24 at 13:09 +0100, Jan Beulich wrote:
>> >>> On 21.09.14 at 00:13, <mengxu@xxxxxxxxxxxxx> wrote:
>
>> *** CID 1240234:  Division or modulo by zero  (DIVIDE_BY_ZERO)
>> /xen/common/sched_rt.c: 327 in rt_update_deadline()
>> 321             do
>> 322                 svc->cur_deadline += svc->period;
>> 323             while ( svc->cur_deadline <= now );
>> 324         }
>> 325         else
>> 326         {
>> >>>     CID 1240234:  Division or modulo by zero  (DIVIDE_BY_ZERO)
>> >>>     In expression "(now - svc->cur_deadline) / svc->period", division by 
>> >>> expression "svc->period" which may be zero has undefined behavior.
>> 327             long count = ((now - svc->cur_deadline) / svc->period) + 1;
>> 328             svc->cur_deadline += count * svc->period;
>> 329         }
>> 330
>> 331         svc->cur_budget = svc->budget;
>>
>> with
>>
>>     ASSERT(svc->period != 0);
>>
>> a few lines up. However, the ASSERT() itself is currently invalid
>> because above code doesn't make sure zero wouldn't get stored
>> into that field. I.e. the ASSERT() currently (indirectly) verifies
>> valid caller input rather than valid hypervisor state.
>>
> Right, good point. Sorry I missed this while reviewing the series.
>
>> This needs
>> to be fixed. And I would have sent a patch right away if it wasn't
>> unclear to me whether op->u.rtds.budget should also be checked
>> against zero (under the assumption that all other values are valid
>> for these two fields).
>>
> I'd go for doing the sanity checking in rt_dom_cntl(), SCHEDOP_putinfo
> branch, of course... Meng, do you agree? If yes, can you send a patch to
> that effect?

Sure! We need do sanity check for this, although the toolstack does
not allow zero value for period and budget of a vcpu. When I do the
sanity check, I will return EINVAL if period or budget is zero.

>
>> Furthermore the function appears to lack serialization with other
>> scheduler operations.
>>
> Not sure what you mean here. There's only one lock serializing
> everything in this scheduler, and it's already taken when this is
> called, the only exception being when it's called from rt_alloc_vdata().
>
> Is it that one you're referring to?
>
> About it, what I'd personally do is look at moving such call from there
> to inside rt_vcpu_insert(), which probably makes even more sense, in
> general, and it resolves the locking issue. Meng? George?

I think it makes sense. I can have a try after knowing this is the
issue Jan referred to. :-)

Thanks!

Meng

>
> 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)
>



-- 


-----------
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