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

["Choi, Anderson" <Anderson.Choi@xxxxxxxxxx>]

> EXT email: be mindful of links/attachments.
> 
> 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?

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()?
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?

Thanks,
Anderson