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

[Dean Long]

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.

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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130118/7ae5822e/attachment-0001.html