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

[Baesken, Matthias]

> Note this code is likely never exercised due to the default
> ThreadPriorityPolicy of 0.

Hi  David,  ThreadPriorityPolicy=1 is a valid setting / product flag.
You find quite a few people setting  ThreadPriorityPolicy when you search for it,  so  the coding should better be fixed I guess .

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

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