Re: [PATCH v2 18/20] mips: csrc-r4k: Decrease r4k-clocksource rating if CPU_FREQ enabled

[Serge Semin]

Thomas,
Could you take a look at my comment below so I could proceed with the
patchset v3 development?

-Sergey

On Mon, May 11, 2020 at 04:31:21PM +0300, Serge Semin wrote:
> On Fri, May 08, 2020 at 05:41:50PM +0200, Thomas Bogendoerfer wrote:
> > On Wed, May 06, 2020 at 08:42:36PM +0300, Sergey.Semin@xxxxxxxxxxxxxxxxxxxx wrote:
> > > From: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > > 
> > > Commit 07d69579e7fe ("MIPS: Don't register r4k sched clock when
> > > CPUFREQ enabled") disabled the r4k-clock usage for scheduler ticks
> > > counting due to the scheduler being non-tolerant for unstable
> > > clocks sources. For the same reason the clock should be used
> > > in the system clocksource framework only as a last resort if CPU
> > > frequency may change.
> > > 
> > > Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > > Cc: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> > > Cc: Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx>
> > > Cc: Paul Burton <paulburton@xxxxxxxxxx>
> > > Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > > Cc: linux-pm@xxxxxxxxxxxxxxx
> > > Cc: devicetree@xxxxxxxxxxxxxxx
> > > ---
> > >  arch/mips/kernel/csrc-r4k.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/arch/mips/kernel/csrc-r4k.c b/arch/mips/kernel/csrc-r4k.c
> > > index 437dda64fd7a..d81fb374f477 100644
> > > --- a/arch/mips/kernel/csrc-r4k.c
> > > +++ b/arch/mips/kernel/csrc-r4k.c
> > > @@ -71,7 +71,11 @@ int __init init_r4k_clocksource(void)
> > >  		return -ENXIO;
> > >  
> > >  	/* Calculate a somewhat reasonable rating value */
> > > +#ifndef CONFIG_CPU_FREQ
> > >  	clocksource_mips.rating = 200 + mips_hpt_frequency / 10000000;
> > > +#else
> > > +	clocksource_mips.rating = 99;
> > > +#endif
> > 
> > I dislike this patch. Assuming you have an other clocksource, why not
> > simply disable csrc-r4k, if CPU_FREQ is enabled ?
> 
> Me neither and the best way would be to update the clocksource frequency
> dynamically the same way it's done for cevt-r4k and MIPS GIC timers. Alas the
> clocksource doesn't support it. Due to this together with CPU-freq facility
> enabled we have to use a very slow DW APB Timer instead of the fast embedded
> into the CPU core r4k and MIPS GIC timers. Just note the difference: it takes
> 220 ns to read the counter from DW APB Timer in comparison to a few nanoseconds
> reading from MIPS GIC and R4K. So IMO disabling the timer as you suggest isn't
> the best option. By making the CPUFREQ and CSRC_R4K mutual exclusive we'd
> assume a use-case that the system will always use the CPU-freq facility changing
> the CPU reference frequency. This is obviously not true. Noone prevents the
> system administrator to leave the default setting of the CPU-freq with fixed
> frequency and select a faster, more accurate timer like in our case.
> 
> My idea was not to try to predict how the system would be used, but to let the
> system administration to choose which timer is applicable in particular usecase
> enabling a safest one by default. So if CPUFREQ is available, then we fallback
> to the external timer as safest one. If the system user wouldn't need to have
> the CPUFREQ facility utilized, then the system administrator would want to
> leave the default CPU-freq governor with pre-defined CPU frequency and
> select either R4K (MIPS) or MIPS GIC timer just by writing its name into
> /sys/bus/clocksource/devices/clocksource0/current_clocksource .
>  
> I should note, that currently CPU_FREQ won't be available if there is no
> MIPS_EXTERNAL_TIMER available for the platform. It's prohibited by means of the
> conditional kbuild config inclusion declared in the arch/mips/Kconfig:
> + if CPU_SUPPORTS_CPUFREQ && MIPS_EXTERNAL_TIMER
> + source "drivers/cpufreq/Kconfig"
> + endif
> So if there is no external timer working independently from the CPU core clock
> source, the CPUFREQ won't be available to select for the kernel. Though currently
> this limitation is supposed to be applicable for the R4K/MIPS GIC clocksource
> timers only since clockevents must work fine in unstable reference clock conditions.
> 
> So what can we do to improve the patch? First one is a solution I suggested in
> this patch but it could be a bit altered by using IS_ENABLED() macro to:
> + clocksource_mips.rating = !IS_ENABLED(CONFIG_CPU_FREQ) ?
> +			    200 + mips_hpt_frequency / 10000000 : 99;
> 
> Another idea I discovered when have been searching through the x86 arch code.
> x86's got the same problem with TSC timer, but it doesn't disable it if
> CPU-frequency is switched on. Instead it just marks it as unstable by calling
> the clocksource_mark_unstable() method if CPU frequency changes. I suggest to
> implement the same approach in our case of MIPS GIC (another patchset
> I've sent, see "clocksource: Fix MIPS GIC and DW APB Timer for Baikal-T1 SoC
> support" in your email client) and R4K timers. We'll subscribe to the CPU
> frequency change and if it changes we'll call clocksource_mark_unstable() with
> MIPS GIC and R4K clocksource handlers passed. This shall reduce their rating and
> cause selecting a clocksource with better one. BTW I suppose it won't be
> necessary to initially lower the rating of the MIPS GIC and R4K clocksource
> timers if this is implemented.
> 
> So, what do you think?
> 
> -Sergey
> 
> > 
> > Thomas.
> > 
> > -- 
> > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> > good idea.                                                [ RFC1925, 2.3 ]