On Mon, Feb 23, 2026 at 6:02 PM John Naylor <[email protected]> wrote:
>
> On Tue, Feb 24, 2026 at 5:28 AM Andres Freund <[email protected]> wrote:
> > On 2026-02-23 16:24:57 +0100, David Geier wrote:
> > > The code wasn't compiling properly on Windows because __x86_64__ is not
> > > defined in Visual C++. I've changed the code to use
> > >
> > > #if defined(__x86_64__) || defined(_M_X64)
> >
> > Independently of this patchset I wonder if it'd be worth introducing a
> > PG_ARCH_X64 or such, to avoid this kind of thing.
>
> +1
>
> I've already borrowed USE_SSE2 for this meaning in commit b9278871f,
> but that's conflating two different things and I'd actually prefer the
> above, plus one that includes 32-bit as well.
+1, would be good to have a consistent definition for this, I hadn't
realized this differs between platforms. John, do you want to take
care of adding that since you recently added USE_SSE2?
Its worth noting that we've previously analyzed when the TSC is
invariant (i.e. doesn't change frequency across power level changes)
and concluded that it is on basically all x86-64 CPUs - but I don't
think that necessarily holds true on 32-bit x86, rare as it is.
Since we now intend to have the GUC for controlling the timing clock
source this seems less problematic, so maybe we can allow (but not
default to) use on any x86 platform that has CPUID telling us both
RDTSC and RDTSCP are available, whether its 32-bit or 64-bit.
>
> +static bool
> +is_rdtscp_available()
> +{
> + uint32 r[4] = {0, 0, 0, 0};
> +
> +#if defined(HAVE__GET_CPUID)
> + if (!__get_cpuid(0x80000001, &r[0], &r[1], &r[2], &r[3]))
> + return false;
> +#elif defined(HAVE__CPUID)
> + __cpuid(r, 0x80000001);
> +#else
> +#error cpuid instruction not available
> +#endif
> +
> + return (r[3] & (1 << 27)) != 0;
> +}
>
> I'm hoping to centralize CPU-specific checks like the above:
>
> https://www.postgresql.org/message-id/CANWCAZbKQ2im1r4ztcyfqNh_6gaJzyewabExYrkACNvMNyqxog%40mail.gmail.com
>
> is_rdtscp_available() is an easy thing to delegate to my patch, but I
> agree it would be easier if that was abstracted a bit more so that a
> different leaf can be passed each time. The latter could also be used
> to simplify the frequency and hypervisor stuff as well.
Yeah, that makes sense, agreed it'd be nice to centralize the CPU
architecture specific code that utilizes cpuid/etc.
Looking at your v5/0002 over there, that should work well. As you
note, is_rdtscp_available is an easy delegation to your logic - I
think we can probably always fetch the 0x80000001 leaf to check for
RDTSCP presence in the proposed set_x86_features?
For set_tsc_frequency_khz, if we go forward with this route, I think
we should just turn that back into a get_... function (i.e. not
setting the global inside) and have that as an additional exported
method from pg_cpu.h that instr_time.c can consume. Generalizing use
of the __get_cpuid/__cpuid variants use makes sense to me.
I'll get back to the other patch feedback tomorrow.
Thanks,
Lukas
--
Lukas Fittl
