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

[David Holmes]

On 20/12/2018 9:56 pm, Baesken, Matthias wrote:
>> Note this code is likely never exercised due to the default
>> ThreadPriorityPolicy of 0.
> 
> Hi  David,  ThreadPriorityPolicy=1 is a valid setting / product flag.

Yes it is. That doesn't mean it is actually used to any great extent.

> You find quite a few people setting  ThreadPriorityPolicy when you search for it,  so  the coding should better be fixed I guess .

Quite a few? I could only find a couple (Cassandra, Gatling) and they 
were on Linux. I was surprised to find how many people knew about the 
bug that allowed the root check to be by-passed.

The fact this code is incorrect on OS X indicates to me it is not 
actually being used - or by chance priority is only ever changed in the 
current thread.

>>
>>   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!
> 
> Yes , true !
> Same for  os::set_native_priority  in  os_bsd.cpp , there pthread_self()  is also  used instead of the  real Thread .
> 
> 2338OSReturn os::set_native_priority(Thread* thread, int newpri) {
> 2339  if (!UseThreadPriorities || ThreadPriorityPolicy == 0) return OS_OK;
> 2340
>    ....
> 2346#elif defined(__APPLE__) || defined(__NetBSD__)
> 2347  struct sched_param sp;
> 2348  int policy;
> 2349  pthread_t self = pthread_self();
> 2350
> 2351  if (pthread_getschedparam(self, &policy, &sp) != 0) {
> 2352    return OS_ERR;
> 2353  }

If you intend fixing this probably best to modify the bug synopsis and 
description.

Cheers,
David

> Best  regards, Matthias
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Donnerstag, 20. Dezember 2018 11:45
>> To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
>> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
>> Subject: Re: RFR [XS] : 8215707: [macosx] check return code of
>> pthread_getschedparam
>>
>> 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/Conce
>> ptual/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
>>>