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.hildebr...@amd.com>
> Suggested-by: Nathan Studer <nathan.stu...@dornerworks.com>
> Signed-off-by: Anderson Choi <anderson.c...@boeing.com>
>
>>> @@ -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
>
>
