From mboxrd@z Thu Jan  1 00:00:00 1970
From: George Dunlap <George.Dunlap@eu.citrix.com>
Subject: Re: [PATCH 1/1] Xen ARINC 653 Scheduler (updated to add
	support for CPU pools)
Date: Wed, 16 Jun 2010 16:50:14 +0100
Message-ID: <AANLkTimYBp1SiABd6SGNqi9GGfQWRe85oPqcLeJh2INc@mail.gmail.com>
References: <D3E384327F5C6D48AADCEA84160B7D7301470EC7@mcbain.dw.local>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <D3E384327F5C6D48AADCEA84160B7D7301470EC7@mcbain.dw.local>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Kathy Hadley <Kathy.Hadley@dornerworks.com>
Cc: xen-devel@lists.xensource.com, Keir Fraser <keir.fraser@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

On Wed, Jun 16, 2010 at 4:04 PM, Kathy Hadley
<Kathy.Hadley@dornerworks.com> wrote:
> +/***********************************************************************=
***
> + * Global data=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 *
> +
> *************************************************************************=
*/
> +static arinc653_sched_private_t arinc653_schedule;
[snip]
> +=A0=A0=A0 /* Allocate memory for ARINC 653 scheduler data structure */
> +=A0=A0=A0 prv =3D &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