Re: [PATCH] x86: Reduce clock calibration time during slave cpu startup

Jack Steiner
2011-08-29T15:00:37Z

On Fri, Aug 26, 2011 at 04:56:34PM -0700, Andrew Morton wrote:
> On Wed, 27 Jul 2011 08:57:31 -0500
> Jack Steiner <steiner@sgi.com> wrote:
>
> > Reduce the startup time for slave cpus.
> >
> > This patch adds hooks for an arch-specific function for clock calibration.
> > These hooks are used on x86. They assume all cores in a physical socket
> > run at the same core speed. If a newly started cpu has the same phys_proc_id
> > as a core already active, use the already-calculated value of loops_per_jiffy.
> >
> > This patch reduces the time required to start slave cpus on a 4096 cpu
> > system from:
> > 465 sec OLD
> > 62 sec NEW
>
> Eight minutes is just stupid.

Agree. I'd like to reduce that. It currently takes about 65 minutes to
boot a 4096p system with a reasonable sized IO config (a big part
of the boot time is IO dependent). Reducing by 8 min is a good improvement
but we still have more to do. Calibration is one of larger contributors
to boot times.

>
> 100ms/cpu is just stupid too. What's the CPU doing? Spinning around
> counting ticks? That's parallelizable.

The time is spent in the clock calibration code. It unfortunately takes a while
to calibrate to a high degree of accuracy.

Ingo was concerned that trying to calibrate in parallel would introduce error.

Running calibration in parallel is pretty stupid: cores/threads might
impact each other and there might be a lot of avoidable noise in the
results.

Thanks, Ingo

>
> > This reduces boot time on a 4096p system by almost 7 minutes. Nice...
> >
> >
> > Signed-off-by: Jack Steiner <steiner@sgi.com>
> >
> >
> > ---
> > Note: patch assumes that all multi-core x86 processor sockets have the same
> > clock frequency for all cores. AFAIK, this is true & will continue
> > to be true for a long time. Have I overlooked anything???
>
> Well, Andi thinks this may become untrue relatively soon. Then what do
> we do?

I posted a V3 version of the patch that eliminates this assumption. The new version
skip recalibration of cores within a socket only if the delay loop uses the TSC
and for CONSTANT_TSC for the cores within the socket.

So far, I have not received any feedback. The patch is at:

http://marc.info/?l=linux-kernel&m=131309367414891&w=2

I'll resend again.

>
> > /*
> > + * Check if another cpu is in the same socket and has already been calibrated.
> > + * If found, use the previous value. This assumes all cores in the same physical
> > + * socket have the same core frequency.
> > + */
> > +unsigned long __cpuinit calibrate_delay_is_known(void)
> > +{
> > + int i, cpu = smp_processor_id();
> > +
> > + for_each_online_cpu(i)
> > + if (cpu_data(i).phys_proc_id == cpu_data(cpu).phys_proc_id)
>
> This will always match when `i' reaches `cpu'. Or is this cpu not
> online at this time?

Correct - not online.

>
> > + return cpu_data(i).loops_per_jiffy;
> > + return 0;
> > +}
> > +
> > +/*
> > * Activate a secondary processor.
> > */
> > notrace static void __cpuinit start_secondary(void *unused)
> > Index: linux/init/calibrate.c
> > ===================================================================
> > --- linux.orig/init/calibrate.c 2011-07-26 08:01:15.571979739 -0500
> > +++ linux/init/calibrate.c 2011-07-27 08:39:35.691983745 -0500
> > @@ -243,6 +243,20 @@ recalibrate:
> > return lpj;
> > }
> >
> > +/*
> > + * Check if cpu calibration delay is already known. For example,
> > + * some processors with multi-core sockets may have all sockets
> > + * use the same core frequency. It is not necessary to calibrate
> > + * each core.
> > + *
> > + * Architectures should override this function if a faster calibration
> > + * method is available.
> > + */
> > +unsigned long __attribute__((weak)) __cpuinit calibrate_delay_is_known(void)
>
> __weak
>
> > +{
> > + return 0;
> > +}
> > +
> > void __cpuinit calibrate_delay(void)
> > {
> > unsigned long lpj;
> > @@ -257,6 +271,8 @@ void __cpuinit calibrate_delay(void)
> > lpj = lpj_fine;
> > pr_info("Calibrating delay loop (skipped), "
> > "value calculated using timer frequency.. ");
> > + } else if ((lpj = calibrate_delay_is_known())) {
> > + ;
> > } else if ((lpj = calibrate_delay_direct()) != 0) {
> > if (!printed)
> > pr_info("Calibrating delay using timer "