From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/3] arinc: Add cpu-pool support to scheduler. Date: Tue, 19 Nov 2013 18:16:35 +0000 Message-ID: <528BAB03.4050706@citrix.com> References: <1384805814-3597-1-git-send-email-nate.studer@dornerworks.com> <1384805814-3597-3-git-send-email-nate.studer@dornerworks.com> <528B3DBA.9020802@citrix.com> <528B6E99.8090707@dornerworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <528B6E99.8090707@dornerworks.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Nate Studer Cc: George Dunlap , Robert VanVossen , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 19/11/2013 13:58, Nate Studer wrote: > On 11/19/2013 5:30 AM, Andrew Cooper wrote: > >>> >>> @@ -380,7 +369,9 @@ a653sched_deinit(const struct scheduler *ops) >>> static void * >>> a653sched_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd) >>> { >>> + a653sched_priv_t *sched_priv = SCHED_PRIV(ops); >>> arinc653_vcpu_t *svc; >>> + int entry; >> sched_priv->num_schedule_entries is inconsistently used as signed and >> unsigned. It should be an unsigned value, and updated to be so >> everywhere, including in the a653sched_priv_t structure. >> > Right, this inconsistency should be fixed. > >>> >>> /* >>> * Allocate memory for the ARINC 653-specific scheduler data information >>> @@ -390,6 +381,19 @@ a653sched_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd) >>> if ( svc == NULL ) >>> return NULL; >>> >>> + /* add every one of dom0's vcpus to the schedule */ >>> + if (vc->domain->domain_id == 0) >> Xen style would include spaces immediately inside the brackets. >> >> Also, it looks like you could do with a bounds check against >> ARINC653_MAX_DOMAINS_PER_SCHEDULE before trying to put another dom0 into >> the mix. > Will add bounds checking. > >>> /** >>> @@ -538,8 +542,13 @@ a653sched_do_schedule( >>> static int sched_index = 0; >>> static s_time_t next_switch_time; >>> a653sched_priv_t *sched_priv = SCHED_PRIV(ops); >>> + const int cpu = smp_processor_id(); >> This should be an unsigned int. > Yes it should. This needs to be fixed in pick_cpu as well. > >>> >>> - if ( now >= sched_priv->next_major_frame ) >>> + if ( sched_priv->num_schedule_entries < 1 ) >>> + { >>> + sched_priv->next_major_frame = now + DEFAULT_TIMESLICE; >>> + } >> Xen style would require these braces to be omitted. > Even when followed by a multiple statement "else if"? I see braces in the same > construct in the credit scheduler. > > if ( list_empty(&svc->active_vcpu_elem) ) > { > __csched_vcpu_acct_start(prv, svc); > } > else if ( _csched_cpu_pick(ops, current, 0) != cpu ) > { > > I have no problem changing it, since I want to avoid spreading styling > inconsistencies, but I just want to make sure. Yes, even with multiple "else if" statements. In this case, the credit scheduler would be wrong. Style fixes like this are typically introduced on a 'when working in the area' basis. This avoids style fixes for the sake of style fixes, as much as it prevents propagating bad style. ~Andrew > >>> + else if ( now >= sched_priv->next_major_frame ) >>> { >>> /* time to enter a new major frame >>> * the first time this function is called, this will be true */ > The remaining comments are style comments, which I will fix up in the next > version of the patch. > > Nate >