Re: [RFC 13/13] m68k: mac: convert to generic clockevent

[Greg Ungerer <gerg@linux-m68k.org>]

On 30/10/20 10:41 am, Finn Thain wrote:
> On Fri, 23 Oct 2020, Arnd Bergmann wrote:
>> On Sun, Oct 18, 2020 at 2:55 AM Finn Thain <fthain@telegraphics.com.au>  wrote:
>>> On Thu, 15 Oct 2020, Arnd Bergmann wrote:
>>>> On Thu, Oct 15, 2020 at 3:19 AM Finn Thain <fthain@telegraphics.com.au> wrote:
>>>>> On Sat, 10 Oct 2020, Arnd Bergmann wrote:
>>>
>>> That configuration still produces the same 5 KiB of bloat. I see that
>>> kernel/time/Kconfig has this --
>>>
>>> # Core internal switch. Selected by NO_HZ_COMMON / HIGH_RES_TIMERS. This is
>>> # only related to the tick functionality. Oneshot clockevent devices
>>> # are supported independent of this.
>>> config TICK_ONESHOT
>>>          bool
>>>
>>> But my question was really about both kinds of dead code (oneshot
>>> device support and oneshot tick support). Anyway, after playing with
>>> the code for a bit I don't see any easy way to reduce the growth in
>>> text size.
>>
>> Did you look more deeply into where those 5KB are? Is this just the code
>> in kernel/time/{clockevents,tick-common}.c and the added platform
>> specific bits, or is there something more?
> 
> What I did was to list the relevant functions using scripts/bloat-o-meter
> and tried stubbing out some code related to oneshot clockevent devices. I
> didn't find any low fruit and don't plan to pursue that without the help
> of LTO.
> 
>> I suppose the sysfs interface and the clockevents_update_freq() logic
>> are not really needed on m68k, but it wouldn't make much sense to split
>> those out either.
>>
>> How does the 5KB bloat compare to the average bloat we get from one
>> release to the next? Geert has been collecting statistics for this.
>>
> 
> Perhaps that 5 KB is justified by gaining the hrtimers feature... hard to
> say; it's never been available on these platforms. I can see the value in
> it though.
> 
>>>> Yes, makes sense. I think the one remaining problem with the
>>>> periodic mode in this driver is that it can drop timer ticks when
>>>> interrupts are disabled for too long, while in oneshot mode there
>>>> may be a way to know how much time has passed since the last tick as
>>>> long as the counter does not overflow.
>>>
>>> Is there any benefit from adopting a oneshot tick (rather than
>>> periodic) if no clocksource is consulted when calculating the next
>>> interval? (I'm assuming NO_HZ is not in use, for reasons discussed
>>> below.)
>>
>> If the clocksource does not set CLOCK_SOURCE_IS_CONTINOUS, the kernel
>> will keep using periodic timers and not allow hrtimers.
>>
> 
> IIUC, when HIGH_RES_TIMERS=y, the kernel will enable hrtimers only if the
> platform provides both a continuous clocksource device and a oneshot
> clockevent device. However, the "jiffies" clocksource does not set
> CLOCK_SOURCE_IS_CONTINOUS and neither does the one in
> arch/arm/mach-rpc/time.c.
> 
> When HIGH_RES_TIMERS=n and NO_HZ_COMMON=n (which is presently the case for
> all platforms with GENERIC_CLOCKEVENTS=n) there's no use for a oneshot
> clockevent device, right?
> 
> It seems likely that NO_HZ_COMMON=n because the clocksources on these
> platforms produce a periodic interrupt regardless (e.g.
> clocksource/i8253.c, arm/rpc, m68k platform timers etc.).
> 
> Finally, HIGH_RES_TIMERS=n seems likely if the only clocksource is
> unreliable (e.g. because it loses time due to interrupt priorities). There
> may be a few of platforms in this category (Mac, Atari?).
> 
>>>> I would agree that despite this oneshot mode is probably worse
>>>> overall for timekeeping if the register accesses introduce
>>>> systematic errors.
>>>>
>>>
>>> It probably has to be tried. But consulting a VIA clocksource on every
>>> tick would be expensive on this platform, so if that was the only way
>>> to avoid cumulative errors, I'd probably just stick with the periodic
>>> tick.
>>
>> I'm sure there is a tradeoff somewhere. Without hrtimers, some events
>> will take longer when they have to wait for the next tick, and using
>> NO_HZ_FULL can help help make things faster on some workloads.
>>
> 
> Yes, such a tradeoff is discussed in drivers/iio/adc/ep93xx_adc.c.
> 
> But OTOH, Documentation/timers/timers-howto.rst says,
> 
>      On slower systems, (embedded, OR perhaps a speed-stepped PC!) the
>      overhead of setting up the hrtimers for usleep *may* not be worth it
> 
> I guess it has to be tried.
> 
>> ...
>>> The other 11 platforms in that category also have 'synthetic'
>>> clocksources derived from a timer reload interrupt. In 3 cases, the
>>> clocksource read method does not (or can not) check for a pending
>>> counter reload interrupt. For these also, I see no practical
>>> alternative to the approach you've taken in your RFC patch:
>>>
>>> arch/m68k/68000/timers.c
>>> arch/m68k/atari/time.c
>>> arch/m68k/coldfire/timers.c
>>
>> Agreed. It's possible there is a way, but I don't see one either.
>>
> 
> For arch/m68k/68000/timers.c, I suppose we may be able to check for the
> TMR1 bit in the Interrupt Status Register at 0xFFFFF30C or the COMP bit in
> the Timer Status Register at 0xFFFFF60A. But testing that patch could be
> difficult.
> 
> I expect that equivalent flags are available in Coldfire registers, making
> it possible to improve upon mcftmr_read_clk() and m68328_read_clk() if
> need be -- that is, if it turns out that the clocksource interrupt was
> subject to higher priority IRQs that would slow down the clocksource or
> defeat its monotonicity.
> 
> The other difficulty is a lack of hardware timers. There's only one timer
> on MC68EZ328. On Atari, for now only Timer C is available though Michael
> has said that it would be possible to free up Timer D. Some Coldfire chips
> have only 2 timers and the second timer seems to be allocated to
> profiling.

FWIW, I would have no problem with ditching the profiling clock,
and using both on ColdFire platforms that have this. I doubt anyone has 
used that profiling setup in a _very_ long time.

Regards
Greg