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.
>
> 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.
Nate
>
> Thanks,
> Anderson
>
