From mboxrd@z Thu Jan  1 00:00:00 1970
From: Nate Studer <nate.studer@dornerworks.com>
Subject: Re: [PATCH 2/3] arinc: Add cpu-pool support to
	scheduler.
Date: Tue, 19 Nov 2013 08:58:49 -0500
Message-ID: <528B6E99.8090707@dornerworks.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>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <528B3DBA.9020802@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>, Robert VanVossen <robert.vanvossen@dornerworks.com>, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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.

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