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

[David Holmes]

Thanks Dan!

Also thanks Jerry and Thomas.

I will push the changes.

David
-----

On 29/01/2016 3:52 AM, Daniel D. Daugherty wrote:
> 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
>>>>>>
>>>>>
>>>
>