Re: [Xen-devel] [PATCH 1/1] Xen ARINC 653 Scheduler (updated to add support for CPU pools)

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Jun 16, 2010 at 4:04 PM, Kathy Hadley
<Kathy.Hadley@xxxxxxxxxxxxxxx> wrote:
> +/**************************************************************************
> + * Global data                                                            *
> +
> **************************************************************************/
> +static arinc653_sched_private_t arinc653_schedule;
[snip]
> +    /* Allocate memory for ARINC 653 scheduler data structure */
> +    prv = &arinc653_schedule;

You didn't actually allocate memory, you just used the static
structure.  The point of cpupools is to allow multiple instances of a
scheduler to coexist -- if people create two pools, both using the
ARINC scheduler, there will be problems with this.  Is there any
reason not to actually call xmalloc() (as is done in
sched_credit.c:csched_init())?  (Perhaps this is a missed FIXME or a
merge-o?)

Some of the notices in headers seems a little excessive; if
sched_arinc653.c credits Dornerworks, surely people can infer who
added the control structure in xen/include/public/sysctl.h, and added
a link to it in scheduler.c?

Not NACK-worthy, but: In struct arin..._sched_private_s, the element
"arinc653_schedule" should probably be named something a bit more
descriptive.  Similarly, having arinc653 in ..._major_frame seems a
bit redundant, and inconsistent with naming for the other elements.

Looks fine to me otherwise.  (I haven't reviewed the algorithm itself.)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel