Add mfence before issuing rdtsc

[itzmeanjan]

Given the fact that a x86 CPU doesn't guarantee when it'll actually issue RDTSC instruction, if used in isolation, we must use it with a memory fence instruction such as MFENCE so that all memory load/ store operations, which appear before RDTSC instruction, in the program order (i.e. source code or compiler generated assembly), are globally visible.

It's necessary to use a memory fence either lfence or mfence to enforce ordering constraints on CPU issuing rdtsc instruction.

See

• RDTSC without any ordering guarantees @ https://github.com/torvalds/linux/blob/9f8413c4a66f2fb776d3dc3c9ed20bf435eb305e/arch/x86/include/asm/msr.h#L171-L187 (currently being used in this library)
• RDTSC with some ordering guarantees https://github.com/torvalds/linux/blob/9f8413c4a66f2fb776d3dc3c9ed20bf435eb305e/arch/x86/include/asm/msr.h#L189-L223 (this is where I'm proposing a transition to, a safer place)

Note, I use mfence (serializes both load and stores) instead of lfence (serializes only loads), just to be safer. It shouldn't hurt.
More about memory fencing @ https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.htm#text=fence&ig_expand=3977,4395,4395.

[oreparaz]

Thanks for this! I'm positive about the change.

Have you witness any difference in behavior? From a quick search I see some slight difference between mfence and lfence, do you think we should see if they cause any difference?

[itzmeanjan]

Thanks for this! I'm positive about the change.

Thanks :)

Have you witness any difference in behavior?

One of the motivation behind submitting this patch is, when using dudect in production environment, we're seeing several intermittent false positive reports. False positives are mostly reported when the machine which is running these tests, is under heavy load. But with this memory fence barrier (mfence) added right before rdtsc, I've not seen any false positive reports yet.

From a quick search I see some slight difference between mfence and lfence, do you think we should see if they cause any difference?

I don't think using mfence would hurt anyway. I'll just stick to mfence because it should ensure both load and store operations appearing before (in program order) that fence must be serialized.

[itzmeanjan]

We're getting some CI failures @ https://github.com/oreparaz/dudect/actions/runs/7466176204/job/20402300482, but they are failing due to different reasons.

[oreparaz]

[...] with this memory fence barrier (mfence) added right before rdtsc, I've not seen any false positive reports yet.

Nice to see that by introducing mfence you're getting less false positives.

I'll just stick to mfence because it should ensure both load and store operations appearing before (in program order) that fence must be serialized

Let's keep the mfence if that works OK for you. I'm reading a bit and there's some indication that mfence may not actually serialize the instruction stream [1] but someone else says that it does in Skylake [2]. I'll open an issue in case someone wants to do some more research here.

[1] https://www.felixcloutier.com/x86/mfence : "MFENCE does not serialize the instruction stream".
[2] https://hadibrais.wordpress.com/2018/05/14/the-significance-of-the-x86-lfence-instruction/#comment-310 "MFENCE also serializes the instruction stream on Skylake the same way LFENCE does. It doesn’t guarantee that on paper, though"

We're getting some CI failures

Fixing in #31 . I'll rebase this, add a bit of documentation and merge. Thanks!

[oreparaz]

See #32 for more comments on this.

[oreparaz]

Thanks! Merging...

[itzmeanjan]

Let's keep the mfence if that works OK for you. I'm reading a bit and there's some indication that mfence may not actually serialize the instruction stream [1] but someone else says that it does in Skylake [2]. I'll open an issue in case someone wants to do some more research here.

Makes sense, just another resource link from GNU/Linux kernel https://github.com/torvalds/linux/blob/9f8413c4a66f2fb776d3dc3c9ed20bf435eb305e/arch/x86/include/asm/msr.h#L201-L220.