Request for review (XS): 8006431: os::Bsd::initialize_system_info() sets _physical_memory too large

[David Holmes]

Hi Bengt,

This looks fine to me.

Though examining this does highlight some issues with the way 
os::physical_memory is being used - and in particular here how the BSD 
case restricts it to the maximum data segment size!

David

On 21/01/2013 4:48 PM, Bengt Rutisson wrote:
>
> Hi Dean,
>
> Thanks for looking at this!
>
> On 1/19/13 6:51 AM, Dean Long wrote:
>> How about checking that the returned "len" is as expected:
>>
>> if (sysctl(mib, 2, &mem_val, &len, NULL, 0) != -1) {
>> assert(len == sizeof(mem_val), "oops");
>> _physical_memory = mem_val;
>>
>> which would have detected the problem with HW_USERMEM.
>
> Good idea. I added the assert. I also added it to the only other call to
> sysctl in the hotspot source. That call is in the same method.
>
> I also tried reverting to HW_USERMEM and the assert catches the issue
> with that key. We get 4 instead of 8 in len.
>
> Updated webrev:
> http://cr.openjdk.java.net/~brutisso/8006431/webrev.02/
>
> Thanks,
> Bengt
>
>>
>> dl
>>
>> On 1/18/2013 6:31 AM, Bengt Rutisson wrote:
>>>
>>> Hi Vitaly,
>>>
>>> Thanks for looking at this!
>>>
>>> I updated the comment. Here is an updated webrev:
>>> http://cr.openjdk.java.net/~brutisso/8006431/webrev.01/
>>>
>>> On 1/18/13 3:11 PM, Vitaly Davidovich wrote:
>>>>
>>>> Also, is it worthwhile to initialize mem_value to 0 explicitly
>>>> before passing it to the syscall? That should avoid garbage in the
>>>> upper 32 bits; I realize it's not needed now but maybe for
>>>> completeness sake?
>>>>
>>>
>>> HW_MEMSIZE is explictly documented to be a 64 bit value. I don't
>>> think it should be necessary to initialize mem_value to 0 now.
>>>
>>> Thanks again for the review!
>>> Bengt
>>>
>>>
>>>> Sent from my phone
>>>>
>>>> On Jan 18, 2013 8:57 AM, "Vitaly Davidovich" <vitalyd at gmail.com
>>>> <mailto:vitalyd at gmail.com>> wrote:
>>>>
>>>>     Hi Bengt,
>>>>
>>>>     The comment there needs to be updated because it's still talking
>>>>     about usermem.
>>>>
>>>>     Thanks
>>>>
>>>>     Sent from my phone
>>>>
>>>>     On Jan 18, 2013 7:48 AM, "Bengt Rutisson"
>>>>     <bengt.rutisson at oracle.com <mailto:bengt.rutisson at oracle.com>>
>>>>     wrote:
>>>>
>>>>
>>>>
>>>>         Hi Mikael,
>>>>
>>>>         Thanks for the review!
>>>>
>>>>         On 1/18/13 8:09 AM, Mikael Vidstedt wrote:
>>>>>
>>>>>         Looks good, assuming u_long is a 64-bit type.
>>>>
>>>>         Good point. It seems like u_long is a 64 bit value on OSX,
>>>>         but that's not guaranteed since it is just "unsigned long".
>>>>         I changed mem_val to be julong, which should always be 64
>>>>         bit. (The instance variable _physical_memory is also a julong.)
>>>>
>>>>         So, now it is a two-word review request :)
>>>>
>>>>         diff --git a/src/os/bsd/vm/os_bsd.cpp b/src/os/bsd/vm/os_bsd.cpp
>>>>         --- a/src/os/bsd/vm/os_bsd.cpp
>>>>         +++ b/src/os/bsd/vm/os_bsd.cpp
>>>>         @@ -243,7 +243,7 @@
>>>>         int mib[2];
>>>>         size_t len;
>>>>         int cpu_val;
>>>>         - u_long mem_val;
>>>>         + julong mem_val;
>>>>
>>>>         /* get processors count via hw.ncpus sysctl */
>>>>         mib[0] = CTL_HW;
>>>>         @@ -260,7 +260,7 @@
>>>>         * instead of hw.physmem because we need size of allocatable
>>>>         memory
>>>>         */
>>>>         mib[0] = CTL_HW;
>>>>         - mib[1] = HW_USERMEM;
>>>>         + mib[1] = HW_MEMSIZE;
>>>>         len = sizeof(mem_val);
>>>>         if (sysctl(mib, 2, &mem_val, &len, NULL, 0) != -1)
>>>>         _physical_memory = mem_val;
>>>>
>>>>
>>>>         Thanks,
>>>>         Bengt
>>>>
>>>>
>>>>>
>>>>>         /Mikael
>>>>>
>>>>>         On 17 jan 2013, at 22:36, Bengt Rutisson
>>>>>         <bengt.rutisson at oracle.com
>>>>>         <mailto:bengt.rutisson at oracle.com>> wrote:
>>>>>
>>>>>>
>>>>>>         Hi all,
>>>>>>
>>>>>>         Could I have a couple of reviews for this small fix?
>>>>>>
>>>>>>         http://cr.openjdk.java.net/~brutisso/8006431/webrev.00/
>>>>>>         <http://cr.openjdk.java.net/%7Ebrutisso/8006431/webrev.00/>
>>>>>>
>>>>>>         On OSX we used HW_USERMEM value from sysctl() to get the
>>>>>>         available physical memory on the machine. This is a 32 bit
>>>>>>         value but we store it in a 64 bit variable. This means
>>>>>>         that we get kind of random and normally very large values
>>>>>>         for what we think the physical memory is.
>>>>>>
>>>>>>         We actually don't want a 32 bit value since we want to
>>>>>>         handle machines with more than 2 or 4 GB of memory.
>>>>>>         Instead of HW_USERMEM we should be using HW_MEMSIZE which
>>>>>>         will give us a 64 bit value.
>>>>>>
>>>>>>         See:
>>>>>>         https://developer.apple.com/library/mac/#documentation/Darwin/Reference/ManPages/man3/sysctl.3.html
>>>>>>
>>>>>>         Thanks Staffan Larsen for both detecting the problem and
>>>>>>         providing a solution.
>>>>>>
>>>>>>         This is a one-word-change. So, to save you a mouse click
>>>>>>         on the webrev link above, here is the diff:
>>>>>>
>>>>>>         --- a/src/os/bsd/vm/os_bsd.cpp
>>>>>>         +++ b/src/os/bsd/vm/os_bsd.cpp
>>>>>>         @@ -260,7 +260,7 @@
>>>>>>         * instead of hw.physmem because we need size of
>>>>>>         allocatable memory
>>>>>>         */
>>>>>>         mib[0] = CTL_HW;
>>>>>>         - mib[1] = HW_USERMEM;
>>>>>>         + mib[1] = HW_MEMSIZE;
>>>>>>         len = sizeof(mem_val);
>>>>>>         if (sysctl(mib, 2, &mem_val, &len, NULL, 0) != -1)
>>>>>>         _physical_memory = mem_val;
>>>>>>
>>>>>>         Thanks,
>>>>>>         Bengt
>>>>
>>>
>