RFR : 8215962: Support ThreadPriorityPolicy mode 1 for non-root users on linux/bsd
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Jan 4 17:13:00 UTC 2019
On 1/4/19 12:04 PM, Baesken, Matthias wrote:
> New webrev :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8215962.3/
src/hotspot/os/bsd/os_bsd.cpp
No comments.
src/hotspot/os/linux/os_linux.cpp
No comments.
src/hotspot/share/runtime/globals.hpp
No comments.
Thumbs up.
Dan
>
> - comma added
> - FLAG_IS_DEFAULT(...) check is back - I thought that the check is no longer needed but maybe I was wrong
>
>
> Best regards, Matthias
>
>
>> -----Original Message-----
>> From: Daniel D. Daugherty <daniel.daugherty at oracle.com>
>> Sent: Freitag, 4. Januar 2019 17:04
>> To: Baesken, Matthias <matthias.baesken at sap.com>; David Holmes
>> <david.holmes at oracle.com>; 'hotspot-dev at openjdk.java.net' <hotspot-
>> dev at openjdk.java.net>
>> Subject: Re: RFR : 8215962: Support ThreadPriorityPolicy mode 1 for non-root
>> users on linux/bsd
>>
>> On 1/4/19 10:38 AM, Baesken, Matthias wrote:
>>> Hi David/Dan , here is my new webrev, the warning (for non-root) is
>> back and uses the improved wording :
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8215962.2/
>> src/hotspot/os/bsd/os_bsd.cpp
>> L2260: // (e.g. root privilege or CAP_SYS_NICE capability).
>> nit: please add a comma after 'e.g.'.
>>
>> old L2310: if (!FLAG_IS_DEFAULT(ThreadPriorityPolicy)) {
>> Why did you drop this check around the warning? The
>> warning should only be issued if ThreadPriorityPolicy
>> is not the default value, i.e., it was set on the cmd
>> line to a non-default value. If some distro chooses to
>> change the default of ThreadPriorityPolicy from 0 to 1,
>> then this warning will always happen.
>>
>> src/hotspot/os/linux/os_linux.cpp
>> L4080: // (e.g. root privilege or CAP_SYS_NICE capability).
>> nit: please add a comma after 'e.g.'.
>>
>> old L4107: if (!FLAG_IS_DEFAULT(ThreadPriorityPolicy)) {
>> Why did you drop this check around the warning?
>>
>> src/hotspot/share/runtime/globals.hpp
>> No comments.
>>
>> Dan
>>
>>> Best Regards , Matthias
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: David Holmes <david.holmes at oracle.com>
>>>> Sent: Donnerstag, 3. Januar 2019 23:52
>>>> To: daniel.daugherty at oracle.com; Baesken, Matthias
>>>> <matthias.baesken at sap.com>; 'hotspot-dev at openjdk.java.net'
>> <hotspot-
>>>> dev at openjdk.java.net>
>>>> Subject: Re: RFR : 8215962: Support ThreadPriorityPolicy mode 1 for non-
>> root
>>>> users on linux/bsd
>>>>
>>>> Hi Dan,
>>>>
>>>> Thanks for your reluctant agreement. I like your updated wording for the
>>>> warning.
>>>>
>>>> David
>>>>
>>>> On 4/01/2019 8:21 am, Daniel D. Daugherty wrote:
>>>>> On 1/3/19 4:49 PM, David Holmes wrote:
>>>>>> Hi Matthias,
>>>>>>
>>>>>> On 4/01/2019 12:56 am, Baesken, Matthias wrote:
>>>>>>>> I still want to know how the OS_ERR gets handled by all the higher
>>>>>>>> level
>>>>>>>> code. How will this failure at runtime get reported back to
>>>>>>>> application code?
>>>>>>> Hi David , the "best practice" currently is to just ignore the
>>>>>>> return code of os::set_native_priority , it is called and we hope
>>>>>>> for the best ,
>>>>>> Silent failure is not good.
>>>>> Agreed.
>>>>>
>>>>>
>>>>>> In that case I think it is appropriate to issue a warning whenever
>>>>>> ThreadPriorityPolicy=1 is set, though compatibility dictates no
>>>>>> warning in the case that you are root.
>>>>> Agreed that you should not get the warning if you are root.
>>>>> Reluctantly agree to always issue the warning if
>>>>> -XX:ThreadPriorityPolicy=1 is specified and user != root.
>>>>>
>>>>>
>>>>>> Which brings us back to square one and the original patch with a
>>>>>> different warning message:
>>>>>>
>>>>>> warning("-XX:ThreadPriorityPolicy=1 requires system level permissions
>>>>>> to be applied. If these permissions do not exist, changes to priority
>>>>>> will be silently ignored.");
>>>>> Perhaps:
>>>>>
>>>>> warning("-XX:ThreadPriorityPolicy=1 may require system level
>> permission,
>>>>> e.g., being the 'root' user. If the necessary permission is not
>>>>> possessed, changes to priority will be silently ignored.");
>>>>>
>>>>> I changed:
>>>>>
>>>>> - "requires" -> "may require" because you don't always need
>>>>> a special permission to do the operation.
>>>>> - "system level permissions to be applied" -> "system level permission"
>>>>> so switch to singular permission, dropped "to be applied"
>>>>> - added ", e.g., being the 'root' user"
>>>>> - "If these permissions do not exist" -> "If the necessary permission is
>>>>> not possessed"
>>>>> so switch to singular permission, switch from "do not exist" to
>>>>> "is not possessed"
>>>>>
>>>>>
>>>>>> which then takes us back to Dan's comments that the warning should
>> be
>>>>>> at time of use. But to that I maintain that because use may or may not
>>>>>> fail depending on both the available permissions and the requested
>>>>>> priority value, that it is better to have a single generic warning in
>>>>>> the existing place.
>>>>> This is where David and I disagree. I do not think we should issue a
>>>>> warning unless the operation failed and David prefers the generic
>>>>> warning in one place.
>>>>>
>>>>> I will reluctantly agree to always issue the warning if
>>>>> -XX:ThreadPriorityPolicy=1 is specified and user != root.
>>>>>
>>>>> Dan
>>>>>
>>>>>>> for example :
>>>>>>>
>>>>>>> jdk/src/hotspot/share/runtime/vmThread.cpp
>>>>>>>
>>>>>>> 299 int prio = (VMThreadPriority == -1)
>>>>>>> 300 ? os::java_to_os_priority[NearMaxPriority]
>>>>>>> 301 : VMThreadPriority;
>>>>>>> 302 // Note that I cannot call os::set_priority because it expects Java
>>>>>>> 303 // priorities and I am *explicitly* using OS priorities so that
>>>>>>> it's
>>>>>>> 304 // possible to set the VM thread priority higher than any Java
>>>>>>> thread.
>>>>>>> 305 os::set_native_priority( this, prio );
>>>>>>>
>>>>>>>
>>>>>>> jdk/src/hotspot/share/compiler/compileBroker.cpp
>>>>>>>
>>>>>>> 783 int native_prio = CompilerThreadPriority;
>>>>>>> 784 if (native_prio == -1) {
>>>>>>> 785 if (UseCriticalCompilerThreadPriority) {
>>>>>>> 786 native_prio = os::java_to_os_priority[CriticalPriority];
>>>>>>> 787 } else {
>>>>>>> 788 native_prio = os::java_to_os_priority[NearMaxPriority];
>>>>>>> 789 }
>>>>>>> 790 }
>>>>>>> 791 os::set_native_priority(thread, native_prio);
>>>>>>>
>>>>>>>
>>>>>>> A difference is
>>>>>>>
>>>>>>> jdk/src/hotspot/share/runtime/os.cpp
>>>>>>>
>>>>>>> 217OSReturn os::set_priority(Thread* thread, ThreadPriority p) {
>>>>>>> 218
>> debug_only(Thread::check_for_dangling_thread_pointer(thread);)
>>>>>>> 219
>>>>>>> 220 if (p >= MinPriority && p <= MaxPriority) {
>>>>>>> 221 int priority = java_to_os_priority[p];
>>>>>>> 222 return set_native_priority(thread, priority);
>>>>>>>
>>>>>>> Where the return code of set_native_priority() is returned
>>>>>>> (however then it is later usually not handled by the callers of
>>>>>>> os::set_priority.
>>>>>>>
>>>>>>>> As I stated this is not a complete statement as on Linux at least you
>>>>>>>> also have to account for RLIMIT_RTPRIO.
>>>>>>>>
>>>>>>> If you want me to do, I can for course add a short statement about
>>>>>>> this .
>>>>>> Quite the opposite, I'd rather see a generic statement about
>>>>>> permissions than try to cover all the possible situations.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Best regards, Matthias
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: David Holmes <david.holmes at oracle.com>
>>>>>>>> Sent: Donnerstag, 3. Januar 2019 14:02
>>>>>>>> To: Baesken, Matthias <matthias.baesken at sap.com>;
>>>>>>>> daniel.daugherty at oracle.com; 'hotspot-dev at openjdk.java.net'
>>>> <hotspot-
>>>>>>>> dev at openjdk.java.net>
>>>>>>>> Subject: Re: RFR : 8215962: Support ThreadPriorityPolicy mode 1 for
>>>>>>>> non-root
>>>>>>>> users on linux/bsd
>>>>>>>>
>>>>>>>> Hi Matthias,
>>>>>>>>
>>>>>>>> On 3/01/2019 9:13 pm, Baesken, Matthias wrote:
>>>>>>>>> Hello David and Dan , here is a second webrev :
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8215962.1/
>>>>>>>>>
>>>>>>>>> - adjusted copyright years + fixed some typos
>>>>>>>>> - added the missing return for FreeBSD (pointed out by Dan)
>>>>>>>>> - removed the warning message completely
>>>>>>>> I still want to know how the OS_ERR gets handled by all the higher
>>>>>>>> level
>>>>>>>> code. How will this failure at runtime get reported back to application
>>>>>>>> code?
>>>>>>>>
>>>>>>>> ! // It is only used when ThreadPriorityPolicy=1 and requires root
>>>>>>>> privilege or
>>>>>>>> ! // CAP_SYS_NICE capability.
>>>>>>>>
>>>>>>>> As I stated this is not a complete statement as on Linux at least you
>>>>>>>> also have to account for RLIMIT_RTPRIO.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
More information about the hotspot-dev
mailing list