RFR: 8219241: Provide basic virtualization related info in the hs_error file on linux/windows x86_64

David Holmes david.holmes at oracle.com
Fri Mar 29 00:15:48 UTC 2019 

Hi Matthias,

I have two issues with this API:

1. Why do we have/need os::<os>::print_virtualization_info when we also 
have VM_Version::print_platform_virtualization_info?

2. I don't like the fact that the there are two ways to define the 
platform specific information:
   a) override VM_Version::print_platform_virtualization_info; or
   b) hook into the get_detected_virtualization switch

And IIUC code that uses (a) relies on the switch doing nothing 
(NoVirtualization) and code that uses (b) relies on 
print_platform_virtualization_info doing nothing!

Why doesn't the default implementation of 
VM_Version::print_platform_virtualization_info define the 
get_detected_virtualization() switch logic, and do away with 
Abstract_VM_Version::print_detected_virtualization? Code that wants to 
print this info can just call 
VM_Version::print_platform_virtualization_info().

Sorry but I just find the current proposal has too many methods and an 
unclear structure.

David
-----

On 29/03/2019 2:12 am, Baesken, Matthias wrote:
>>
>> which calls
>>
>> VM_Version::print_platform_virtualization_info
>>
>> which you have defined for s390 and PPC, but I don't see it for other
>> architectures?
> 
> 
> Hi David ,  we have   in   class Abstract_VM_Version
> 
> static void print_detected_virtualization(outputStream*);
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8219241.3/src/hotspot/share/runtime/vm_version.hpp.frames.html
> 
> 
> ( same approach that is used for other  Abstract_VM_Version     methods  like
> 
>              .....  Platforms that
>    83   // need to specialize this define VM_Version::early_initialize().
>    84   static void early_initialize() { }
> 
> )
> 
>>
>> Are the other platforms handled by the switch on
>> VM_Version::get_detected_virtualization() ? Why isn't that encapsulated
>> in print_platform_virtualization_info for those platforms (which I think
>> is only x86 at this stage?).
>>
> 
> Yes  (and true,  it is  x86 only because  all the other platforms  have    _detected_virtualization = NoDetectedVirtualization;    ).
> 
> 
> 
> Best regards , Matthias
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Donnerstag, 28. März 2019 08:21
>> To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
>> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
>> Cc: Doerr, Martin <martin.doerr at sap.com>
>> Subject: Re: RFR: 8219241: Provide basic virtualization related info in the
>> hs_error file on linux/windows x86_64
>>
>> On 26/03/2019 6:32 pm, Baesken, Matthias wrote:
>>> Hi David,
>>>
>>>> This is looking better though I'm struggling to keep all the dots
>>>> connected in terms of the APIs - I can't quite see the pattern for what
>>>> calls what and what overrides what.
>>>
>>> I   just  followed  here what is done  at  other places  of    VM_Version  /
>> Abstract_VM_Version   , see
>>>     for example  :
>>
>> Sorry but I'm having trouble following all the levels of indirection.
>> You have:
>>
>> VM_version::print_detected_virtualization
>>
>> which calls
>>
>> VM_Version::print_platform_virtualization_info
>>
>> which you have defined for s390 and PPC, but I don't see it for other
>> architectures?
>>
>> Then you have os::Linux and os::Windows print_virtualization_info that
>> then calls VM_version::print_detected_virtualization
>>
>> Are the other platforms handled by the switch on
>> VM_Version::get_detected_virtualization() ? Why isn't that encapsulated
>> in print_platform_virtualization_info for those platforms (which I think
>> is only x86 at this stage?).
>>
>> I'm finding it extremely hard to see the clear shape of this API - sorry.
>>
>> David
>> -----
>>
>>> src/hotspot/share/runtime/vm_version.hpp
>>>
>>>     82   //  .....    Platforms that
>>>     83   // need to specialize this define VM_Version::early_initialize().
>>>     84   static void early_initialize() { }
>>>     85
>>>     86   // Called to initialize VM variables needing initialization
>>>     87   // after command line parsing. Platforms that need to specialize
>>>     88   // this should define VM_Version::init_before_ergo().
>>>     89   static void init_before_ergo() {}
>>>
>>>
>>>> 2. os_windows.cpp
>>>>
>>>> Why the indirection through os::win32::print_virtualization_info instead
>>>> of just calling VM_Version::print_detected_virtualization directly?
>>>
>>> Yes that's a good point - I think I  should change this .
>>>
>>> Best regards, Matthias
>>>
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: David Holmes <david.holmes at oracle.com>
>>>> Sent: Mittwoch, 20. März 2019 07:15
>>>> To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
>>>> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
>>>> Cc: Doerr, Martin <martin.doerr at sap.com>
>>>> Subject: Re: RFR: 8219241: Provide basic virtualization related info in the
>>>> hs_error file on linux/windows x86_64
>>>>
>>>> Hi Matthias,
>>>>
>>>> On 19/03/2019 11:53 pm, Baesken, Matthias wrote:
>>>>> Hello, here is a new  webrev :
>>>>>
>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8219241.3/
>>>>>
>>>>> This moved  the ppc/s390  virtualization related info output into  the
>>>> platform/cpu  - specific    coding .
>>>>
>>>> This is looking better though I'm struggling to keep all the dots
>>>> connected in terms of the APIs - I can't quite see the pattern for what
>>>> calls what and what overrides what. A couple of initial comments:
>>>>
>>>> 1. We already have a ton of cpuid related parsing in
>>>> ./cpu/x86/vm_version_x86.hpp - can't you hook into that to "check
>>>> virtualizations" and store the information for later use?
>>>>
>>>> 2. os_windows.cpp
>>>>
>>>> Why the indirection through os::win32::print_virtualization_info instead
>>>> of just calling VM_Version::print_detected_virtualization directly?
>>>>
>>>> I'll keep trying to piece this together. :)
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> Best regards, Matthias
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: David Holmes <david.holmes at oracle.com>
>>>>>> Sent: Montag, 11. März 2019 08:36
>>>>>> To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
>>>>>> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
>>>>>> Cc: Doerr, Martin <martin.doerr at sap.com>
>>>>>> Subject: Re: RFR: 8219241: Provide basic virtualization related info in the
>>>>>> hs_error file on linux/windows x86_64
>>>>>>
>>>>>> On 7/03/2019 3:13 am, Baesken, Matthias wrote:
>>>>>>> Hello ,  could I push the  latest version of   8219241  ?
>>>>>>
>>>>>> Sorry based on your earlier email I thought you were looking at doing
>>>>>> the restructuring. I was then on vacation most of last week.
>>>>>>
>>>>>>> A  change that moves the coding more into  platform/cpu  - specific
>>>> coding
>>>>>> (if this is wanted)
>>>>>>>      could be  done when bringing in  the AIX  virtualization related info
>> in
>>>>>> another patch  which I plan to do .
>>>>>>
>>>>>> If that is imminent then okay - I don't like the code as it is.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>

More information about the hotspot-dev mailing list