Re: timer_create () abort

[Daniel Jacobowitz <drow at mvista dot com>]

On Thu, Jun 05, 2003 at 03:50:32PM -0700, Kaz Kylheku wrote:
> On Thu, 5 Jun 2003, Daniel Jacobowitz wrote:
> 
> > timer: ../linuxthreads/sysdeps/pthread/timer_routines.c:578: __timer_dealloc: Assertion `timer->refcount == 0' failed.
> 
> > The assertion's easy: __timer_alloc sets the count to 1, __timer_dealloc
> > assumes it will be 0, we never decrease the refcount before deallocating it
> > on the EAGAIN exit path.  I think just adding a timer_delref before the call
> > to __timer_dealloc is the right fix.  Thoughts?
> 
> That is correct. The decrementing of the refcount and deletion should
> probably have been combined into one operation.
> 
> > But why are we failing at all?  Well, we're out of threads. 
> > __timer_thread_alloc looks on the thread free list for something to move to
> > the active list; __timer_dealloc moves it from the active list to the free
> > list; but __timer_thread_find_matching removes from the active list and it
> > doesn't get put back.  
> 
> This looks like a bug. This function should quite simply not be
> removing the thread from the active list. To be on the active list
> simply means to be an allocated valid thread; it's just a way of
> keeping track of the allocated thread so that this function can do its
> job of finding a matching one. 
> 
> This is nothing but good old object interning at work. We want two
> threads that have the same set of properties to be the same object, so
> that for any unique set of thread properties (priority, etc) there is
> just one thread handling all the timers which want that type of thread.
> 
> > So once a thread gets one timer, it never gets
> > another.  And when the timer is deleted, the thread does not exit, so we
> > never have room for a new thread - I think that's by design.
> 
> This is a straight leak; effectively, be removing the thread from the
> active list, we are losing a reference without destroying it.
> 
> When a timer is deleted, we don't want the thread to exit, because it
> can have other timers. Essentially, threads persist.   There could be
> some additional logic in timer_delete to kill the thread and delete the
> thread structure if this is the last timer being serviced by that
> thread.
> 
> The design assumes that the appliaction will only generate a small
> variety of different kinds of threads for SIGEV_THREAD notification. So
> there will just be at most a tiny pool of threads in the active list,
> and these can just endure over the lifetime of the application.  

Thanks.  In that case, would a glibc maintainer please consider this
patch?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-06-19  Daniel Jacobowitz  <drow@mvista.com>

	* sysdeps/pthread/timer_create.c (timer_create): Call timer_delref
	before __timer_dealloc.
	* sysdeps/pthread/timer_routines.c (__timer_thread_find_matching):
	Don't call list_unlink.

Index: linuxthreads/sysdeps/pthread/timer_create.c
===================================================================
RCS file: /cvs/glibc/libc/linuxthreads/sysdeps/pthread/timer_create.c,v
retrieving revision 1.8
diff -u -p -r1.8 timer_create.c
--- linuxthreads/sysdeps/pthread/timer_create.c	3 Mar 2003 05:28:17 -0000	1.8
+++ linuxthreads/sysdeps/pthread/timer_create.c	20 Jun 2003 03:24:50 -0000
@@ -178,7 +178,10 @@ timer_create (clock_id, evp, timerid)
       if (thread != NULL)
 	__timer_thread_dealloc (thread);
       if (newtimer != NULL)
-	__timer_dealloc (newtimer);
+	{
+	  timer_delref (newtimer);
+	  __timer_dealloc (newtimer);
+	}
     }
 
   pthread_mutex_unlock (&__timer_mutex);
Index: linuxthreads/sysdeps/pthread/timer_routines.c
===================================================================
RCS file: /cvs/glibc/libc/linuxthreads/sysdeps/pthread/timer_routines.c,v
retrieving revision 1.21
diff -u -p -r1.21 timer_routines.c
--- linuxthreads/sysdeps/pthread/timer_routines.c	29 Aug 2002 01:41:17 -0000	1.21
+++ linuxthreads/sysdeps/pthread/timer_routines.c	20 Jun 2003 03:24:51 -0000
@@ -538,10 +538,7 @@ __timer_thread_find_matching (const pthr
 
       if (thread_attr_compare (desired_attr, &candidate->attr)
 	  && desired_clock_id == candidate->clock_id)
-	{
-	  list_unlink (iter);
-	  return candidate;
-        }
+	return candidate;
 
       iter = list_next (iter);
     }