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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2487 bytes --]

On mer, 2014-09-24 at 13:09 +0100, Jan Beulich wrote:
> >>> On 21.09.14 at 00:13, <mengxu@cis.upenn.edu> 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?

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

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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