sched_choosecpu_fork()

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

sched_choosecpu_fork()

Martin Pieuchot
When choosing the initial CPU for a new thread to run on, the scheduler
do not consider that CPU0 is handling interrupts.

Threads that are created early, like the network taskq, always end up
scheduled on CPU0.  If the machine isn't running many programs, like
simple firewalls, there won't be contention and the thread will always
stay on its original CPU.

This behavior derives to how yield() and preempt() are implemented.  These
functions do not look for another CPU to run on when a busy thread tries to
be cooperative.  This is what we want.  However in the case of the  network
taskq we'd prefer to avoid CPU0.

I don't know if enforcing this behavior makes sense or if we should
accept that in some moon phases our forwarding performances are only 50%
of what they could be.

However using the same logic to pick a CPU during fork than during a
sleep cycle seems to be good enough to avoid the problem described above.
If there's contention the taskq will move between CPUs, if there isn't it
will stay, generally, on CPU1.

Since FORK_PPWAIT isn't implemented since 10 years, I'd say it's also a
simplification :o)

What would you think of killing sched_choosecpu_fork()?

Any other suggestion?

ok?

Index: kern/kern_fork.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_fork.c,v
retrieving revision 1.209
diff -u -p -r1.209 kern_fork.c
--- kern/kern_fork.c 6 Jan 2019 12:59:45 -0000 1.209
+++ kern/kern_fork.c 15 Feb 2019 13:49:32 -0000
@@ -329,7 +329,7 @@ fork_thread_start(struct proc *p, struct
 
  SCHED_LOCK(s);
  p->p_stat = SRUN;
- p->p_cpu = sched_choosecpu_fork(parent, flags);
+ p->p_cpu = sched_choosecpu(parent);
  setrunqueue(p);
  SCHED_UNLOCK(s);
 }
Index: kern/kern_sched.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sched.c,v
retrieving revision 1.54
diff -u -p -r1.54 kern_sched.c
--- kern/kern_sched.c 17 Nov 2018 23:10:08 -0000 1.54
+++ kern/kern_sched.c 15 Feb 2019 13:50:06 -0000
@@ -340,62 +340,6 @@ again:
 }
 
 struct cpu_info *
-sched_choosecpu_fork(struct proc *parent, int flags)
-{
-#ifdef MULTIPROCESSOR
- struct cpu_info *choice = NULL;
- fixpt_t load, best_load = ~0;
- int run, best_run = INT_MAX;
- struct cpu_info *ci;
- struct cpuset set;
-
-#if 0
- /*
- * XXX
- * Don't do this until we have a painless way to move the cpu in exec.
- * Preferably when nuking the old pmap and getting a new one on a
- * new cpu.
- */
- /*
- * PPWAIT forks are simple. We know that the parent will not
- * run until we exec and choose another cpu, so we just steal its
- * cpu.
- */
- if (flags & FORK_PPWAIT)
- return (parent->p_cpu);
-#endif
-
- /*
- * Look at all cpus that are currently idle and have nothing queued.
- * If there are none, pick the one with least queued procs first,
- * then the one with lowest load average.
- */
- cpuset_complement(&set, &sched_queued_cpus, &sched_idle_cpus);
- cpuset_intersection(&set, &set, &sched_all_cpus);
- if (cpuset_first(&set) == NULL)
- cpuset_copy(&set, &sched_all_cpus);
-
- while ((ci = cpuset_first(&set)) != NULL) {
- cpuset_del(&set, ci);
-
- load = ci->ci_schedstate.spc_ldavg;
- run = ci->ci_schedstate.spc_nrun;
-
- if (choice == NULL || run < best_run ||
-    (run == best_run &&load < best_load)) {
- choice = ci;
- best_load = load;
- best_run = run;
- }
- }
-
- return (choice);
-#else
- return (curcpu());
-#endif
-}
-
-struct cpu_info *
 sched_choosecpu(struct proc *p)
 {
 #ifdef MULTIPROCESSOR
Index: sys/sched.h
===================================================================
RCS file: /cvs/src/sys/sys/sched.h,v
retrieving revision 1.50
diff -u -p -r1.50 sched.h
--- sys/sched.h 17 Nov 2018 23:10:08 -0000 1.50
+++ sys/sched.h 15 Feb 2019 13:50:14 -0000
@@ -150,7 +150,6 @@ void mi_switch(void);
 void cpu_switchto(struct proc *, struct proc *);
 struct proc *sched_chooseproc(void);
 struct cpu_info *sched_choosecpu(struct proc *);
-struct cpu_info *sched_choosecpu_fork(struct proc *parent, int);
 void cpu_idle_enter(void);
 void cpu_idle_cycle(void);
 void cpu_idle_leave(void);

Reply | Threaded
Open this post in threaded view
|

Re: sched_choosecpu_fork()

Amit Kulkarni
On Wed, 20 Feb 2019 14:08:45 -0300
Martin Pieuchot <[hidden email]> wrote:

> When choosing the initial CPU for a new thread to run on, the scheduler
> do not consider that CPU0 is handling interrupts.
>
> Threads that are created early, like the network taskq, always end up
> scheduled on CPU0.  If the machine isn't running many programs, like
> simple firewalls, there won't be contention and the thread will always
> stay on its original CPU.
>
> This behavior derives to how yield() and preempt() are implemented.  These
> functions do not look for another CPU to run on when a busy thread tries to
> be cooperative.  This is what we want.  However in the case of the  network
> taskq we'd prefer to avoid CPU0.
>
> I don't know if enforcing this behavior makes sense or if we should
> accept that in some moon phases our forwarding performances are only 50%
> of what they could be.
>
> However using the same logic to pick a CPU during fork than during a
> sleep cycle seems to be good enough to avoid the problem described above.
> If there's contention the taskq will move between CPUs, if there isn't it
> will stay, generally, on CPU1.
>
> Since FORK_PPWAIT isn't implemented since 10 years, I'd say it's also a
> simplification :o)
>
> What would you think of killing sched_choosecpu_fork()?
>
> Any other suggestion?
>
> ok?
>
> Index: kern/kern_fork.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_fork.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 kern_fork.c
> --- kern/kern_fork.c 6 Jan 2019 12:59:45 -0000 1.209
> +++ kern/kern_fork.c 15 Feb 2019 13:49:32 -0000
> @@ -329,7 +329,7 @@ fork_thread_start(struct proc *p, struct
>  
>   SCHED_LOCK(s);
>   p->p_stat = SRUN;
> - p->p_cpu = sched_choosecpu_fork(parent, flags);
> + p->p_cpu = sched_choosecpu(parent);
>   setrunqueue(p);
>   SCHED_UNLOCK(s);
>  }
> Index: kern/kern_sched.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sched.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 kern_sched.c
> --- kern/kern_sched.c 17 Nov 2018 23:10:08 -0000 1.54
> +++ kern/kern_sched.c 15 Feb 2019 13:50:06 -0000
> @@ -340,62 +340,6 @@ again:
>  }
>  
>  struct cpu_info *
> -sched_choosecpu_fork(struct proc *parent, int flags)
> -{
> -#ifdef MULTIPROCESSOR
> - struct cpu_info *choice = NULL;
> - fixpt_t load, best_load = ~0;
> - int run, best_run = INT_MAX;
> - struct cpu_info *ci;
> - struct cpuset set;
> -
> -#if 0
> - /*
> - * XXX
> - * Don't do this until we have a painless way to move the cpu in exec.
> - * Preferably when nuking the old pmap and getting a new one on a
> - * new cpu.
> - */
> - /*
> - * PPWAIT forks are simple. We know that the parent will not
> - * run until we exec and choose another cpu, so we just steal its
> - * cpu.
> - */
> - if (flags & FORK_PPWAIT)
> - return (parent->p_cpu);
> -#endif
> -
> - /*
> - * Look at all cpus that are currently idle and have nothing queued.
> - * If there are none, pick the one with least queued procs first,
> - * then the one with lowest load average.
> - */
> - cpuset_complement(&set, &sched_queued_cpus, &sched_idle_cpus);
> - cpuset_intersection(&set, &set, &sched_all_cpus);
> - if (cpuset_first(&set) == NULL)
> - cpuset_copy(&set, &sched_all_cpus);
> -
> - while ((ci = cpuset_first(&set)) != NULL) {
> - cpuset_del(&set, ci);
> -
> - load = ci->ci_schedstate.spc_ldavg;
> - run = ci->ci_schedstate.spc_nrun;
> -
> - if (choice == NULL || run < best_run ||
> -    (run == best_run &&load < best_load)) {
> - choice = ci;
> - best_load = load;
> - best_run = run;
> - }
> - }
> -
> - return (choice);
> -#else
> - return (curcpu());
> -#endif
> -}
> -
> -struct cpu_info *
>  sched_choosecpu(struct proc *p)
>  {
>  #ifdef MULTIPROCESSOR
> Index: sys/sched.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/sched.h,v
> retrieving revision 1.50
> diff -u -p -r1.50 sched.h
> --- sys/sched.h 17 Nov 2018 23:10:08 -0000 1.50
> +++ sys/sched.h 15 Feb 2019 13:50:14 -0000
> @@ -150,7 +150,6 @@ void mi_switch(void);
>  void cpu_switchto(struct proc *, struct proc *);
>  struct proc *sched_chooseproc(void);
>  struct cpu_info *sched_choosecpu(struct proc *);
> -struct cpu_info *sched_choosecpu_fork(struct proc *parent, int);
>  void cpu_idle_enter(void);
>  void cpu_idle_cycle(void);
>  void cpu_idle_leave(void);
>

In sched_choosecpu_fork(), we see best_run is INT_MAX, the comparison below actually reduces to a assignment ==> choice = ci;
-               if (choice == NULL || run < best_run ||
-                   (run == best_run &&load < best_load)) {
-                       choice = ci;
-                       best_load = load;
-                       best_run = run;
-               }

because run will always be less than INT_MAX + run can rarely be INT_MAX, and choice is always NULL!

In sched_choosecpu(), last_cost is not being used where it could be useful. Setting to INT_MAX, the comparison will always be false. This is like you can safely delete sched_proc_to_cpu_cost() also. choice is again null.

                int cost = sched_proc_to_cpu_cost(ci, p);

                if (choice == NULL || cost < last_cost) {
                        choice = ci;
                        last_cost = cost;
                }
                cpuset_del(&set, ci);


Finally, the other call to sched_proc_to_cpu_cost() in kern/kern_fork.c can be wrapped in ifdef MULTIPROCESSOR instead of calling the function if it's a single CPU, save a function call.

static inline void
fork_thread_start(struct proc *p, struct proc *parent, int flags)
{
        int s;

        SCHED_LOCK(s);
        p->p_stat = SRUN;
#ifdef MULTIPROCESSOR
        p->p_cpu = sched_choosecpu(parent);
#else
        p->p_cpu = 0;
#endif
        setrunqueue(p);
        SCHED_UNLOCK(s);
}

Reply | Threaded
Open this post in threaded view
|

Re: sched_choosecpu_fork()

Amit Kulkarni
> In sched_choosecpu_fork(), we see best_run is INT_MAX, the comparison below actually reduces to a assignment ==> choice = ci;
> -               if (choice == NULL || run < best_run ||
> -                   (run == best_run &&load < best_load)) {
> -                       choice = ci;
> -                       best_load = load;
> -                       best_run = run;
> -               }
>
> because run will always be less than INT_MAX + run can rarely be INT_MAX, and choice is always NULL!
>
> In sched_choosecpu(), last_cost is not being used where it could be useful. Setting to INT_MAX, the comparison will always be false. This is like you can safely delete sched_proc_to_cpu_cost() also. choice is again null.
>
> int cost = sched_proc_to_cpu_cost(ci, p);
>
> if (choice == NULL || cost < last_cost) {
> choice = ci;
> last_cost = cost;
> }
> cpuset_del(&set, ci);
>

For the above I just realized this is only true for the first time in the loop, so please ignore this above part!