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

[Bengt Rutisson]

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
>

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