On Thu, Apr 18, 2013 at 05:16:22PM +0300, Alexander Motin wrote: > On 17.04.2013 11:50, Konstantin Belousov wrote: > > On Wed, Apr 17, 2013 at 09:46:15AM +0300, Alexander Motin wrote: > >> On 17.04.2013 03:25, Jim Harris wrote: > >>> > >>> On Tue, Apr 16, 2013 at 3:04 PM, Alexander Motin <m...@freebsd.org > >>> <mailto:m...@freebsd.org>> wrote: > >>> > >>> Hi. > >>> > >>> Recently I've got 6-core/12-thread system on Sandy Bridge-E Core > >>> i7-3930K CPU and was unpleasantly surprised to see that TSCs are not > >>> synchronized there. While all 11 APs were synchronized, BSP was far > >>> behind them. Since it is single-socket system, I don't know any good > >>> reason for such behavior except some BIOS bug. But I've recalled > >>> that somewhere was some discussions about possible TSC > >>> synchronization. I've implemented patch below that allows to adjust > >>> TSC values of BSPs to AP's one on boot using CPU MSRs, hoping that > >>> they should not diverge after that: > >>> http://people.freebsd.org/~__mav/tsc_adj2.patch > >>> <http://people.freebsd.org/~mav/tsc_adj2.patch> > >>> > >>> I don't know very much about all different TSC hardware to predict > >>> when it is safe to enable the functionality, but at least on my > >>> system being enabled via loader tunable it seems working well. > >>> > >>> Comments? > >>> > >>> > >>> You may be remembering this thread on r238755 last year: > >>> > >>> http://lists.freebsd.org/pipermail/svn-src-head/2012-July/038992.html > >>> > >>> This was a bug fix in the TSC synchronization test code though, not > >>> anything for trying to adjust out-of-sync TSCs. > >> > >> I remember that thread, but I think I've seen somebody told somewhere > >> that it could be interesting to implement some MI mechanism. Never mind. > >> > >>> The Intel SDM (volume 3, section 17.13 of March 2013 revision) says > >>> earlier models can only write to lower 32 bits of > >>> IA32_TIME_STAMP_COUNTER, but these models also should not have invariant > >>> TSC so they would never even get to your new routine. So your patch > >>> seems OK for Intel CPUs, at least as a tunable that is disabled by > >>> default. > >> > >> Thanks. > >> > >>> My only concern would be why TSC on the BSP started out-of-sync on your > >>> system. Theoretically, BIOS could adjust TSCs in SMM to try to hide SMI > >>> code execution from the OS, which could then make them out-of-sync > >>> again. Not sure if that's what's happening here, but might be worth a > >>> test putting the TSC test code on a periodic timer to see if they ever > >>> get out of sync again. > >> > >> I did one more interesting observation: on every reboot drift between > >> BSP and APs is growing proportionally to the previous system power-on > >> time. On first boot it is -3878361036 (just above one second), after > >> reboot some minutes later it is -1123454492776 (about 6 minutes), after > >> another reboot it is -1853033521804 (about 10 minutes). > >> > >> Unless my adjustment code would be active, I would guess that AP's TSC > >> is running linearly while BSP's for some reason reset to zero on every > >> reboot. But since I am synchronizing them on each boot, the only > >> possibility for it I see is that there is some other timer(s) / > >> counter(s) not affected by MSR writes that ticks linearly and reloading > >> AP's TSC, but for some reason not reloading BSP's. > > > > For me it sounds as the BIOS bug, indeed. Could you verify the content > > of IA32_TSC_ADJUST on all cores (I believe it is present on E5) ? > > Also, using TSC_ADJUST to correct the skew seems to be preferrable, > > according to the Intel docs. > > IA32_TSC_ADJUST register seems not present there. At least cpucontrol > doesn't want to read it. In Intel docs I also see it mentioned only in > context of future Haswell generation. And I don't see "Standard Extended > Features" line in dmesg. > > > Why do you use cpuid in the assembly sequence ? As I understand, you > > ensure that there is a serialization point, but why do you need it ? > > The idea was to minimize time distance between following MSR read and > write. But may be it is not needed, I am not exactly sure about that magic.
Well, I do not believe that such trick is useful. Patch with removed cpuid and with resync disabled by default (as it is now, AFAIR) would be IMO fine for the commit.
pgpzjLRbnEjZP.pgp
Description: PGP signature
