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

[Bengt Rutisson]

Thanks Staffan, Mikael, David, Vitaly and Dean for the reviews!

All set to push this now.

Bengt

On 1/21/13 8:17 AM, David Holmes wrote:
> 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
>>>>>
>>>>
>>