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.stu...@dornerworks.com>
>>> 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
>>
>
