RE: [PATCH] arinc653: move next_switch_time access under lock

[Nathan Studer <Nathan.Studer@xxxxxxxxxxxxxxx>]

On 17/03/25 05:31, Jan Beulich wrote:
> Even before its recent movement to the scheduler's private data structure it 
> looks
> to have been wrong to update the field under lock, but then read it with the 
> lock
> no longer held.
> 
> Coverity-ID: 1644500
> Fixes: 9f0c658baedc ("arinc: add cpu-pool support to scheduler")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> The Fixes: tag references where the locking was added; I can't exclude there 
> was
> an issue here already before that.
> 
> --- a/xen/common/sched/arinc653.c
> +++ b/xen/common/sched/arinc653.c
> @@ -579,6 +579,9 @@ a653sched_do_schedule(
>       */
>      BUG_ON(now >= sched_priv->next_major_frame);
> 
> +    prev->next_time = sched_priv->next_switch_time - now;
> +
> +    /* Return the amount of time the next domain has to run. */

This could be pushed up to immediately after next_switch_time is set, but here 
is
good enough.  However, did you mean to put the comment after the assignment
separated by whitespace?

        Nate

>      spin_unlock_irqrestore(&sched_priv->lock, flags);
> 
>      /* Tasklet work (which runs in idle UNIT context) overrides all else. */ 
> @@ -
> 590,11 +593,7 @@ a653sched_do_schedule(
>           && (sched_unit_master(new_task) != cpu) )
>          new_task = IDLETASK(cpu);
> 
> -    /*
> -     * Return the amount of time the next domain has to run and the address
> -     * of the selected task's UNIT structure.
> -     */
> -    prev->next_time = sched_priv->next_switch_time - now;
> +    /* Also return the address of the selected task's UNIT structure.
> + */
>      prev->next_task = new_task;
>      new_task->migrated = false;