RFR : 8215962: Support ThreadPriorityPolicy mode 1 for non-root users on linux/bsd
Baesken, Matthias
matthias.baesken at sap.com
Fri Jan 4 17:04:34 UTC 2019
New webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8215962.3/
- 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