[PATCH] pthreadgetcpuclockid.c: Implement pthread_getcpuclockid to return thread Id

[Joel Sherrill]

Just to clear up some of the questions earlier in this thread. I threw this
code
together in less than 5 minutes in response to an email so it shouldn't be
trusted
as correct.

In order to merge this code, there must be thorough test code, the POSIX
Users Guide updated, the POSIX Compliance Guide updated, and per
the ticket #3891, there are at least use cases in clock_gettime and
clock_settime
that must also be addressed. All code modified/added must have test cases
and may impact documentation.

https://devel.rtems.org/ticket/3891

The other POSIX use cases are based on capabilities enabled when
_POSIX_THREAD_CPUTIME  is defined by the implementation.
pthread_create() has a reference to it. Along with at least clock_gettime
and clock_gettime

https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_create.html


This is the top of POSIX Issue 7.

https://pubs.opengroup.org/onlinepubs/9699919799/

More below.

On Sun, Apr 5, 2020 at 10:06 AM Gedare Bloom <gedare at rtems.org> wrote:

> Hi Joel,
>
> Something about the critical section logic here bugs me, see my crude
> analysis below:
>
> On Thu, Mar 12, 2020 at 2:00 PM Joel Sherrill <joel at rtems.org> wrote:
> >
> > ---
> >  cpukit/posix/src/pthreadgetcpuclockid.c | 31
> ++++++++++++++++++++++++++-----
> >  1 file changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/cpukit/posix/src/pthreadgetcpuclockid.c
> b/cpukit/posix/src/pthreadgetcpuclockid.c
> > index cd7814b..c15962e 100644
> > --- a/cpukit/posix/src/pthreadgetcpuclockid.c
> > +++ b/cpukit/posix/src/pthreadgetcpuclockid.c
> > @@ -7,8 +7,10 @@
> >
> >  /*
> >   *  20.1.6 Accessing a Thread CPU-time Clock, P1003.4b/Draft 8, p. 58
> > - *
> > - *  COPYRIGHT (c) 1989-2007.
> > + */
> > +
> > +/*
> > + *  COPYRIGHT (c) 1989-2007,2020.
> >   *  On-Line Applications Research Corporation (OAR).
> >   *
> >   *  The license and distribution terms for this file may be
> > @@ -23,12 +25,31 @@
> >  #include <pthread.h>
> >  #include <errno.h>
> >
> > -#include <rtems/seterr.h>
> > +#include <rtems/score/threadimpl.h>
> >
> >  int pthread_getcpuclockid(
> > -  pthread_t    pid,
> > +  pthread_t    thread,
> >    clockid_t   *clock_id
> >  )
> >  {
> > -  rtems_set_errno_and_return_minus_one( ENOSYS );
> > +  Thread_Control               *the_thread;
> > +  ISR_lock_Context              lock_context;
> > +
> > +  if ( clock_id == NULL ) {
> > +    return EINVAL;
> > +  }
> > +
> > +  the_thread = _Thread_Get( thread, &lock_context );
> > +
> This lock_context now holds an ISR lock
>
> > +  if ( the_thread == NULL ) {
> > +    return ESRCH;
> > +  }
> > +
> > +  _Thread_State_acquire_critical( the_thread, &lock_context );
> > +
> In SMP the lock_context is now overwritten by the thread's TQ lock.
>
> > +  *clock_id = the_thread->Object.id;
> > +
> > +  _Thread_State_release( the_thread, &lock_context );
> Releasing the TQ lock
>
> > +
> > +  return 0;
> Leaking the ISR lock, returning with ISRs disabled.
>

I thought I copied the pattern from one of the simple pthread methods. I
don't remember which one but pthread_getnamemp() should be a
better reference that whatever I chose. It doesn't cause any changes
impacting scheduling either that would impact dispatching. Based
on that method, I think it is more like this:

 _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();

return 0;


Does that look better.

FWIW I think the signature of the code in git is wrong. It has

pid as the first parameter. It should be this.


int pthread_getcpuclockid(pthread_t thread_id, clockid_t *clock_id);


For sure, the patch set needs to be complete and add tests,

documentation, etc.

--joel



>  }
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20200405/5031875c/attachment.html>