Possible bug in cpu_chooseproc?

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Possible bug in cpu_chooseproc?

Matthew Dempsky-3
As the name suggests, remrunqueue(p) removes p from its run queue, and
I believe that makes TAILQ_FOREACH() here unsafe.  Instead of actually
removing all threads from the processor, we'll only remove the first
from each of its run queues.

Diff below replaces TAILQ_FOREACH with the safe/idiomatic pattern for
draining a queue.

ok?

(This hasn't bit me and I don't know any practical consequences of it.
Just spotted it while tracking down a bug in a kern_synch.c diff I'm
working on.)

Index: kern_sched.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sched.c,v
retrieving revision 1.32
diff -u -p -r1.32 kern_sched.c
--- kern_sched.c 4 May 2014 05:03:26 -0000 1.32
+++ kern_sched.c 13 Jul 2014 20:18:38 -0000
@@ -272,7 +272,7 @@ sched_chooseproc(void)
  if (spc->spc_schedflags & SPCF_SHOULDHALT) {
  if (spc->spc_whichqs) {
  for (queue = 0; queue < SCHED_NQS; queue++) {
- TAILQ_FOREACH(p, &spc->spc_qs[queue], p_runq) {
+ while ((p = TAILQ_FIRST(&spc->spc_qs[queue]))) {
  remrunqueue(p);
  p->p_cpu = sched_choosecpu(p);
  setrunqueue(p);

Reply | Threaded
Open this post in threaded view
|

Re: Possible bug in cpu_chooseproc?

Mark Kettenis
> Date: Sun, 13 Jul 2014 13:12:46 -0700
> From: Matthew Dempsky <[hidden email]>
>
> As the name suggests, remrunqueue(p) removes p from its run queue, and
> I believe that makes TAILQ_FOREACH() here unsafe.  Instead of actually
> removing all threads from the processor, we'll only remove the first
> from each of its run queues.
>
> Diff below replaces TAILQ_FOREACH with the safe/idiomatic pattern for
> draining a queue.
>
> ok?

Ugh, yes.

> Index: kern_sched.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sched.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 kern_sched.c
> --- kern_sched.c 4 May 2014 05:03:26 -0000 1.32
> +++ kern_sched.c 13 Jul 2014 20:18:38 -0000
> @@ -272,7 +272,7 @@ sched_chooseproc(void)
>   if (spc->spc_schedflags & SPCF_SHOULDHALT) {
>   if (spc->spc_whichqs) {
>   for (queue = 0; queue < SCHED_NQS; queue++) {
> - TAILQ_FOREACH(p, &spc->spc_qs[queue], p_runq) {
> + while ((p = TAILQ_FIRST(&spc->spc_qs[queue]))) {
>   remrunqueue(p);
>   p->p_cpu = sched_choosecpu(p);
>   setrunqueue(p);
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Possible bug in cpu_chooseproc?

Masao Uebayashi
> > Index: kern_sched.c
> > ===================================================================
> > RCS file: /cvs/src/sys/kern/kern_sched.c,v
> > retrieving revision 1.32
> > diff -u -p -r1.32 kern_sched.c
> > --- kern_sched.c 4 May 2014 05:03:26 -0000 1.32
> > +++ kern_sched.c 13 Jul 2014 20:18:38 -0000
> > @@ -272,7 +272,7 @@ sched_chooseproc(void)
> >   if (spc->spc_schedflags & SPCF_SHOULDHALT) {
> >   if (spc->spc_whichqs) {
> >   for (queue = 0; queue < SCHED_NQS; queue++) {
> > - TAILQ_FOREACH(p, &spc->spc_qs[queue], p_runq) {
> > + while ((p = TAILQ_FIRST(&spc->spc_qs[queue]))) {
> >   remrunqueue(p);
> >   p->p_cpu = sched_choosecpu(p);
> >   setrunqueue(p);
> >
> >
>

I'd add KASSERT(TAILQ_EMPTY()).