[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: os_windows.cpp : simplify is_thread_cpu_time_supported ?
From:       David Holmes <david.holmes () oracle ! com>
Date:       2021-03-27 0:08:20
Message-ID: 3b9acb57-c17f-c767-efcd-4e3c929118d5 () oracle ! com
[Download RAW message or body]

On 26/03/2021 6:06 pm, Baesken, Matthias wrote:
> Hi David, thanks for the info .
> 
> I found  https://docs.microsoft.com/en-us/windows/win32/procthread/thread-security-and-access-rights
>  so it looks like  we  need  THREAD_QUERY_INFORMATION or \
> THREAD_QUERY_LIMITED_INFORMATION access right   for  GetThreadTimes . 
> On the other hand ,   the test in os::is_thread_cpu_time_supported()   on Windows   \
> might  (temporary ?)  fail for  other reasons too , it  is not clear to me if this  \
> is really always related to the wrong access rights ?

Sure it could fail for other reasons (unfortunately win32 docs don't 
actually list them). So I tend to agree that this kind of check as a 
global "do we support thread cpu times" is not the right test to do. The 
question is really about "does this platform provide an API for getting 
the thread cpu time" - and it does. Whether you can query a given target 
thread at runtime is a different matter altogether.

I wish I knew exactly what caused this check to be put in place as it 
doesn't seem appropriate. Maybe someone from serviceablity (cc'd) remembers?

> And at some places in HS code like  jfrThreadCPULoadEvent.cpp  ,    \
> os::thread_cpu_time  is called anyway without checking  for  \
> os::is_thread_cpu_time_supported()  ; same for thread.cpp  / Thread::print_on  but \
> this is just printing some output so it is most likely not really a big issue  on \
> Windows .

As long as they can tolerate the -1 return if it fails then it is up to 
that code whether or not to bother with the check. But I think the check 
is primarily for the M&M/JVMTI capability checking.

Cheers,
David

> Best regards, Matthias
> 
> 
> 
> 
> > > On 25/03/2021 11:49 pm, Baesken, Matthias wrote:
> > > > Hello,  I wonder , should we just return  true  in
> > > > os::is_thread_cpu_time_supported()   on Windows  ?
> > > > 
> > > > See
> > > > 
> > > > https://github.com/openjdk/jdk/blob/master/src/hotspot/os/windows/os_windows.cpp#L4588
> > > >  
> > > > According to  MSDN
> > > > https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getthreadtimes
> > > >  
> > > > GetThreadTimes is supported  on Win2003/XP and higher . This should be
> > > > fine for OpenJDK .
> > > 
> > > Yes it should be fine. There may be other Windows archaisms in the code
> > > that could be cleaned up now.
> > 
> > The issue was not API availability (we got rid of that check a long time
> > ago) but security permissions. We actually just returned "true" prior to
> > JDK 5 but that was changed by JDK-4884677 when the JVM TI support was added.
> > 
> > It is a bit messy. We use the result of
> > os::is_thread_cpu_time_supported() at initialization time, on the main
> > thread to then decide the global availability of this feature. And via
> > the normal launcher that thread will have all access bits set and so we
> > will flag thread_cpu_time as being available. At runtime we might
> > encounter a thread for which the access bits are not present and so the
> > actual get_thread_cpu_time call may return -1. In theory the JVM could
> > be loaded on a thread without full permissions and so we would then
> > globally disable thread_cpu_time.
> > 
> > So I think this code has to stay.
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]