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

[Bengt Rutisson]

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/
>>
>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130118/14596719/attachment.html