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

[David Holmes]

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