RFR : 8215962: Support ThreadPriorityPolicy mode 1 for non-root users on linux/bsd
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Jan 4 16:04:05 UTC 2019
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