[RFC,0/4] treewide: Use clocksource id for get_device_system_crosststamp()

Message

Peter Hilber Aug. 18, 2023, 1:12 a.m. UTC

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.

For this, modify some code which is relevant to
get_device_system_crosststamp(), in timekeeping, ptp/kvm, x86/kvm, and
x86/tsc.

The series does the following: First, introduce clocksource ids for x86 TSC
and kvm-clock. Then, refactor the x86 TSC a tiny bit to keep changes in the
last, "treewide" patch to a minimum. In the treewide patch, replace
system_counterval_t.cs by .cs_id.

This series should not alter any behavior. Out of the existing
get_device_system_crosststamp() users, only ptp_kvm has been tested (on
x86-64 and arm64). This series is a prerequisite for the virtio_rtc driver
(RFC v2 to be posted). Through this series, virtio_rtc can work without
modifying arm_arch_timer.


Peter Hilber (4):
  x86/tsc: Add clocksource ids for TSC and early TSC
  x86/kvm: Add clocksource id for kvm-clock
  x86/tsc: Use bool, not pointer, for ART availability
  treewide: Use clocksource id for struct system_counterval_t

 arch/x86/kernel/kvmclock.c      |  2 ++
 arch/x86/kernel/tsc.c           | 23 +++++++++++++++--------
 drivers/ptp/ptp_kvm_common.c    |  3 ++-
 include/linux/clocksource_ids.h |  3 +++
 include/linux/timekeeping.h     |  4 ++--
 kernel/time/timekeeping.c       |  3 ++-
 6 files changed, 26 insertions(+), 12 deletions(-)


base-commit: 47762f08697484cf0c2f2904b8c52375ed26c8cb

Comments

John Stultz Aug. 25, 2023, 4:18 a.m. UTC | #1

On Thu, Aug 17, 2023 at 6:13 PM Peter Hilber
<[e-mail]> 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?

I just worry that switching to an enumeration solution might be
eventually exposing more than we would like to userland.

thanks
-john

Peter Hilber Sept. 13, 2023, 9:10 a.m. UTC | #2

On Fri, Aug 25, 2023 6:18 John Stultz <[e-mail]> wrote:
> On Thu, Aug 17, 2023 at 6:13 PM Peter Hilber
> <[e-mail]> 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/[e-mail]/
[2] https://lore.kernel.org/all/[e-mail]/
[3] https://lore.kernel.org/all/[e-mail]/
[4] https://lore.kernel.org/all/[e-mail]/