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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 7/22/25 20:16, Choi, Anderson wrote:
> Stewart,
> 
>> EXT email: be mindful of links/attachments.
>>
>> Hi,
>>
>> It largely looks OK to me, just a few small comments below.
>>
>> On 7/18/25 05:16, Anderson Choi wrote:
>>> ARINC653 specificaion requires partition scheduling to be
>>> deterministic
>>
>> Typo: s/specificaion/specification/
>>
> Addressed the typo.
> 
> ARINC653 specification requires partition scheduling to be deterministic
> 
>>> Per discussion with Nathan Studer, there are odd cases the first minor
>>> frame can be also missed. In that sceanario, iterate through the
>>> schedule after resyncing
>>
>> Typo: s/sceanario/scenario/
>>
>> The line is also too long.
>>
> Addressed the typo and the lengthy sentence.
> 
> Per discussion with Nathan Studer, there are odd cases the first minor
> frame can be also missed. In that scenario, iterate through the schedule
> after resyncing the expected next major frame.
> 
>>> Per suggestion from Stewart Hildebrand, the while loop to calculate
>>> the start of next major frame can be eliminated by using a modulo
>> operation.
>>
>> The while loop was only in earlier revisions of the patch, not in the 
>> upstream
>> codebase, so it doesn't make sense to mention it in the commit message.
>>
> Removed the reference to the while loop.
> 
> Per suggestion from Stewart Hildebrand, use a modulo operation to
> calculate the start of next major frame.
> 
>>>
>>> Fixes: 22787f2e107c ("ARINC 653 scheduler")
>>>
>>
>> Please remove the extraneous newline between the Fixes: tag and
>> remaining tags
>>
> Removed the newline.
> 
> Fixes: 22787f2e107c ("ARINC 653 scheduler")
> Suggested-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> Suggested-by: Nathan Studer <nathan.studer@xxxxxxxxxxxxxxx>
> Signed-off-by: Anderson Choi <anderson.choi@xxxxxxxxxx>
> 
>>> @@ -526,29 +528,30 @@ a653sched_do_schedule(
>>>
>>>      spin_lock_irqsave(&sched_priv->lock, flags);
>>
>> Since the num_schedule_entries < 1 case is no longer handled below, we
>> depend on major_frame > 0. Please add ASSERT(sched_priv->major_frame >
>> 0); here.
>>
>>> -    if ( sched_priv->num_schedule_entries < 1 )
>>> -        sched_priv->next_major_frame = now + DEFAULT_TIMESLICE;
>>> -    else if ( now >= sched_priv->next_major_frame )
>>> +    /* Switch to next major frame directly eliminating the use of
>>> + loop */
>>
>> Similarly to the commit message, there was no loop in the code before, it
>> doesn't make sense to mention it in the in-code comment.
>>
> Added ASSERT() and removed the mention to the loop.
> 
>      spin_lock_irqsave(&sched_priv->lock, flags);
> 
> -    /* Switch to next major frame directly eliminating the use of loop */
> +    ASSERT(sched_priv->major_frame > 0);
> +
> +    /* Switch to next major frame using a modulo operation */
> 
>>> +    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 */
>>> -        /* start with the first domain in the schedule */
>>> +        s_time_t major_frame = sched_priv->major_frame;
>>> +        s_time_t remainder = (now - sched_priv->next_major_frame) %
>>> + major_frame;
>>> +
>>> +        /* major_frame and schedule[0].runtime contain DEFAULT_TIMESLICE
>>> +         * if num_schedule_entries is zero.
>>> +         */
>>
>> The start of the multi-line comment should be on its own line. I know the
>> comment format was a preexisting issue, but since you are changing the lines
>> anyway, please adhere to CODING_STYLE on the changed lines.
>>
> Addressed as below.
> 
> -        /* major_frame and schedule[0].runtime contain DEFAULT_TIMESLICE
> +        /*
> +         * major_frame and schedule[0].runtime contain DEFAULT_TIMESLICE
>           * if num_schedule_entries is zero.
>           */
>          sched_priv->sched_index = 0;
> 
>>> +        sched_priv->sched_index++;
>>> +        sched_priv->next_switch_time +=
>>> +            sched_priv->schedule[sched_priv->sched_index].runtime;
>>>      }
>>> -
>>> +
>>
>> Please remove the extraneous white space
>>
> Removed the white space.
> 
> I do appreciate your valuable review.
> Would it be okay to submit v4 with all the changes applied?

Yes, please do

> Please let me know if there's anything that doesn't reflect your comments 
> correctly.

Your inline replies look good, thanks

> 
> Thanks,
> Anderson
> 
>