RFR : 8215962: Support ThreadPriorityPolicy mode 1 for non-root users on linux/bsd

[Daniel D. Daugherty]

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
>>> -----
>>>
>>