RFR [XS] 8215707: [macosx] fix pthread_getschedparam and pthread_setschedparam calls - was : RE: RFR [XS] : 8215707: [macosx] check return code of pthread_getschedparam

[David Holmes]

Hi Matthias,

On 21/12/2018 5:01 pm, Baesken, Matthias wrote:
> Hello David, I adjusted  the  bug synopsis and description,  the second webrev   uses  the pthread id  of the passed  Thread*  instead of  pthread_self()  :
> 
> 
> https://bugs.openjdk.java.net/browse/JDK-8215707
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8215707.1/

Looks good.

Thanks,
David

> 
> 
> Best regards, Matthias
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Donnerstag, 20. Dezember 2018 13:19
>> 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
>>
>> 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
>>
>