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

[Bengt Rutisson]

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

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