RFR: 8197408: Bad pointer comparison and small cleanup in os_linux.cpp
Bob Vandette
bob.vandette at oracle.com
Tue Feb 20 15:13:24 UTC 2018
> On Feb 19, 2018, at 8:03 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
>
> Hi Bob,
>
> On 02/15/2018 04:50 PM, Bob Vandette wrote:
>> The changes look ok to me.
>
> Thanks!
>
>> I would have preferred that you kept the original logic embedded in the
>> higher level functions rather than adding additional static functions. We’re hoping
>> to back port this capability to JDK 8 and it just makes the merges more error prone.
>
> Here I moved the logic back:
>
> http://cr.openjdk.java.net/~rehn/8197408/v3/inc/
> http://cr.openjdk.java.net/~rehn/8197408/v3/full/
>
> I prefer the v2 version of os_linux.cpp, stick with v3 or go back to v2?
I prefer v3 although I still see changes that are unnecessary even in that changeset.
My implementation of os::Linux::available_memory() is more efficient since I’m only testing
memory limit and usage once. The compiler may optimize this but there is no need to check
usage if the limit cannot be read or is unlimited. Also, memory_usage_in_bytes will never return unlimited.
Can’t you just update my log_debug lines to provide the additional information?
In print_container_info, if you are going to eliminate the multiple if/else entries for some of
the output, why didn’t you also do that for active_processor_count?
Bob.
>
> I also added a test which just verifies we can read the cgroup's values without failing when in a container. And if not in container verifies that we failed as expected.
>
> Both v2 and v3 passes this test via the in container case.
>
> Thanks, Robbin
>
>> I’m also a bit uncomfortable with freeing potentially NULL pointers in print_container_info.
>> I’ve worked with too many different Linux clibs to trust that they all have the same
>> behavior but it is documented on both Linux and Bsd so I guess we’re ok.
>> Bob.
>>> On Feb 15, 2018, at 4:37 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
>>>
>>> Hi Thomas, thanks for having a look.
>>>
>>> On 2018-02-14 15:06, Thomas Stüfe wrote:
>>>> Hi Robin,
>>>> Had a short look. Note that I cannot open the issue. Link is wrong, and the link in the source.patch is wrong too. So, I just looked at the webrev.
>>>
>>> Sorry bug is:
>>> https://bugs.openjdk.java.net/browse/JDK-8197408
>>>
>>> Inc here:
>>> http://cr.openjdk.java.net/~rehn/8197408/v2/inc/webrev/
>>> Full here:
>>> http://cr.openjdk.java.net/~rehn/8197408/v2/full/webrev/
>>>
>>> Comments on remarks below:
>>>
>>>> Remarks:
>>>> -> This is a matter of taste but I prefer pointers to references for output variables. Makes the intent clearer at the calling site, and that way you could get rid of the /* output */ comment.
>>>
>>> We had a debate about this, my opinion was that avoiding the NULL check makes it worth using a ref, but I see I'm pretty lonely in this camp so fixed.
>>>
>>>> -> in available_memory_container():
>>>> Could we also add "using host value" to the logging in error case as you do in physical_memory_container()?
>>>
>>> Fixed, and tried to improve the logs.
>>>
>>>> -> (Not part of your patch) Can OSContainer::memory_limit_in_bytes() actually ever return "OSCONTAINER_ERROR"? I may be wrong here but:
>>>> OSCONTAINER_ERROR = -2. /memory.limit_in_bytes gets returned as julong, so we have (julong)(-2). That gets compared with julong _unlimited_memory which is basically LONG_MAX, so signed long max, which should be smaller than (julong)(-2), or? So OSContainer::memory_limit_in_bytes() should always return -1 for both errors and the unlimited case.
>>>
>>> Manually tested this, it seems to work. The -2 gets promoted to unsigned in comparison.
>>> We go from (jlong)-2 to (julong)ULONG_MAX-1 back to (jlong)-2.
>>> Not obviously that it will always work. I will not touch that in this changeset.
>>>
>>>> -> Can we:
>>>> - st->print("container_type: %s\n", p != NULL ? p : "failed");
>>>> + st->print_cr("container_type: %s", p != NULL ? p : "failed");
>>>> ?
>>>
>>> All of these use print + \n, you want me to just change this one or ?
>>> I'll rather leave them alone.
>>>
>>>> -> char * OSContainer::container_type() - you free() that value but it is not strdup()ed.
>>>
>>> Thanks for seeing this, copy-paste :)
>>>
>>>> (Btw I do not like that some functions return strduped values, some do not. If OSContainer::container_type() wants to return a static string, its return type should be at least const char*, not char*. They even added an explicit cast to nonconst char* .)
>>>
>>> Changed to const.
>>>
>>> Thanks, Robbin
>>>
>>>> Kind Regards, Thomas
>>>> On Wed, Feb 14, 2018 at 2:18 PM, Robbin Ehn <robbin.ehn at oracle.com <mailto:robbin.ehn at oracle.com>> wrote:
>>>> Ping!
>>>> /Robbin
>>>> On 2018-02-08 13:35, Robbin Ehn wrote:
>>>> Hi David,
>>>> On 02/08/2018 01:19 PM, David Holmes wrote:
>>>> On 8/02/2018 10:08 PM, Robbin Ehn wrote:
>>>> Hi David,
>>>> On 02/08/2018 12:43 PM, David Holmes wrote:
>>>> Hi Robbin,
>>>> On 8/02/2018 7:03 PM, Robbin Ehn wrote:
>>>> Hi all,
>>>> There is a bad pointer comparison in os_linux.cpp while looking at that
>>>> You seem to be missing the fact that OSContainer::cpu_cpuset_memory_nodes() can return a pointer or an error code.
>>>> This is not true for macro:
>>>> GET_CONTAINER_INFO_CPTR
>>>> As far I can see?
>>>> Sorry - you're right. I misread current code and misremembered what happened at the initial code review - where I'm sure this "pointer versus error code" issue was also flagged.
>>>> It returns:
>>>> if (err != 0)
>>>> return (return_type) NULL;
>>>> or:
>>>> return os::strdup(mems);
>>>> If you know a method that returns an integer in a char*, it's broken should be fixed.
>>>> I saw some if statement were missing bracket, a lot of extra scopes and complexity in the scoping.
>>>> You'd better check all this with Bob Vandette as its his container support code.
>>>> Not sure what you mean. It passes container tests.
>>>> In what environment did you run the container tests? Most of the code you've been refactoring deals with various errors and misconfigurations that can occur.
>>>> I'm sure Bob will want a chance to check the refactoring still does as he intended.
>>>> On mach5 all platforms with the container test (+ hotspot_tier1) and locally.
>>>> I said before, regarding containers, all Linux have cgroups configured so this logic always thinks we are in a container and does this logging and calculation.
>>>> But no there is no tests for miss-configuration that I found.
>>>> Added Bob!
>>>> Thanks, Robbin
>>>> Cheers,
>>>> David
>>>> -----
>>>> Thanks, Robbin
>>>> Cheers,
>>>> David
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~rehn/8197408/webrev/ <http://cr.openjdk.java.net/~rehn/8197408/webrev/>
>>>> Bug:
>>>> http://cr.openjdk.java.net/~rehn/8197408/webrev/ <http://cr.openjdk.java.net/~rehn/8197408/webrev/>
>>>> Thanks, Robbin
More information about the hotspot-runtime-dev
mailing list