Re: [PATCH 2/3] arinc: Add cpu-pool support to scheduler.

[Nate Studer <nate.studer@dornerworks.com>]

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