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

[Vitaly Davidovich]

Looks good.

Yeah, zeroing is just future-proofing against same issue if someone
mistakenly changes to use a new sysctl returning 32 bit value.  It's
paranoia only though :)

Thanks

Sent from my phone
On Jan 18, 2013 9:31 AM, "Bengt Rutisson" <bengt.rutisson at oracle.com> 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> 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>
>> 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>
>>> 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/4b89b8de/attachment-0001.html