RFR: 8197408: Bad pointer comparison and small cleanup in os_linux.cpp
Robbin Ehn
robbin.ehn at oracle.com
Tue Feb 20 16:44:02 UTC 2018
Hi Bob, thanks for having another look.
On 2018-02-20 16:13, Bob Vandette wrote:
>
>> 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?
Good catch on the memory_usage_in_bytes it snuck in v1->v2.
diff -r 98ee8add2b00 src/hotspot/os/linux/os_linux.cpp
--- a/src/hotspot/os/linux/os_linux.cpp Mon Feb 19 10:24:37 2018 +0100
+++ b/src/hotspot/os/linux/os_linux.cpp Tue Feb 20 17:29:56 2018 +0100
@@ -185,2 +185,1 @@
- log_debug(os, container)("container memory usage %s: " JLONG_FORMAT ", using host value",
- mem_usage == OSCONTAINER_ERROR ? "failed" : "unlimited", mem_usage);
+ log_debug(os, container)("container memory usage failed: " JLONG_FORMAT ", using host value", mem_usage);
I don't either like multiple checking of limit, that is why I like v2 with a separate function for container and merging a new function should apply pretty cleanly.
But my both versions is much simpler (as in easier to read), which was the cleanup.
If you are worried about performance we should not use sscanf to read a file just containing a number, sscanf is slow.
But I suggest we stick sscanf to avoid yet another macro.
>
> 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?
active_processor_count only have 2 levels of nesting, while the code I cleanup had 4 levels.
And this is not an attempt to clean the whole file.
Also can you comment on test, do you see any cases that was missed or other issues?
Thanks, Robbin
>
> 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