RFR [XS] : 8215707: [macosx] check return code of pthread_getschedparam

[David Holmes]

Hi Matthias,

On 20/12/2018 7:21 pm, Baesken, Matthias wrote:
> Hello,  while looking at the  thread  prio + scheduling handling in  hotspot, I noticed that  at one place  we seem to handle pthread_getschedparam  in a wrong way.

Note this code is likely never exercised due to the default 
ThreadPriorityPolicy of 0. It is entirely possible no one has ever 
really used it. That seems more likely given that the code is incorrect:

pthread_getschedparam(pthread_self(), &policy, &sp);

this always gets the priority of the current thread, but the method need 
not be called on the current thread!

> According to the API documentation, pthread_getschedparam returns an int which is 0 when successful, otherwise it is a value != 0.
> See
> 
> https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/pthread_setschedparam.3.html
> 
> 
> "If successful, these functions return 0. Otherwise, an error number is returned to indicate the error."

The only possible error for pthread_getschedparam is ESRCH if the thread 
id is not valid - though that is impossible with the current code using 
pthread_self()! So the current code - as written - did not need to check 
the return value. If you fix the real bug then you should check the 
return value.

I've also highly suspicious of using this API because the man page 
claims that the scheduling policy can only be SCHED_FIFO or SCHED_RR - 
that surprised me because they are both fixed-priority preemptive 
scheduling algorithms normally reserved for "real-time". There is no 
mention of SCHED_OTHER which would be the normal non-realtime scheduling 
mode. So how this function can return only SCHED_FIFO or SCHED_RR if 
you've never actually set that scheduling mode seems a problem! maybe 
the manpage is just wrong.

> 
> At some places the return code of pthread_getschedparam is checked in the OpenJDK hotspot  sources, however at os::get_native_priority (os_bsd.cpp) it is not done, but should be added.
> The current  coding handles the  function in a  getpriority-like way  (which might or might not work), but at least it is not what is documented in the API doc and done at other places in the coding.
> 
> 
> Bug/webrev :
> 
> https://bugs.openjdk.java.net/browse/JDK-8215707
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8215707.0/

!   int res = pthread_getschedparam(pthread_self(), &policy, &sp);
     *priority_ptr = sp.sched_priority;
+   if (res != 0) {
+     return OS_ERR;
+   } else {
+     return OS_OK;
+   }

You should only set *priority_ptr if pthread_getschedParam() was successful!

Seriously you are walking through a minefield with this scheduling 
stuff, especially on OS X.

Cheers,
David
-----

> 
> Thanks, Matthias
>