| Message ID | 20201008154651.1901126-14-arnd@arndb.de (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Clean up legacy clock tick users | expand |
Hi Arnd, Perhaps patch 13 does not belong in this series (?). All m68k platforms will need conversion before the TODO can be removed from Documentation/features/time/clockevents/arch-support.txt. On m68k, HZ is fixed at 100. Without addressing that, would there be any benefit from adopting GENERIC_CLOCKEVENTS as per this RFC patch? On Thu, 8 Oct 2020, Arnd Bergmann wrote: > Now that the infrastructure allows kernels to have both legacy timer > ticks and clockevent drivers in the same image, start by moving one > platform to generic clockevents. > > As qemu only supports the q800 platform among the classic m68k, use that > as an example. > Correct VIA emulation is suprisingly difficult, so this kind of work should be tested on real hardware. I say that because when I did the clocksource conversion for m68k I ran into a bug in QEMU (since fixed) and also because I once worked on some of the bugs in the emulated VIA device used in MAME/MESS. > I also tried adding oneshot mode, which was successful but broke the > clocksource. It's probably not hard to make it work properly, but this > is where I've stopped. > I'm not so sure that one timer is able to support both a clocksource driver and a clockevent driver. In some cases we may have to drop the clocksource driver (i.e. fall back on the jiffies clocksource). Anyway, even on Macs with only one VIA chip we still have two timers. So I think we should try to use Timer 1 as a freerunning clocksource and Timer 2 as a oneshot clock event. This may result in better accuracy and simpler code. This may require some experimentation though. > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > I have never tried implementing a clockevent or clocksource > driver in the past, so this is really just an experiment and > I expect I got something wrong. > Writing clockevent drivers is new to me too. I'm still trying to discover what the implications might be if the only available clockevent device offers oneshot mode or periodic mode but not both.
On Sat, Oct 10, 2020 at 12:21 AM Finn Thain <fthain@telegraphics.com.au> wrote: > > Hi Arnd, > > Perhaps patch 13 does not belong in this series (?). > > All m68k platforms will need conversion before the TODO can be removed > from Documentation/features/time/clockevents/arch-support.txt. Yes, correct. I marked this patch as RFC instead of PATCH, as I'm just trying to find out where it should be headed. I would hope the other patches can just get merged. > On m68k, HZ is fixed at 100. Without addressing that, would there be any > benefit from adopting GENERIC_CLOCKEVENTS as per this RFC patch? I don't think so, I mainly did it to see if there is a problem with mixing the two modes, and I couldn't find any. The behavior seems unchanged before and after my patch, the main difference being a few extra kilobytes in kernel .text for the generic clockevents code. > On Thu, 8 Oct 2020, Arnd Bergmann wrote: > > > Now that the infrastructure allows kernels to have both legacy timer > > ticks and clockevent drivers in the same image, start by moving one > > platform to generic clockevents. > > > > As qemu only supports the q800 platform among the classic m68k, use that > > as an example. > > > > Correct VIA emulation is suprisingly difficult, so this kind of work > should be tested on real hardware. > > I say that because when I did the clocksource conversion for m68k I ran > into a bug in QEMU (since fixed) and also because I once worked on some of > the bugs in the emulated VIA device used in MAME/MESS. Good point, though I would be surprised if anything went wrong with this patch on real hardware but not in emulation, as all the register-level interactions with the timer are the same. Adding oneshot mode is a completely different matter though, that clearly needs to be tested on real hardware. > > I also tried adding oneshot mode, which was successful but broke the > > clocksource. It's probably not hard to make it work properly, but this > > is where I've stopped. > > > > I'm not so sure that one timer is able to support both a clocksource > driver and a clockevent driver. In some cases we may have to drop the > clocksource driver (i.e. fall back on the jiffies clocksource). > > Anyway, even on Macs with only one VIA chip we still have two timers. So I > think we should try to use Timer 1 as a freerunning clocksource and Timer > 2 as a oneshot clock event. This may result in better accuracy and simpler > code. This may require some experimentation though. Ah, good. This is partly what I had been hoping for, as my patch can be used as a starting point for that if you want to give it a go. Arnd
On Sat, 10 Oct 2020, Arnd Bergmann wrote: > > Perhaps patch 13 does not belong in this series (?). > > > > All m68k platforms will need conversion before the TODO can be removed > > from Documentation/features/time/clockevents/arch-support.txt. > > Yes, correct. I marked this patch as RFC instead of PATCH, as I'm just > trying to find out where it should be headed. I would hope the other > patches can just get merged. > I wonder whether we can improve support for your proposed configuration i.e. a system with no oneshot clockevent device. The 16 platforms you identified are not all in that category but I suspect that there are others which are (though they don't appear in this series because they already use GENERIC_CLOCKEVENTS). One useful optimization would be some way to elide oneshot clockevent support (perhaps with the help of Link Time Optimization). > > On m68k, HZ is fixed at 100. Without addressing that, would there be > > any benefit from adopting GENERIC_CLOCKEVENTS as per this RFC patch? > > I don't think so, I mainly did it to see if there is a problem with > mixing the two modes, and I couldn't find any. The behavior seems > unchanged before and after my patch, the main difference being a few > extra kilobytes in kernel .text for the generic clockevents code. > I think that is a good reason to convert all m68k platforms at once and to elide some of the dead code. > > On Thu, 8 Oct 2020, Arnd Bergmann wrote: > > > > > Now that the infrastructure allows kernels to have both legacy timer > > > ticks and clockevent drivers in the same image, start by moving one > > > platform to generic clockevents. > > > > > > As qemu only supports the q800 platform among the classic m68k, use > > > that as an example. > > > > > > > Correct VIA emulation is suprisingly difficult, so this kind of work > > should be tested on real hardware. > > > > I say that because when I did the clocksource conversion for m68k I > > ran into a bug in QEMU (since fixed) and also because I once worked on > > some of the bugs in the emulated VIA device used in MAME/MESS. > > Good point, though I would be surprised if anything went wrong with this > patch on real hardware but not in emulation, as all the register-level > interactions with the timer are the same. > On the subject of register accesses, via1[ACR] is shared with ADB drivers, so this patch probably has to protect those accesses with local_irq_save/restore or local_irq_disable/enable. (I can't be sure of the contexts in which .set_state_shutdown and .set_state_periodic methods are called.) > Adding oneshot mode is a completely different matter though, that > clearly needs to be tested on real hardware. > Right, and many emulators trade-off timing accuracy for performance which makes them unsuitable for testing invasive changes of that sort. > > > I also tried adding oneshot mode, which was successful but broke the > > > clocksource. It's probably not hard to make it work properly, but > > > this is where I've stopped. > > > > > > > I'm not so sure that one timer is able to support both a clocksource > > driver and a clockevent driver. In some cases we may have to drop the > > clocksource driver (i.e. fall back on the jiffies clocksource). > > > > Anyway, even on Macs with only one VIA chip we still have two timers. > > So I think we should try to use Timer 1 as a freerunning clocksource > > and Timer 2 as a oneshot clock event. This may result in better > > accuracy and simpler code. This may require some experimentation > > though. > > Ah, good. This is partly what I had been hoping for, as my patch can be > used as a starting point for that if you want to give it a go. > After looking at the chip documentation I don't think it's viable to use the hardware timers in the way I proposed. A VIA register access requires at least one full VIA clock cycle (about 1.3 us) which means register accesses themselves cause timing delays. They also make clocksource reads expensive. I think this rules out oneshot clockevent devices because if the system offered such a device it would preferentially get used as a tick device. So I think your approach (periodic clockevent device driven by the existing periodic tick interrupt) is best for this platform due to simplicity (not much code) and performance (good accuracy, no additional overhead). I suspect the same approach would work equally well on other platforms too (even though they are probably be capable of oneshot clockevent devices). > Arnd >
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: > > > > > > Perhaps patch 13 does not belong in this series (?). > > > > > > > > All m68k platforms will need conversion before the TODO can be removed > > > > from Documentation/features/time/clockevents/arch-support.txt. > > > > > > Yes, correct. I marked this patch as RFC instead of PATCH, as I'm just > > > trying to find out where it should be headed. I would hope the other > > > patches can just get merged. > > > > > > > I wonder whether we can improve support for your proposed configuration > > i.e. a system with no oneshot clockevent device. > > > > The 16 platforms you identified are not all in that category but I suspect > > that there are others which are (though they don't appear in this series > > because they already use GENERIC_CLOCKEVENTS). > > > > One useful optimization would be some way to elide oneshot clockevent > > support (perhaps with the help of Link Time Optimization). > > I think this already happens if one picks CONFIG_HZ_PERIODIC while > disabling HIGH_RES_TIMERS. In that case, CONFIG_TICK_ONESHOT > remains disabled. > 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. > ... > > After looking at the chip documentation I don't think it's viable to > > use the hardware timers in the way I proposed. A VIA register access > > requires at least one full VIA clock cycle (about 1.3 us) which means > > register accesses themselves cause timing delays. They also make > > clocksource reads expensive. > > > > I think this rules out oneshot clockevent devices because if the > > system offered such a device it would preferentially get used as a > > tick device. > > > > So I think your approach (periodic clockevent device driven by the > > existing periodic tick interrupt) is best for this platform due to > > simplicity (not much code) and performance (good accuracy, no > > additional overhead). > > 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.) > 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. > ... > The arm/rpc timer seems to be roughly in the same category as most of > the m68k ones or the i8253 counter on a PC. It's possible that some of > them could use the same logic as drivers/clocksource/i8253.o as long as > there is any hardware oneshot mode. > There appear to be 15 platforms in that category. 4 have no clocksource besides the jiffies clocksource, meaning there's no practical alternative to using a periodic tick, like you did in your RFC patch: arch/m68k/apollo/config.c arch/m68k/q40/q40ints.c arch/m68k/sun3/sun3ints.c arch/m68k/sun3x/time.c 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 That leaves 8 platforms that have reliable clocksource devices which should be able to provide an accurate reading even in the presence of a dropped tick (due to drivers disabling interrupts for too long): arch/arm/mach-rpc/time.c arch/m68k/amiga/config.c arch/m68k/bvme6000/config.c arch/m68k/coldfire/sltimers.c arch/m68k/hp300/time.c arch/m68k/mac/via.c arch/m68k/mvme147/config.c arch/m68k/mvme16x/config.c But is there any reason to adopt a oneshot tick on any of these platforms, if NO_HZ won't eliminate the timer interrupt that's needed to run a 'synthetic' clocksource?
Hi Arnd, On Fri, Oct 23, 2020 at 9:52 AM Arnd Bergmann <arnd@kernel.org> 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? > 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. It would be a fair share of the typical increase of ca. 30 KiB per kernel release. Still, it would be lost in the noise of the increase for v5.10-rc1: add/remove: 1200/455 grow/shrink: 1419/821 up/down: 468970/-93714 (375256) Function old new delta _printk_rb_static_infos - 180224 +180224 write_buf 8192 32768 +24576 _printk_rb_static_descs - 24576 +24576 HUF_decompress4X4_usingDTable_internal - 5664 +5664 HUF_decompress4X2_usingDTable_internal - 5006 +5006 __ext4_ioctl - 4774 +4774 sock_ops_convert_ctx_access 3840 8462 +4622 ZSTD_decompressSequences - 3100 +3100 > > > The arm/rpc timer seems to be roughly in the same category as most of > > > the m68k ones or the i8253 counter on a PC. It's possible that some of > > > them could use the same logic as drivers/clocksource/i8253.o as long as > > > there is any hardware oneshot mode. > > > > There appear to be 15 platforms in that category. 4 have no clocksource > > besides the jiffies clocksource, meaning there's no practical alternative > > to using a periodic tick, like you did in your RFC patch: > > > > arch/m68k/apollo/config.c > > arch/m68k/q40/q40ints.c > > arch/m68k/sun3/sun3ints.c > > arch/m68k/sun3x/time.c > > Do any of these have users? I'm fairly sure sun3x has never worked in mainline, > sun3 seems to still need the same few patches it did 20 years ago. I > couldn't find > much about Linux on Apollo or q40, the information on the web for either > of them seems to all be for linux-2.4 kernels. They probably don't have any users. AFAIK, the only users are the usual triumvirate of amiga/atari/mac (and perhaps the fleet of VME boards in the Australian navy? ;) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, Oct 23, 2020 at 11:24 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Fri, Oct 23, 2020 at 9:52 AM Arnd Bergmann <arnd@kernel.org> 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? > > 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. > > It would be a fair share of the typical increase of ca. 30 KiB per > kernel release. Still, it would be lost in the noise of the increase for > v5.10-rc1: > > add/remove: 1200/455 grow/shrink: 1419/821 up/down: 468970/-93714 (375256) > Function old new delta > _printk_rb_static_infos - 180224 +180224 > write_buf 8192 32768 +24576 > _printk_rb_static_descs - 24576 +24576 > HUF_decompress4X4_usingDTable_internal - 5664 +5664 > HUF_decompress4X2_usingDTable_internal - 5006 +5006 > __ext4_ioctl - 4774 +4774 > sock_ops_convert_ctx_access 3840 8462 +4622 > ZSTD_decompressSequences - 3100 +3100 FTR, 3.9 KiB reclaimed by upgrading from gcc 8.4.0 in Ubuntu 18.04LTS to gcc 9.3.0 in Ubuntu 20.04LTS. Gr{oetje,eeting}s, Geert
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. > > That leaves 8 platforms that have reliable clocksource devices which > > should be able to provide an accurate reading even in the presence of > > a dropped tick (due to drivers disabling interrupts for too long): > > > > arch/arm/mach-rpc/time.c > > arch/m68k/amiga/config.c > > arch/m68k/bvme6000/config.c > > arch/m68k/coldfire/sltimers.c > > arch/m68k/hp300/time.c > > arch/m68k/mac/via.c > > arch/m68k/mvme147/config.c > > arch/m68k/mvme16x/config.c > > > > But is there any reason to adopt a oneshot tick on any of these platforms, > > if NO_HZ won't eliminate the timer interrupt that's needed to run a > > 'synthetic' clocksource? > > I would expect that these could be changed to behave more like > drivers/clocksource/i8253.c, which does support a clocksource in oneshot > mode. > I think you meant to write, "... support a clockevent device in oneshot mode". I would agree. Since Macs do have a spare hardware timer, I will attempt to convert arch/m68k/mac/via.c to a oneshot clockevent, using your GENERIC_CLOCKEVENTS patch series as a basis, and see what effect that has in the NO_HZ_COMMON=n, HIGH_RES_TIMERS=y case. > Arnd >
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
On Fri, 23 Oct 2020, Geert Uytterhoeven wrote: > > > > The arm/rpc timer seems to be roughly in the same category as most > > > > of the m68k ones or the i8253 counter on a PC. It's possible that > > > > some of them could use the same logic as > > > > drivers/clocksource/i8253.o as long as there is any hardware > > > > oneshot mode. > > > > > > There appear to be 15 platforms in that category. 4 have no > > > clocksource besides the jiffies clocksource, meaning there's no > > > practical alternative to using a periodic tick, like you did in your > > > RFC patch: > > > > > > arch/m68k/apollo/config.c > > > arch/m68k/q40/q40ints.c > > > arch/m68k/sun3/sun3ints.c > > > arch/m68k/sun3x/time.c > > > > Do any of these have users? I'm fairly sure sun3x has never worked in > > mainline, sun3 seems to still need the same few patches it did 20 > > years ago. I couldn't find much about Linux on Apollo or q40, the > > information on the web for either of them seems to all be for > > linux-2.4 kernels. > > They probably don't have any users. I have access to several Sun 3 machines but no time to work on that port, unfortunately. Are these 4 platforms (those with no clocksource besides the "jiffies" clocksource) the only reason for CONFIG_TIME_LOW_RES on m68k?
On Fri, 30 Oct 2020, Greg Ungerer wrote: > > > > > ... > > > > 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. > If we ditched the Coldfire profiling clock, it would be possible to dedicate one hardware timer to the clocksource device and the other to the (oneshot) clockevent device. That's a win if it means that the clocksource can use the full counter width (making timer interrupts less frequent and timer interrupt latency less problematic) and run at higher frequency (making the clocksource more precise). Also, note that hrtimers won't work with a periodic clockevent device (as in Arnd's RFC patch). If you want hrtimers, I think you'll need both hardware timers or else re-implement the existing synthetic clocksource using the same oneshot timer driving a new oneshot clockevent device. Please note that the lack of a spare hardware timer is a separate issue to the failure of mcftmr_read_clk() and m68328_read_clk() to check the timer reload interrupt flag (which may make those clocksources needlessly susceptible to issues caused by timer interrupt latency...).
On Fri, Nov 06, 2020 at 01:52:01PM +1100, Finn Thain wrote: > On Fri, 23 Oct 2020, Geert Uytterhoeven wrote: > > > > > > The arm/rpc timer seems to be roughly in the same category as most > > > > > of the m68k ones or the i8253 counter on a PC. It's possible that > > > > > some of them could use the same logic as > > > > > drivers/clocksource/i8253.o as long as there is any hardware > > > > > oneshot mode. > > > > > > > > There appear to be 15 platforms in that category. 4 have no > > > > clocksource besides the jiffies clocksource, meaning there's no > > > > practical alternative to using a periodic tick, like you did in your > > > > RFC patch: > > > > > > > > arch/m68k/apollo/config.c > > > > arch/m68k/q40/q40ints.c > > > > arch/m68k/sun3/sun3ints.c > > > > arch/m68k/sun3x/time.c > > > > > > Do any of these have users? I'm fairly sure sun3x has never worked in > > > mainline, sun3 seems to still need the same few patches it did 20 > > > years ago. I couldn't find much about Linux on Apollo or q40, the > > > information on the web for either of them seems to all be for > > > linux-2.4 kernels. > > > > They probably don't have any users. > > I have access to several Sun 3 machines but no time to work on that port, > unfortunately. > > Are these 4 platforms (those with no clocksource besides the "jiffies" > clocksource) the only reason for CONFIG_TIME_LOW_RES on m68k? Sun3x was probably at least as close (if not closer) than Sun3 to working in mainline back in the day, but unfortunately I'm in the same place as Finn... I've got the hardware, but time is harder to come by. -- Sam
diff --git a/arch/m68k/Kconfig.machine b/arch/m68k/Kconfig.machine index 8a4e8bd8aade..cccabdad618e 100644 --- a/arch/m68k/Kconfig.machine +++ b/arch/m68k/Kconfig.machine @@ -30,7 +30,7 @@ config MAC depends on MMU select MMU_MOTOROLA if MMU select HAVE_ARCH_NVRAM_OPS - select LEGACY_TIMER_TICK + select GENERIC_CLOCKEVENTS help This option enables support for the Apple Macintosh series of computers (yes, there is experimental support now, at least for part diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c index 8ad734e3c934..dd4c13c318b6 100644 --- a/arch/m68k/mac/via.c +++ b/arch/m68k/mac/via.c @@ -24,6 +24,7 @@ */ #include <linux/clocksource.h> +#include <linux/clockchips.h> #include <linux/types.h> #include <linux/kernel.h> #include <linux/mm.h> @@ -602,27 +603,54 @@ static u32 clk_total, clk_offset; static irqreturn_t via_timer_handler(int irq, void *dev_id) { + struct clock_event_device *evt = dev_id; + clk_total += VIA_TIMER_CYCLES; clk_offset = 0; - legacy_timer_tick(1); + evt->event_handler(evt); return IRQ_HANDLED; } -void __init via_init_clock(void) +static int via_set_periodic(struct clock_event_device *evt) { - if (request_irq(IRQ_MAC_TIMER_1, via_timer_handler, IRQF_TIMER, "timer", - NULL)) { - pr_err("Couldn't register %s interrupt\n", "timer"); - return; - } - via1[vT1LL] = VIA_TC_LOW; via1[vT1LH] = VIA_TC_HIGH; via1[vT1CL] = VIA_TC_LOW; via1[vT1CH] = VIA_TC_HIGH; via1[vACR] |= 0x40; + return 0; +} + +static int via_set_shutdown(struct clock_event_device *evt) +{ + via1[vACR] &= ~0x40; + + return 0; +} + +static struct clock_event_device via_clk_event = { + .name = "via1", + .rating = 250, + .irq = IRQ_MAC_TIMER_1, + .owner = THIS_MODULE, + + .features = CLOCK_EVT_FEAT_PERIODIC, + .set_state_shutdown = via_set_shutdown, + .set_state_periodic = via_set_periodic, +}; + +void __init via_init_clock(void) +{ + clockevents_config_and_register(&via_clk_event, VIA_CLOCK_FREQ, 1, 0xffff); + + if (request_irq(IRQ_MAC_TIMER_1, via_timer_handler, IRQF_TIMER, "timer", + &via_clk_event)) { + pr_err("Couldn't register %s interrupt\n", "timer"); + return; + } + clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ); }
Now that the infrastructure allows kernels to have both legacy timer ticks and clockevent drivers in the same image, start by moving one platform to generic clockevents. As qemu only supports the q800 platform among the classic m68k, use that as an example. I also tried adding oneshot mode, which was successful but broke the clocksource. It's probably not hard to make it work properly, but this is where I've stopped. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- I have never tried implementing a clockevent or clocksource driver in the past, so this is really just an experiment and I expect I got something wrong. arch/m68k/Kconfig.machine | 2 +- arch/m68k/mac/via.c | 44 ++++++++++++++++++++++++++++++++------- 2 files changed, 37 insertions(+), 9 deletions(-)