Messages in this thread | | | Date | Wed, 13 Sep 2023 11:10:21 +0200 | From | Peter Hilber <> | Subject | Re: [RFC PATCH 0/4] treewide: Use clocksource id for get_device_system_crosststamp() |
| |
On Fri, Aug 25, 2023 6:18 John Stultz <jstultz@google.com> wrote: > On Thu, Aug 17, 2023 at 6:13 PM Peter Hilber > <peter.hilber@opensynergy.com> wrote: >> >> This patch series changes struct system_counterval_t to identify the >> clocksource through enum clocksource_ids, rather than through struct >> clocksource *. The net effect of the patch series is that >> get_device_system_crosststamp() callers can supply clocksource ids instead >> of clocksource pointers, which can be problematic to get hold of. > > Hey Peter, > Thanks for sending this out. I'm a little curious though, can you > expand a bit on how clocksource pointers can be problematic to get a > hold of? What exactly is the problem that is motivating this change? >
Hi John,
I'm very sorry for the late reply; there was some unexpected delay.
Thank you for the remark; I'll expand on the motivation in the next patch series iteration, similar to the explanation below.
The immediate motivation for this patch series is to enable the virtio_rtc RFC v2 driver [4] to refer to the Arm Generic Timer without requiring new helper functions in the arm_arch_timer driver. Other future get_device_system_crosststamp() users may profit from this change as well.
Clocksource structs are normally private to clocksource drivers. Therefore, get_device_system_crosststamp() callers require that clocksource drivers expose the clocksource of interest in some way.
Drivers such as virtio_rtc [4] could obtain all information for calling get_device_system_crosststamp() from their bound device, except for clocksource identification. Such drivers' only direct relation with the clocksource driver is clocksource identification. So using the clocksource enum, rather than obtaining pointers in a clocksource driver specific way, would reduce the coupling between the get_device_system_crosststamp() callers and clocksource drivers.
Next, I provide some details to support the low coupling argument. There are two sorts of get_device_system_crosststamp() callers in the current kernel:
1) On Intel platforms, some PTP hardware clocks obtain the clocksource pointer for get_device_system_crosststamp() using convert_art_to_tsc() or convert_art_ns_to_tsc() from arch/x86.
2) The ptp_kvm driver uses kvm_arch_ptp_get_crosststamp(), which is implemented for platforms with kvm_clock or arm_arch_timer. Amongst other things, kvm_arch_ptp_get_crosststamp() returns a clocksource pointer. The Arm implementation is in the arm_arch_timer driver.
When I proposed in the virtio_rtc RFC v1 patch series [3] to obtain the clocksource pointer of the arm_arch_timer driver through a generic helper function, one of the maintainers wasn't very enthusiastic about it and suggested reusing kvm_arch_ptp_get_crosststamp() somehow [1]. But to me there seems not to be much in common [2].
Quoting myself from [2]:
> If[!] &clocksource_counter should not be exposed, then I can see two > alternatives: > > Alternative 1: Put a function of type > > int (*get_time_fn) (ktime_t *device_time, > struct system_counterval_t *sys_counterval, > void *ctx) > > into arm_arch_timer.c, as required by get_device_system_crosststamp() > (and include a virtio_rtc header).
This looks inelegant, since it would require virtio_rtc to put part of its code into arm_arch_timer.c, and would require including a virtio_rtc header in arm_arch_timer.c.
The second alternative is using this patch series to expand the use of the clocksource enum to get_device_system_crosststamp(). This should also make it easy to use get_device_system_crosststamp() with other clocksources in the future, by just extending the clocksource enum.
> I just worry that switching to an enumeration solution might be > eventually exposing more than we would like to userland.
ATM the enum is not in a UAPI header. So IMHO exposing this to userland in the future would require a pretty explicit change.
Thanks for the review,
Peter
[1] https://lore.kernel.org/all/87ila4qwuw.wl-maz@kernel.org/ [2] https://lore.kernel.org/all/151befb2-8fbc-b796-47bb-39960a979065@opensynergy.com/ [3] https://lore.kernel.org/all/20230630171052.985577-1-peter.hilber@opensynergy.com/ [4] https://lore.kernel.org/all/20230818012014.212155-1-peter.hilber@opensynergy.com/
| |