<div dir="ltr"><div>Just to clear up some of the questions earlier in this thread. I threw this code</div><div>together in less than 5 minutes in response to an email so it shouldn't be trusted</div><div>as correct. </div><div><br></div><div>In order to merge this code, there must be thorough test code, the POSIX </div><div>Users Guide updated, the POSIX Compliance Guide updated, and per</div><div>the ticket #3891, there are at least use cases in clock_gettime and clock_settime</div><div>that must also be addressed. All code modified/added must have test cases</div><div>and may impact documentation.<br><br><a href="https://devel.rtems.org/ticket/3891">https://devel.rtems.org/ticket/3891</a> </div><div><br></div><div>The other POSIX use cases are based on capabilities enabled when </div><div><span style="color:rgb(0,0,0);font-family:Verdana,Arial,Helvetica,sans-serif;font-size:13.3333px">_POSIX_THREAD_CPUTIME </span> is defined by the implementation. <br></div><div>pthread_create() has a reference to it. Along with at least clock_gettime</div><div>and clock_gettime</div><div><br></div><div><a href="https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_create.html">https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_create.html</a> <br></div><div><br></div><div>This is the top of POSIX Issue 7.</div><div><br></div><div><a href="https://pubs.opengroup.org/onlinepubs/9699919799/">https://pubs.opengroup.org/onlinepubs/9699919799/</a> <br></div><div><br></div><div>More below. <br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Apr 5, 2020 at 10:06 AM Gedare Bloom <<a href="mailto:gedare@rtems.org">gedare@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Joel,<br>
<br>
Something about the critical section logic here bugs me, see my crude<br>
analysis below:<br>
<br>
On Thu, Mar 12, 2020 at 2:00 PM Joel Sherrill <<a href="mailto:joel@rtems.org" target="_blank">joel@rtems.org</a>> wrote:<br>
><br>
> ---<br>
> cpukit/posix/src/pthreadgetcpuclockid.c | 31 ++++++++++++++++++++++++++-----<br>
> 1 file changed, 26 insertions(+), 5 deletions(-)<br>
><br>
> diff --git a/cpukit/posix/src/pthreadgetcpuclockid.c b/cpukit/posix/src/pthreadgetcpuclockid.c<br>
> index cd7814b..c15962e 100644<br>
> --- a/cpukit/posix/src/pthreadgetcpuclockid.c<br>
> +++ b/cpukit/posix/src/pthreadgetcpuclockid.c<br>
> @@ -7,8 +7,10 @@<br>
><br>
> /*<br>
> * 20.1.6 Accessing a Thread CPU-time Clock, P1003.4b/Draft 8, p. 58<br>
> - *<br>
> - * COPYRIGHT (c) 1989-2007.<br>
> + */<br>
> +<br>
> +/*<br>
> + * COPYRIGHT (c) 1989-2007,2020.<br>
> * On-Line Applications Research Corporation (OAR).<br>
> *<br>
> * The license and distribution terms for this file may be<br>
> @@ -23,12 +25,31 @@<br>
> #include <pthread.h><br>
> #include <errno.h><br>
><br>
> -#include <rtems/seterr.h><br>
> +#include <rtems/score/threadimpl.h><br>
><br>
> int pthread_getcpuclockid(<br>
> - pthread_t pid,<br>
> + pthread_t thread,<br>
> clockid_t *clock_id<br>
> )<br>
> {<br>
> - rtems_set_errno_and_return_minus_one( ENOSYS );<br>
> + Thread_Control *the_thread;<br>
> + ISR_lock_Context lock_context;<br>
> +<br>
> + if ( clock_id == NULL ) {<br>
> + return EINVAL;<br>
> + }<br>
> +<br>
> + the_thread = _Thread_Get( thread, &lock_context );<br>
> +<br>
This lock_context now holds an ISR lock<br>
<br>
> + if ( the_thread == NULL ) {<br>
> + return ESRCH;<br>
> + }<br>
> +<br>
> + _Thread_State_acquire_critical( the_thread, &lock_context );<br>
> +<br>
In SMP the lock_context is now overwritten by the thread's TQ lock.<br>
<br>
> + *clock_id = the_thread->Object.id;<br>
> +<br>
> + _Thread_State_release( the_thread, &lock_context );<br>
Releasing the TQ lock<br>
<br>
> +<br>
> + return 0;<br>
Leaking the ISR lock, returning with ISRs disabled.<br></blockquote><div><br></div><div>I thought I copied the pattern from one of the simple pthread methods. I</div><div>don't remember which one but pthread_getnamemp() should be a </div><div>better reference that whatever I chose. It doesn't cause any changes</div><div>impacting scheduling either that would impact dispatching. Based</div><div>on that method, I think it is more like this:</div><div><br></div><div><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code>
<pre style="padding:0px;margin-top:0px;margin-bottom:0px"><code> _Objects_Allocator_lock();
the_thread = _Thread_Get( thread, &lock_context );
if ( the_thread == NULL ) {
_Objects_Allocator_unlock();
return ESRCH;
}
_ISR_lock_ISR_enable( &lock_context );
*clock_id = the_thread->Object.id
_Objects_Allocator_unlock();</code></pre> return 0;</code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code><br></code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code>Does that look better.
<br></code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code>FWIW I think the signature of the code in git is wrong. It has</code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code>pid as the first parameter. It should be this.</code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code>
int pthread_getcpuclockid(pthread_t thread_id, clockid_t *clock_id);</code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code><br></code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code>For sure, the patch set needs to be complete and add tests, </code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code>documentation, etc.
<br></code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px">--joel</pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code><br></code></pre><pre style="padding:0px;margin-top:0px;margin-bottom:0px;color:rgb(0,0,0);font-size:13.3333px"><code><br></code></pre></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> }<br>
> --<br>
> 1.8.3.1<br>
><br>
> _______________________________________________<br>
> devel mailing list<br>
> <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
</blockquote></div></div>