Hi Yasumasa, thanks a lot for measuring and improving the overhead. This version looks much better.
I still wonder what the reason was for using a stub. There was probably a reason, but I don't remember. Hopefully, Matthias can find time for another review when he's back next week. Thanks and best regards, Martin > -----Original Message----- > From: Yasumasa Suenaga <suen...@oss.nttdata.com> > Sent: Dienstag, 18. August 2020 05:03 > To: David Holmes <david.hol...@oracle.com>; Baesken, Matthias > <matthias.baes...@sap.com>; hotspot-runtime-...@openjdk.java.net; > build-dev@openjdk.java.net; Doerr, Martin <martin.do...@sap.com> > Subject: Re: PING: RFR: 8250598: Hyper-V is detected in spite of running on > host OS > > On 2020/08/18 10:42, David Holmes wrote: > > Hi Yasumasa, > > > > On 14/08/2020 12:36 pm, Yasumasa Suenaga wrote: > >> Hi David, Matthias, Martin, > >> > >> I uploaded new webrev. Could you review again? > >> > >> http://cr.openjdk.java.net/~ysuenaga/JDK-8250598/webrev.02/ > >> > >> It generates runtime stub for hypervisor detection, and it can distinguish > between Hyper-V host (root partition) and guest. And also it works on 32bit > platforms. > >> > >> I measured startup performance (java --version) with Measure-Command > on PowerShell, this change is faster than current implementation. > >> > >> TotalMilliseconds : 57.8196 > >> TotalMilliseconds : 66.8379 > >> TotalMilliseconds : 64.7693 > >> TotalMilliseconds : 55.6546 > >> TotalMilliseconds : 63.848 > >> average : 61.78588 > > > > This seems good to me, but again I can't review the details. My main > concern is how you are testing this across a range of virtualized hosts? But > if it > fixes your Hyper-V issue and doesn't break anything else then I am fine with > it. > > Thanks David! > > I don't have any other virtualized hosts excepting Hyper-V, so I cannot test > any more. > However detection logic is equivalent from current implementation, so I > believe it does not break anything else if it works fine on Hyper-V. > > And also Matthias has queued this change to internal tests in SAP. I guess he > will share the result next week. > > > > I will leave the technical review to Martin and Matthias. > > > > FYI I'm heading out on a weeks vacation after today so won't be able to > follow up further. > > Have a nice vacation! > > > Thanks, > > Yasumasa > > > > Thanks, > > David > > > >> This change has passed tests on submit repo (mach5-one-ysuenaga-JDK- > 8250598-20200814-0119-13424118), and also it works fine on following > environments: > >> > >> - Hyper-V host (Windows 10 x64) > >> - Hyper-V guest (Windows 10 x64) > >> - Hyper-V guest (Linux x64) > >> - Hyper-V guest (32bit JDK on Linux x64) > >> > >> > >> Thanks, > >> > >> Yasumasa > >> > >> > >> On 2020/08/13 20:39, David Holmes wrote: > >>> On 13/08/2020 5:39 pm, Yasumasa Suenaga wrote: > >>>> Hi Matthias, David, > >>>> > >>>> On 2020/08/13 15:52, Baesken, Matthias wrote: > >>>>> > >>>>>> > >>>>>> Should we make the change to determine just before it is needed > (e.g. VM.info or hs_err log) at first? > >>>>>> It is out of scope of this change, so I want to work as another issue > >>>>>> if > it is needed. > >>>>>> > >>>>> > >>>>> Hi , > >>>>> I think we better do not call into WMI , when writing an hs_err file > >>>>> . > >>>> > >>>> I found out from Hypervisor Top Level Functional Specification [1] > section 2.4.8. > >>>> That says CPUID leaf 0x40000007 is available to the root partition only. > >>>> > >>>> I changed VM_Version::is_in_VM() as below, and it works fine. > >>>> (I use __cpuid intrinsic in here because this code would be compiled by > cl.exe only.) > >>>> > >>>> ``` > >>>> #include <intrin.h> > >>>> > >>>> #define EAX 0 > >>>> #define EBX 1 > >>>> #define ECX 2 > >>>> #define EDX 3 > >>>> > >>>> bool VM_Version::is_in_VM() { > >>>> int regs[4]; > >>>> __cpuid(regs, 0x40000007); > >>>> > >>>> // CPUID leaf 0x40000007 is available to the root partition only. > >>>> return (regs[EAX] == 0x0) && (regs[EBX] == 0x0) && (regs[ECX] == > 0x0) && (regs[EDX] == 0x0); > >>>> } > >>>> ``` > >>>> > >>>> Also startup performance is better than WMI call. > >>>> > >>>> TotalMilliseconds : 66.7863 > >>>> TotalMilliseconds : 56.8123 > >>>> TotalMilliseconds : 57.0015 > >>>> TotalMilliseconds : 62.4755 > >>>> TotalMilliseconds : 67.7632 > >>>> average : 62.16776 > >>>> > >>>> If you are ok this change, I will update webrev, and will send review > request. > >>>> (new webrev will include both loop for CPUID leaf and renaming > is_in_VM) > >>> > >>> If it works and addresses the startup hit then I'm fine with it - but > >>> still > can't actually review the code. > >>> > >>> Thanks, > >>> David > >>> > >>>> > >>>> Thanks, > >>>> > >>>> Yasumasa > >>>> > >>>> > >>>> [1] https://github.com/MicrosoftDocs/Virtualization- > Documentation/raw/master/tlfs/Hypervisor%20Top%20Level%20Functional > %20Specification%20v6.0b.pdf > >>>> > >>>> > >>>>> Best regards, Matthias > >>>>> > >>>>> > >>>>> > >>>>> -----Original Message----- > >>>>> From: Yasumasa Suenaga <suen...@oss.nttdata.com> > >>>>> Sent: Donnerstag, 13. August 2020 06:15 > >>>>> To: David Holmes <david.hol...@oracle.com>; Baesken, Matthias > <matthias.baes...@sap.com>; hotspot-runtime-...@openjdk.java.net; > build-dev@openjdk.java.net > >>>>> Cc: Doerr, Martin <martin.do...@sap.com> > >>>>> Subject: Re: PING: RFR: 8250598: Hyper-V is detected in spite of > running on host OS > >>>>> > >>>>> On 2020/08/13 11:54, David Holmes wrote: > >>>>>> On 13/08/2020 11:12 am, Yasumasa Suenaga wrote: > >>>>>>> Hi Matthias, David, > >>>>>>> > >>>>>>> I measured startup benchmarks with `Measure-Command > {.\jdk\build\windows-x86_64-server-release\images\jdk\bin\java.exe -- > version}` on PowerShell. > >>>>>>> > >>>>>>> * PC: Ryzen 3 3300X, 16GB memory > >>>>>>> * OS: Windows 10 x64 (May 2020 Update) > >>>>>>> * Java: jdk/jdk revision 60537 > >>>>>>> (Compiled by VS 2019 (16.7.0)) > >>>>>>> > >>>>>>> * without patch > >>>>>>> TotalMilliseconds : 70.2124 > >>>>>>> TotalMilliseconds : 64.4465 > >>>>>>> TotalMilliseconds : 59.0854 > >>>>>>> TotalMilliseconds : 68.0255 > >>>>>>> TotalMilliseconds : 72.6467 > >>>>>>> average : 66.8833 > >>>>>>> > >>>>>>> * with webrev.01 > >>>>>>> TotalMilliseconds : 81.7185 > >>>>>>> TotalMilliseconds : 68.539 > >>>>>>> TotalMilliseconds : 85.7226 > >>>>>>> TotalMilliseconds : 72.6584 > >>>>>>> TotalMilliseconds : 75.6091 > >>>>>>> average : 76.84952 > >>>>>>> > >>>>>>> Overhead of WMI seems to be +10ms in this case. > >>>>>> > >>>>>> Which is nearly 15%! Sorry but I just know Claes will be very unhappy > if this were to go in as-is. > >>>>> > >>>>> Should we make the change to determine just before it is needed > (e.g. VM.info or hs_err log) at first? > >>>>> It is out of scope of this change, so I want to work as another issue > >>>>> if it > is needed. > >>>>> > >>>>> > >>>>> Yasumasa > >>>>> > >>>>> > >>>>>> David > >>>>>> ----- > >>>>>> > >>>>>>> > >>>>>>> Yasumasa > >>>>>>> > >>>>>>> > >>>>>>> On 2020/08/13 0:05, Baesken, Matthias wrote: > >>>>>>>>> I understand that if the process runs on Xen on other hypervisor > (e.g. KVM), information for Xen would be set between 0x40000100 and > 0x40010000. > >>>>>>>>> Ok, I will not remove the loop in new webrev, and will add > comment about it. > >>>>>>>> > >>>>>>>> Hi Yasumasa , thanks ! > >>>>>>>> > >>>>>>>> Regarding the WMI overhead , if you could get some more info > on this it would be better to judge if it is perfectly okay or concerning . > >>>>>>>> > >>>>>>>> (I remember that I looked into it a couple of years ago , but > decided not to use it in early VM startup ; but have to confess this was just > based on a "gut feeling", > >>>>>>>> No micro benchmarks ) > >>>>>>>> > >>>>>>>> Best regards, Matthias > >>>>>>>> > >>>>>>>>
