I think the inline here is IMO misleading, as you end up taking a pointer to the function when filling the tsc_selector struct, so the inline is just dropped.

I think it would be enough to just say that the TSC_STABLE is mandatory because TSCs must be synchronized across CPUs or else using vcpu time info of CPU 0 on for all CPUs would be wrong.

Do you really need to use the dma subsystem for this? Isn't it enough to just malloc a region of memory and use it?

You could then also use M_WAITOK maybe?

I think this should return EACCES as the file only allows opening in read-only mode.

Couldn't vDSO became functional at some later point (ie: when migrated to a different host) even if the current vcpu_time_info reports non-stable? I wonder whether we should just check if the stable flag is supported instead of also checking if it's currently set.

Given the list of arguments here I think we should consider creating a structure and passing a pointer to it, or rather fill some of the pvclock struct fields by the caller?

You could split the fields of pvclock between the public ones to be filled by the caller and the private ones.

Nit: I usually try to avoid splitting such log messages because grepping for them afterwards is not possible. I would rather try to place the message starting on the newline as you won't have to split it then.

Given the pvclock has a resolution of 1ns and that's fixed I think it would be fine to just hardcode this as 1us. It's not like PVCLOCK_RESOLUTION_NS can or will be modified anyway?

I decided to just remove this comment altogether.

It was basically just attempting to point out how this flag can theoretically change at any time, which would explain how we could arrive at this location from binuptime() even though binuptime(), indirectly via tk_enabled, just observed PVCLOCK_FLAG_TSC_STABLE as set.

But I don't think it's as subtle of a point as I must have when I originally wrote this, and I can see how this comment might confuse if __vdso_pvclock_tsc() is read separately from binuptime().

Do you really need to use the dma subsystem for this? Isn't it enough to just malloc a region of memory and use it?

I'd decided against malloc() since it doesn't provide API-level alignment guarantees; while, implementation-wise, it does look like >= PAGE_SIZE-ed malloc(9) allocations will be page-aligned, my read of malloc(9) (particularly section IMPLEMENTATION NOTES) is that this should not be assumed by callers.

(FWIW, I had also noted (1) the precedent of hyperv's use of BUS_DMA(9) for its corresponding PV clock in-memory data and (2) internally, this draft's use of BUS_DMA(9) will ultimately do the same thing that the first draft was doing with its direct use of the physical memory pager).

Just recapping, the intention is that timeinfos be readily mmap-able---the allocation containing timeinfos should start and end on a page boundary (with any unused portion of the last page zeroed).

Does this choice make sense or am I missing something?

You could then also use M_WAITOK maybe?

This choice was considering use cases where the driver is loaded during boot (whether compiled in or via loader.conf(5)). My thinking was that a failure to satisfy a relatively small allocation like this during boot would be an indication of far greater problems and, as such, it wouldn't be worth adding any blocking+reclaims+retries to the boot process, especially since the system can still function without kvmclock.

But I suppose it's also ok to allow the reclaims+retries, so, I've switched bus_dmamem_alloc() from BUS_DMA_NOWAIT to BUS_DMA_WAITOK in this version as per this suggestion.

Right. But pvclock_tc_vdso_timehands{,32}() is called with each vDSO timehands update, so, such scenarios should be supported, no?

I think the (pvc->ti_vcpu0_page->flags & PVCLOCK_FLAG_TSC_STABLE) != 0 clause could, indeed, be dropped. But I included it thinking that, in the unstable case, we might as well short-circuit the vDSO codepath sooner than later---with this clause, the vDSO codepath decides to fall back to the syscall codepath after it looks at tk->tk_enabled in binuptime() whereas, without this clause, this decision will happen after the PVCLOCK_FLAG_TSC_STABLE flag check in __vdso_pvclock_tsc().

I obtained a rough measurement of the delta between these two versions of the unstable TSC codepath by inverting the PVCLOCK_FLAG_TSC_STABLE check in __vdso_pvclock_tsc() and then looking at syscall_timing gettimeofday numbers for (1) kern.timecounter.fast_gettime=0 and (2) kern.timecounter.fast_gettime=1. I figure, on my PVCLOCK_FLAG_TSC_STABLE test systems, (1) should roughly simulate the version where the code is left as-is and (2) should roughly simulate the version where this clause is dropped.

These syscall_timing gettimeofday numbers from (1) to (2) regressed from 116ns -> 146ns = +30ns on my AMD HW and 119ns -> 141ns = +22ns on my Intel HW. So, potentially non-negligible and makes sense to keep this clause in place (or achieve the same outcome in some other way)? What do you think?

Do you really need to use the dma subsystem for this? Isn't it enough to just malloc a region of memory and use it?

I'd decided against malloc() since it doesn't provide API-level alignment guarantees; while, implementation-wise, it does look like >= PAGE_SIZE-ed malloc(9) allocations will be page-aligned, my read of malloc(9) (particularly section IMPLEMENTATION NOTES) is that this should not be assumed by callers.

Right, TBH I think malloc returning and address not aligned to a page boundary when allocating a region >= PAGE_SIZE would likely be a bug in the implementation.

(FWIW, I had also noted (1) the precedent of hyperv's use of BUS_DMA(9) for its corresponding PV clock in-memory data and (2) internally, this draft's use of BUS_DMA(9) will ultimately do the same thing that the first draft was doing with its direct use of the physical memory pager).

Just recapping, the intention is that timeinfos be readily mmap-able---the allocation containing timeinfos should start and end on a page boundary (with any unused portion of the last page zeroed).

Does this choice make sense or am I missing something?

Well, I think it's overly complicated (and cumbersome due to all the initialization involved) to use bus_dma just to allocate a memory region that's page aligned. I'm sure there are already instances in-kernel that assume that malloc(PAGE_SIZE) returns a page. I wouldn't be opposed to using:

sc->timeinfos = malloc(size, ...);
if ((sc->timeinfos & PAGE_MASK) != 0) {
     device_printf(..., "failed to allocate aligned memory region");
    KASSERT(0, ("Failed to allocate page aligned region"));
    return (ENOMEM);
}

But I understand others might have different opinions. Maybe @kib could provide some insight on whether there's another way to allocate aligned memory regions without having to resort to bus_dma, I don't have that much insight on the VM subsystem.

You could then also use M_WAITOK maybe?

This choice was considering use cases where the driver is loaded during boot (whether compiled in or via loader.conf(5)). My thinking was that a failure to satisfy a relatively small allocation like this during boot would be an indication of far greater problems and, as such, it wouldn't be worth adding any blocking+reclaims+retries to the boot process, especially since the system can still function without kvmclock.

Likely, if we fail to satisfy such small allocation it's likely the kernel won't be able to finish booting anyway. Adding a comment as to why BUS_DMA_NOWAIT is used (ie: not because the context prevents using BUS_DMA_WAITOK) would be OK also.

But I suppose it's also ok to allow the reclaims+retries, so, I've switched bus_dmamem_alloc() from BUS_DMA_NOWAIT to BUS_DMA_WAITOK in this version as per this suggestion.

While malloc(9) can't give these guarantees, uma(8) can. Creating the tag, allocating and loading it are basically creating a uma zone behind the scenes and using them directly would be better than using busdma just to get alignment.

We have malloc_domainset_aligned(), added exactly to export the alignment guarantees from the malloc subsystem.

Thanks! That's exactly what's needed here. I think this function has not been added to the malloc(9) man page?

Would it make sense to also add some syntactic sugar and introduce a:

#define malloc_aligned(s, a, t, f) malloc_domainset_aligned(s, a, t, DOMAINSET_RR(), f)

Or similar.

Of course, feel free to add. There were no users for it until now.

I do not quite follow this. Why do you need to set pvclock_vcpu_info to MAP_FAILED in advance, during init?

Basically, this introduces somewhat unpleasant and very hard to diagnose case, where two threads happen to execute e.g. gettimeofday(), and then one of them falls back to syscall. Unpleasant part is that on the next gettimeofday() call this thread would use vdso path, and kernel vs. usermode timecounters might be relatively off.

I tried to avoid this with HPET. Also note use of acq/rel atomics to not rely implicitly on x86 TSO.

This blank line is excessive.

Excessive blank line.

And this one. It would be too much spam to comment about all of them.

Why do you need newbus attachment for this thing? Due to the clock kobj interface? But is it required?

Please do not use these linuxisms.

I suspect you need load_acq(&wc->version) instead of these two lines ...

.. and then atomic_thread_fence_acq() instead of this rmb(). But where is the writer?

The writer is the hypervisor.

hm...

Yeah, this was assuming that it's always "ok"---in that a thread will never see a syscall-based time reading that is less than a previous syscall- or vDSO-based time reading---for a thread to fall back to the syscall path and, based on that assumption, was deliberately allowing the described race in exchange for simplified init code.

If it can't be assumed to be safe to switch back and forth between the vDSO and syscall codepaths, then maybe we can't currently support the vDSO codepath for this clock source as the timekeeping code presently stands. (I'll think about this more).

But doesn't the hyperv PV TSC vDSO codepath make this same assumption, since a thread can end up switching between the vDSO and syscall codepaths at any time based on whether TscSequence == 0 (hyperv_ref_tsc->tsc_seq == 0)?

I want to make sure I have a correct and detailed understanding of the specific vDSO/syscall mismatch problem(s) you're referencing. Would you be able to provide example(s) of the specific way(s) that syscall-based and vDSO-based time values can differ that are being referenced here?

I think I can see a case where two calls to clock_gettime() for CLOCK_MONOTONIC_FAST or CLOCK_UPTIME_FAST---which only use th_offset without adding the subsequent delta---could lead to the second call's value being less than the first. This would happen if a thread managed to make a syscall-based reading followed by a vDSO-based reading that both occurred after the in-kernel timehands update but before the corresponding vDSO-based timehands update.

Is this an example of the sorts of cases you're thinking of?

Am I understanding correctly if I'm thinking that this same scenario but for CLOCK_{MONOTONIC,UPTIME}{,_PRECISE}---which use th_offset plus a th_offset_count-relative delta---would not have this problem because the th_offset_count-based delta that would be included in each reading would be relative to its respective th_offset value?

Right, all of this is as per the clock/RTC subsystem (sys/clock.h, kern/subr_rtc.c), which does use the clock kobj interface's entry points.

We do not indent function definitions in macros typically. Please remove these 4 spaces.

There is absolutely no reason to defined this macro. Both kernel and userspace variants are used once, so you could just put the corresponding code directly into libc and kernel, and avoid obfuscation through the macro.

In fact, I think that the better approach is to avoid macros at all. Put the code you want to share (as I understand, this is DEFINE_RDTSC_ORDERED_COMMON) into e.g. sys/x86/include/rdtsc_fenced.h directly, and include it into libc "vdso" and kernel rdtsc.h

Should this case be assert? Is it possible that _CLOCKSOURCE feature disappears?

malloc() for PAGE_SIZE and larger is a strange choice. Why not use kmem_malloc()?

For instance, ldt allocation on x86 uses kmem_malloc(), and it is quite similar to kvm clock case.

I see that kmem_malloc(), internally, always returns page-aligned allocations, but does it formally guarantee this condition at the API level like malloc_{,domainset_}aligned(9) do? There also seem to be so few uses of it relative to malloc(9), especially among drivers---should we maintain that precedent here?

Yes, the kmem_malloc() KPI fundamental property is that it operates on the page granularity. As I said, the intent there (kvmclock) matches the KPI purpose perfectly. It is one-off allocation from the platform-specific driver, which is tightly integrated with the rest of the system.

Ok, sounds good, then. Thanks for the clarification!