RFR: 6515172: Runtime.availableProcessors() ignores Linux taskset command

[Daniel D. Daugherty]

On 1/27/16 8:05 PM, David Holmes wrote:
> On 28/01/2016 3:40 AM, Daniel D. Daugherty wrote:
>> On 1/27/16 12:16 AM, David Holmes wrote:
>>> Okay here is v2:
>>>
>>> http://cr.openjdk.java.net/~dholmes/6515172/webrev.v2/
>>
>> Reviewed by comparing the two patch files. Looks good modulo
>> one question: should the change to src/share/vm/runtime/globals.hpp
>> be in src/os/linux/vm/globals_linux.hpp instead?
>
> Thanks Dan!
>
> http://cr.openjdk.java.net/~dholmes/6515172/webrev.v3/

src/os/linux/vm/globals_linux.hpp
     No comments.

src/os/linux/vm/os_linux.cpp
     No comments.

src/share/vm/logging/logTag.hpp
     No comments.

test/runtime/os/AvailableProcessors.java
     No comments.

Thumbs up.

> I hadn't realized I could do that! There are a few flags in 
> globals.hpp that should be elsewhere.

Yet Another Cleanup Bug (YACB) :-)


> One glitch is that the per-os flag definitions are not set up to 
> handle "experimental". I did not want to have to change all of the 
> platform files to deal with that, so I switched back to a diagnostic 
> option. I hope that is okay? (Comment was updated in os_linux.cpp)

That is fine with me. Hmmmm... I remember running into that
limitation recently, but I don't remember the context. It was
probably when I was downgrading various Java Monitor options
from product flags to diagnostic/experimental.


> I also want to note that Thomas Stuefe raised an issue with the use of 
> strerror() - it may not be thread-safe. We already use it quite a lot 
> in the VM code so I think there needs to be a general solution so I 
> will file a separate CR. I also note that it will make the logging use 
> of strerror very awkward if we need to define a local buffer, and not 
> have any impact when logging is disabled. :(

I saw the new bug. That one is depressing.

Dan


>
> Thanks,
> David
>
>> Dan
>>
>>
>>>
>>> - UseNewCode is replaced by a new experimental flag UseCpuAllocPath
>>> - the logging statement shows whether the path was forced to be taken
>>> - this is now enabled in product builds
>>>
>>> Sorry I don't know how to generate an incremental webrev when working
>>> with uncommitted changes. :(
>>>
>>> Thanks,
>>> David
>>>
>>> On 27/01/2016 9:20 AM, Gerald Thornbrugh wrote:
>>>> Hi David,
>>>>
>>>> I also like the idea of using an "-XX" option, that seems like the 
>>>> best
>>>> way to go.
>>>>
>>>> I am ok with leaving the code in.  If there are any issues with the 
>>>> code
>>>> we can always
>>>> create a bug and remove it.
>>>>
>>>> Jerry
>>>>> On 1/26/16 2:12 PM, David Holmes wrote:
>>>>>> Update ...
>>>>>>
>>>>>> On 22/01/2016 6:06 PM, David Holmes wrote:
>>>>>>> First a special thanks to Martin Buchholz for his input, feedback,
>>>>>>> critique and raising awareness of how non-simple this issue is.
>>>>>>>
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-6515172
>>>>>>> webrev: http://cr.openjdk.java.net/~dholmes/6515172/webrev/
>>>>>>>
>>>>>>> Basic problem:
>>>>>>>    processors available for use <= processors online <= processors
>>>>>>> configured
>>>>>>>
>>>>>>> but we always returned the number of online processors.
>>>>>>>
>>>>>>> Solution is simple in its basic form: use sched_getaffinity to get
>>>>>>> the
>>>>>>> scheduling affinity mask and count the number of available
>>>>>>> processors.
>>>>>>>
>>>>>>> Details are complicated by the desire to handle very large 
>>>>>>> processor
>>>>>>> systems. See the bug report for lots of detailed discussions and
>>>>>>> references.
>>>>>>>
>>>>>>> Testing:
>>>>>>>   - new test that verifies behaviour when running under taskset
>>>>>>>   - diagnostic hook injection (UseNewCodeN) to enable testing of 
>>>>>>> all
>>>>>>> code paths (one hook is left in for non-product to allow easy
>>>>>>> testing of
>>>>>>> the dynamic path)
>>>>>>
>>>>>> I have been told that using the development flag UseNewCode in
>>>>>> released code is a bad idea because it is used internally during
>>>>>> development (as per its defined purpose).
>>>>>>
>>>>>> I would like to be able to test the dynamic path easily, but I 
>>>>>> didn't
>>>>>> want to pay the price of adding a new VM option to do it. So choices
>>>>>> are:
>>>>>>
>>>>>> a) don't do anything (remove the UseNewCode check)
>>>>>> b) add a new diagnostic flag
>>>>>> c) add a new experimental flag
>>>>>
>>>>> My vote:
>>>>>
>>>>> -XX:+UnlockExperimentalVMOptions -XX:+DynamicConfiguredCPUPath
>>>>>
>>>>> With a plan (and a bug) to remove that option down the road.
>>>>>
>>>>> And a code review.
>>>>>
>>>>> src/os/linux/vm/os_linux.cpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/logging/logTag.hpp
>>>>>     No comments.
>>>>>
>>>>> test/runtime/os/AvailableProcessors.java
>>>>>     Nice test. I like that the default (no args) runs a set of tests
>>>>>     and if you (manually) run the test with a specific arg it will
>>>>>     check just that one value.
>>>>>
>>>>> Thumbs up! (Assuming you change UseNewCode to something else or
>>>>> delete its use all together.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>>
>>>>>> Thoughts?
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>
>>>>>>>   - JPRT
>>>>>>>
>>>>>>> Compatability issues:
>>>>>>> - the system code we're using now is at least 5 years old so 
>>>>>>> distro's
>>>>>>> older than that (which are not officially supported) may not work
>>>>>>> - anyone already running under a processor constrained environment
>>>>>>> (like
>>>>>>> Docker) and using availableProcessor() to "size" things, will find
>>>>>>> that
>>>>>>> size has now changed. We do not expect this to be a problem - on 
>>>>>>> the
>>>>>>> contrary we expect Docker users to want the new behaviour.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>
>>>>
>>