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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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
>