From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH 1/1] Xen ARINC 653 Scheduler (updated to add support for CPU pools) Date: Wed, 16 Jun 2010 17:13:53 +0100 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Kathy Hadley , George Dunlap Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 16/06/2010 17:00, "Kathy Hadley" wrote: > George, > I actually tried the xmalloc() method first. I found that when the > .adjust_global function was called, the address of the "ops" data structu= re > passed to that function was different from the address of the "ops" data > structure when the .init function was called. I wanted to use .adjust_gl= obal > to modify the data structure that was created when the .init function was > called, but I could not figure out a way to get the address of the second= data > structure. Suggestions? You should modify the structure you are passed -- that is ops and your private structure as pointed at via ops->sched_data. The latter should always point at a private structure you previously initialised via your .init hook. -- Keir > I can make the modifications you suggest for the other items. Thanks f= or > the comments. >=20 > Regards, > Kathy Hadley > DornerWorks, Ltd. >=20 >> -----Original Message----- >> From: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf Of George >> Dunlap >> Sent: Wednesday, June 16, 2010 11:50 AM >> To: Kathy Hadley >> Cc: xen-devel@lists.xensource.com; Keir Fraser >> Subject: Re: [Xen-devel] [PATCH 1/1] Xen ARINC 653 Scheduler (updated >> to add support for CPU pools) >>=20 >> On Wed, Jun 16, 2010 at 4:04 PM, Kathy Hadley >> wrote: >>>=20 >> +/********************************************************************* >> ***** >>> + * 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 * >>> + >>>=20 >> *********************************************************************** >> ***/ >>> +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; >>=20 >> 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?) >>=20 >> 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? >>=20 >> 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. >>=20 >> Looks fine to me otherwise. (I haven't reviewed the algorithm itself.) >>=20 >> -George