Re: [XEN PATCH v2] xen/arinc653: fix delay in the start of major frame

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 7/17/25 22:16, Nathan Studer wrote:
> On 7/17/25 21:39, Choi, Anderson wrote:
>>> On 7/17/25 9:21, Hildebrand, Stewart wrote:
>> else +        { +            sched_priv->next_switch_time =
>>>>> sched_priv->next_major_frame + +
>>>>> sched_priv->schedule[0].runtime; +
>>>>> sched_priv->next_major_frame += sched_priv->major_frame; +        } +
>>>>>   }
>>>>
>>>> There's no need for the above loop, this can be fixed by subtracting
>>>> the remainder (modulus major_frame). E.g.:
>>>>
>>>>     if ( now >= sched_priv->next_major_frame )
>>>>     {
>>>>         s_time_t major_frame = sched_priv->num_schedule_entries < 1
>>>>                                ? DEFAULT_TIMESLICE
>>>>                                : sched_priv->major_frame;
>>>>         s_time_t remainder = (now - sched_priv->next_major_frame) %
>>>> major_frame;
>>>>
>>>>         sched_priv->sched_index = 0;
>>>>         sched_priv->next_major_frame = now - remainder + major_frame;
>>>>         sched_priv->next_switch_time = now - remainder +
>>>>                                        (sched_priv->num_schedule_entries < 
>>>> 1
>>>>                                         ? DEFAULT_TIMESLICE
>>>>                                         : sched_priv->schedule[0].runtime);
>>>>     }
>>>
>>
>> Stewart,
>>
>> I appreciate your suggestion to eliminate the while loop.
>> What about initializing major_frame and schedule[0].runtime to
>> DEFAULT_TIMESLICE at a653sched_init() and use them until the real parameters
>> are set as below to eliminate the if branch?
> 
> It would make the scheduling code cleaner, but since you asked Stew I'll 
> defer to him.

It feels odd to index schedule[0] when num_schedule_entries may be
zero... But since it's a fixed size array and element 0 is guaranteed to
exist I don't have a strong preference.

> 
>>
>> diff --git a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c
>> index 930361fa5c..73ce5cdfaf 100644
>> --- a/xen/common/sched/arinc653.c
>> +++ b/xen/common/sched/arinc653.c
>> @@ -361,6 +361,8 @@ a653sched_init(struct scheduler *ops)
>>      ops->sched_data = prv;
>>
>>      prv->next_major_frame = 0;
>> +    prv->major_frame = DEFAULT_TIMESLICE;
>> +    prv->schedule[0].runtime = DEFAULT_TIMESLICE;
>>      spin_lock_init(&prv->lock);
>>      INIT_LIST_HEAD(&prv->unit_list);
>>
>> static void cf_check
>> a653sched_do_schedule(
>> <snip>
>>     /* Switch to next major frame directly eliminating the use of loop */
>>     if ( now >= sched_priv->next_major_frame )
>>     {
>>         s_time_t major_frame = sched_priv->major_frame;
>>         s_time_t remainder = (now - sched_priv->next_major_frame) %
>> major_frame;
>>
>>         sched_priv->sched_index = 0;
>>         sched_priv->next_major_frame = now - remainder + major_frame;
>>         sched_priv->next_switch_time = now - remainder + sched_priv-
>>> schedule[0].runtime;
>>     }
>>
>>> The direct method suggested by Stew is preferable in the unusual case
>>> where many major frames are missed.  (We have only seen that happen
>>> when using a debugger.)
>>>
>>> To help uncover any issues like the one this patch addresses in the
>>> future we may also want to follow up this commit with a change to make
>>> scheduler misses more obvious.  Something like the following:
>>>
>>> commit e95cbc9078127c412bd1605d93cb97837751b5b4 (HEAD -> master)
>>> Author: Nathan Studer <nathan.studer@xxxxxxxxxxxxxxx>
>>> Date:   Thu Jul 17 12:43:39 2025 -0400
>>>
>>>     Do not silently skip frame overruns diff --git
>>> a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c index
>>> 2d0d3bcbb3..a2c1c66c27 100644
>>> --- a/xen/common/sched/arinc653.c
>>> +++ b/xen/common/sched/arinc653.c
>>> @@ -523,9 +523,17 @@ a653sched_do_schedule(
>>>      a653sched_priv_t *sched_priv = SCHED_PRIV(ops);
>>>      const unsigned int cpu = sched_get_resource_cpu(smp_processor_id());
>>>      unsigned long flags;
>>> +    unsigned int oindex;
>>> +    unsigned int missed;
>>>
>>>      spin_lock_irqsave(&sched_priv->lock, flags);
>>> +    if ( now > (sched_priv->next_major_frame +  sched_priv->major_frame))
>>> +    {
>>> +        missed = (now - sched_priv->next_major_frame) / sched_priv-
>>>> major_frame;
>>> +        printk(XENLOG_ERR, "Missed %d major frame(s)!\n", missed);
>>> +    }
>>> +
>>>      /* Switch to next major frame while handling potentially missed
>>> frames */ @@ -544,6 +552,7 @@ a653sched_do_schedule(
>>>          }
>>>      }
>>> +    oindex = sched_priv->sched_index;
>>>      /* Switch minor frame or find correct minor frame after a miss */
>>>      while ( (now >= sched_priv->next_switch_time) &&
>>>              (sched_priv->sched_index <
>>>              sched_priv->num_schedule_entries) ) @@ -553,6 +562,12 @@
>>>              a653sched_do_schedule(
>>>              sched_priv->schedule[sched_priv->sched_index].runtime;
>>>      }
>>> +    if ( (oindex - sched_priv->sched_index) > 1)
>>> +    {
>>> +        missed = (oindex - sched_priv->sched_index - 1);
>>> +        printk(XENLOG_WARNING, "Missed %d minor frame(s)!\n", missed);
>>> +    }
>>> +
>>>      /*
>>
>> Nathan,
>>
>> Do we need a comma (,) between XENLOG_WARNING and the quoted string
>> when calling printk()?
> 
> No.
> 
>> And wouldn't the log be printed every time a new minor frame starts, for 
>> example
>> oindex = 0 and sched_priv->sched_index = 1?
>> I think you meant to use the condition "if (sched_priv->sched_index - 
>> oindex) > 1"
>> to check minor frame misses?
> 
> Right, the order should be the other way around in both the condition and the 
> print.
> 
> Regardless, we can just worry about your patch for now and improve the miss 
> detection reporting later.

+1

> 
>      Nate
> 
>>
>> Thanks,
>> Anderson
>>
>