| Message ID | d8c08c22-ee70-4c06-8fcd-ad44fc0dc58f@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arinc653: move next_switch_time access under lock | expand |
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@suse.com> > --- > 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;
On 18.03.2025 16:39, Nathan Studer wrote: > > 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@suse.com> >> --- >> 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? Oops, no, certainly not. It was meant to go ahead of the assignment. I must have been benighted ... Moved locally. Jan
--- 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. */ 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;
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@suse.com> --- The Fixes: tag references where the locking was added; I can't exclude there was an issue here already before that.