Why call calibrate_delay() in smp.c: secondary_start_kernel()

Why call calibrate_delay() in smp.c: secondary_start_kernel()

[Jonas Aaberg]

Hi,

calibrate_delay() calculates loops_per_jiffy. loops_per_jiffy is not per cpu.
Therefore I wonder why there is a calibrate_delay() call in smp.c: 
secondary_start_kernel()?

If you don't set lpj= it looks like it just takes longer time to boot
and bring a secondary cpu online.

u8500 seems to do fine without the calibrate_delay() call.

Best regards,
 Jonas Aaberg

Why call calibrate_delay() in smp.c: secondary_start_kernel()

[Santosh Shilimkar]

> -----Original Message-----
> From: linux-arm-kernel-bounces at lists.infradead.org [mailto:linux-
> arm-kernel-bounces at lists.infradead.org] On Behalf Of Jonas Aaberg
> Sent: Tuesday, January 18, 2011 2:14 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: STEricsson_nomadik_linux
> Subject: Why call calibrate_delay() in smp.c:
> secondary_start_kernel()
>
>
> Hi,
>
> calibrate_delay() calculates loops_per_jiffy. loops_per_jiffy is not
> per cpu.
> Therefore I wonder why there is a calibrate_delay() call in smp.c:
> secondary_start_kernel()?
>
> If you don't set lpj= it looks like it just takes longer time to
> boot
> and bring a secondary cpu online.
>
> u8500 seems to do fine without the calibrate_delay() call.
>
I did send a patch on the same some time back but the conclusion
was we still need to have calibration.

Have one more patch do deal with it so that platform can choose
if they like to skip. My mailer might screw the patch hence attaching
the same

Why call calibrate_delay() in smp.c: secondary_start_kernel()

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 03:46:54PM +0530, Santosh Shilimkar wrote:
> I did send a patch on the same some time back but the conclusion
> was we still need to have calibration.
> 
> Have one more patch do deal with it so that platform can choose
> if they like to skip. My mailer might screw the patch hence attaching
> the same

Actually, the secondary cores probably get a far more accurate lpj
than the primary core as they don't have the interference from the
timer interrupt.  So - if we care - we probably want to update the
primary lpj with the secondary's calibration value at boot.

On the measurements I've made a couple of weeks ago, the lpj value
can be .7% too slow, resulting in udelay() giving shorter than
requested delays.  I asked Linus about that, and he's happy with
that figure.

So the myth which floats around on various lists about udelay() giving
at least the requested delay is just that - a myth.  It has always
given _approximately_ the requested delay on all architectures with
software loop based implementations (as well as, according to Linus,
some x86 tsc implementations of udelay.)

Why call calibrate_delay() in smp.c: secondary_start_kernel()

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 18, 2011 5:01 PM
> To: Santosh Shilimkar
> Cc: Jonas Aaberg; linux-arm-kernel at lists.infradead.org;
> STEricsson_nomadik_linux
> Subject: Re: Why call calibrate_delay() in smp.c:
> secondary_start_kernel()
>
> On Tue, Jan 18, 2011 at 03:46:54PM +0530, Santosh Shilimkar wrote:
> > I did send a patch on the same some time back but the conclusion
> > was we still need to have calibration.
> >
> > Have one more patch do deal with it so that platform can choose
> > if they like to skip. My mailer might screw the patch hence
> attaching
> > the same
>
> Actually, the secondary cores probably get a far more accurate lpj
> than the primary core as they don't have the interference from the
> timer interrupt.  So - if we care - we probably want to update the
> primary lpj with the secondary's calibration value at boot.
>
> On the measurements I've made a couple of weeks ago, the lpj value
> can be .7% too slow, resulting in udelay() giving shorter than
> requested delays.  I asked Linus about that, and he's happy with
> that figure.
>
> So the myth which floats around on various lists about udelay()
> giving
> at least the requested delay is just that - a myth.  It has always
> given _approximately_ the requested delay on all architectures with
> software loop based implementations (as well as, according to Linus,
> some x86 tsc implementations of udelay.)

Ok. Since the udelay() accuracy is acceptable now, what you think
of my latest patch.

It does help for the archs which are ok to skip the calibration .
And since it's configurable with the proposed patch,
the default kernel behavior is maintained if the option isn't
selected.

Regards,
Santosh

Why call calibrate_delay() in smp.c: secondary_start_kernel()

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 05:42:22PM +0530, Santosh Shilimkar wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > Sent: Tuesday, January 18, 2011 5:01 PM
> > To: Santosh Shilimkar
> > Cc: Jonas Aaberg; linux-arm-kernel at lists.infradead.org;
> > STEricsson_nomadik_linux
> > Subject: Re: Why call calibrate_delay() in smp.c:
> > secondary_start_kernel()
> >
> > On Tue, Jan 18, 2011 at 03:46:54PM +0530, Santosh Shilimkar wrote:
> > > I did send a patch on the same some time back but the conclusion
> > > was we still need to have calibration.
> > >
> > > Have one more patch do deal with it so that platform can choose
> > > if they like to skip. My mailer might screw the patch hence
> > attaching
> > > the same
> >
> > Actually, the secondary cores probably get a far more accurate lpj
> > than the primary core as they don't have the interference from the
> > timer interrupt.  So - if we care - we probably want to update the
> > primary lpj with the secondary's calibration value at boot.
> >
> > On the measurements I've made a couple of weeks ago, the lpj value
> > can be .7% too slow, resulting in udelay() giving shorter than
> > requested delays.  I asked Linus about that, and he's happy with
> > that figure.
> >
> > So the myth which floats around on various lists about udelay()
> > giving
> > at least the requested delay is just that - a myth.  It has always
> > given _approximately_ the requested delay on all architectures with
> > software loop based implementations (as well as, according to Linus,
> > some x86 tsc implementations of udelay.)
> 
> Ok. Since the udelay() accuracy is acceptable now, what you think
> of my latest patch.
> 
> It does help for the archs which are ok to skip the calibration .
> And since it's configurable with the proposed patch,
> the default kernel behavior is maintained if the option isn't
> selected.

We should also skip printing the "total bogomips" value too if CPUs were
skipped.

Why call calibrate_delay() in smp.c: secondary_start_kernel()

[Santosh Shilimkar]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, January 18, 2011 5:47 PM
> To: Santosh Shilimkar
> Cc: Jonas Aaberg; linux-arm-kernel at lists.infradead.org;
> STEricsson_nomadik_linux
> Subject: Re: Why call calibrate_delay() in smp.c:
> secondary_start_kernel()
>
> On Tue, Jan 18, 2011 at 05:42:22PM +0530, Santosh Shilimkar wrote:
> > > -----Original Message-----
> > > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > > Sent: Tuesday, January 18, 2011 5:01 PM
> > > To: Santosh Shilimkar
> > > Cc: Jonas Aaberg; linux-arm-kernel at lists.infradead.org;
> > > STEricsson_nomadik_linux
> > > Subject: Re: Why call calibrate_delay() in smp.c:
> > > secondary_start_kernel()
> > >
> > > On Tue, Jan 18, 2011 at 03:46:54PM +0530, Santosh Shilimkar
> wrote:
> > > > I did send a patch on the same some time back but the
> conclusion
> > > > was we still need to have calibration.
> > > >
> > > > Have one more patch do deal with it so that platform can
> choose
> > > > if they like to skip. My mailer might screw the patch hence
> > > attaching
> > > > the same
> > >
> > > Actually, the secondary cores probably get a far more accurate
> lpj
> > > than the primary core as they don't have the interference from
> the
> > > timer interrupt.  So - if we care - we probably want to update
> the
> > > primary lpj with the secondary's calibration value at boot.
> > >
> > > On the measurements I've made a couple of weeks ago, the lpj
> value
> > > can be .7% too slow, resulting in udelay() giving shorter than
> > > requested delays.  I asked Linus about that, and he's happy with
> > > that figure.
> > >
> > > So the myth which floats around on various lists about udelay()
> > > giving
> > > at least the requested delay is just that - a myth.  It has
> always
> > > given _approximately_ the requested delay on all architectures
> with
> > > software loop based implementations (as well as, according to
> Linus,
> > > some x86 tsc implementations of udelay.)
> >
> > Ok. Since the udelay() accuracy is acceptable now, what you think
> > of my latest patch.
> >
> > It does help for the archs which are ok to skip the calibration .
> > And since it's configurable with the proposed patch,
> > the default kernel behavior is maintained if the option isn't
> > selected.
>
> We should also skip printing the "total bogomips" value too if CPUs
> were
> skipped.
Can be done. I can wrap the "total bogomips" print part inside the
ARCH_SKIP_SECONDARY_CALIBRATE.

Regards,
Santosh

Why call calibrate_delay() in smp.c: secondary_start_kernel()

[Linus Walleij]

2011/1/18 Santosh Shilimkar <santosh.shilimkar@ti.com>:

> +config ARCH_SKIP_SECONDARY_CALIBRATE
> + ? ? ? bool "Skip secondary CPU calibration"
> + ? ? ? depends on SMP
> + ? ? ? help
> + ? ? ? ? On some architectures, secondary cores shares clock with
> primiary
> + ? ? ? ? core and hence scale together. Hence secondary core lpj
> calibration
> + ? ? ? ? is not necessary and can be skipped to save considerable time.
> +
> + ? ? ? ? If unsure, say n.

Why do you want to select that manually? Surely the kernel
writer should know whether the cores are clocked together
or not. Just:

config ARCH_HAS_COMMON_CORES_CLOCK
      bool
      depends on SMP

This will be default "n" so archs that want to flag to the
build system that the core clock is common can e.g.:

config ARCH_U8500
        bool "ST-Ericsson U8500 Series"
        select CPU_V7
+       select ARCH_HAS_COMMON_CORES_CLOCK

Looks reasonable? (And feel free to add that to U8500
as part of the patch.)

> ?/*
> + * Skip the secondary calibration on architectures sharing clock
> + * with primary cpu. Archs can use ARCH_SKIP_SECONDARY_CALIBRATE
> + * for this.
> + */
> +static inline int skip_secondary_calibrate(void)

bool? Returning an error code seem overdesigned for a function
named like that.

Also the name is misleading, if you look at how you
use it, it should be named "do_not_skip_secondary_calibrate()".

> +{
> +#ifdef CONFIG_ARCH_SKIP_SECONDARY_CALIBRATE
> + ? ? ? return 0;

return false;

> +#else
> + ? ? ? return -ENXIO;

return true;

> +#endif
> +}


> +
> +/*
> ?* This is the secondary CPU boot entry. ?We're using this CPUs
> ?* idle thread stack, but a set of temporary page tables.
> ?*/
> @@ -312,7 +326,8 @@ asmlinkage void __cpuinit secondary_start_kernel(void)
> ? ? ? ? */
> ? ? ? ?percpu_timer_setup();
>
> - ? ? ? calibrate_delay();
> + ? ? ? if (skip_secondary_calibrate())
> + ? ? ? ? ? ? ? calibrate_delay();

Change sematics of that function and
if (!skip_secondary_calibrate())
  ...

> ? ? ? ?smp_store_cpu_info(cpu);

...and thanks for driving this!
Linus Walleij

Why call calibrate_delay() in smp.c: secondary_start_kernel()

[Santosh Shilimkar]

> -----Original Message-----
> From: linus.ml.walleij at gmail.com [mailto:linus.ml.walleij at gmail.com]
> On Behalf Of Linus Walleij
> Sent: Tuesday, January 18, 2011 8:48 PM
> To: Santosh Shilimkar
> Cc: Russell King - ARM Linux; Jonas Aaberg; linux-arm-
> kernel at lists.infradead.org; STEricsson_nomadik_linux
> Subject: Re: Why call calibrate_delay() in smp.c:
> secondary_start_kernel()
>
> 2011/1/18 Santosh Shilimkar <santosh.shilimkar@ti.com>:
>

[...]

>
> Why do you want to select that manually? Surely the kernel
> writer should know whether the cores are clocked together
> or not. Just:
>
> config ARCH_HAS_COMMON_CORES_CLOCK
>       bool
>       depends on SMP
>
> This will be default "n" so archs that want to flag to the
> build system that the core clock is common can e.g.:
>
> config ARCH_U8500
>         bool "ST-Ericsson U8500 Series"
>         select CPU_V7
> +       select ARCH_HAS_COMMON_CORES_CLOCK
>
> Looks reasonable? (And feel free to add that to U8500
> as part of the patch.)
>
This looks better. Thanks will add U8500

> > ?/*
> > + * Skip the secondary calibration on architectures sharing clock
> > + * with primary cpu. Archs can use ARCH_SKIP_SECONDARY_CALIBRATE
> > + * for this.
> > + */
> > +static inline int skip_secondary_calibrate(void)
>
> bool? Returning an error code seem overdesigned for a function
> named like that.
>
> Also the name is misleading, if you look at how you
> use it, it should be named "do_not_skip_secondary_calibrate()".
>
> > +{
> > +#ifdef CONFIG_ARCH_SKIP_SECONDARY_CALIBRATE
> > + ? ? ? return 0;
>
> return false;
>
> > +#else
> > + ? ? ? return -ENXIO;
>
> return true;
>
> > +#endif
> > +}
>
>
> > +
> > +/*
> > ?* This is the secondary CPU boot entry. ?We're using this CPUs
> > ?* idle thread stack, but a set of temporary page tables.
> > ?*/
> > @@ -312,7 +326,8 @@ asmlinkage void __cpuinit
> secondary_start_kernel(void)
> > ? ? ? ? */
> > ? ? ? ?percpu_timer_setup();
> >
> > - ? ? ? calibrate_delay();
> > + ? ? ? if (skip_secondary_calibrate())
> > + ? ? ? ? ? ? ? calibrate_delay();
>
> Change sematics of that function and
> if (!skip_secondary_calibrate())
>   ...
>
> > ? ? ? ?smp_store_cpu_info(cpu);
>
Fair enough.

> ...and thanks for driving this!

Np.

Regards,
Santosh