From c29f5163db85b1b4097a791ca1ba96f2b52f1f04 Mon Sep 17 00:00:00 2001 From: Simon Farnsworth Date: Fri, 13 Jul 2007 15:41:12 +0100 Subject: Prevent ticket system deadlock when using DVB subtitles When using DVB subtitles on an SMP machine, we see occasional lockups, which appear to be caused by one thread acquiring the same ticket twice. Fix this, by preventing acquire() and release() from blocking if the current thread has already acquired the ticket. Code sequences like the following can still block in all acquires and releases: ticket->acquire(...) /* Do something */ ticket->release(...) However, code sequences like the following, which used to deadlock if ticket was revoked at just the wrong moment, now succeed: ticket->acquire(...) /* Do something */ ticket->acquire(...) /* This acquire cannot block */ /* Do something */ ticket->release(...) /* This release cannot block */ /* Do something */ ticket->release(...) Without this patch, the inner acquire() and release() calls could block if ticket was revoked at the wrong time. revoke() would not unblock the blocking acquire until there have been as many release()s as acquire()s, which cannot happen. --- src/xine-engine/xine.c | 63 +++++++++++++++++++++++++++++++++++++++-- src/xine-engine/xine_internal.h | 6 ++++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/xine-engine/xine.c b/src/xine-engine/xine.c index 254d596b1..00f1dc41c 100644 --- a/src/xine-engine/xine.c +++ b/src/xine-engine/xine.c @@ -127,17 +127,54 @@ void _x_extra_info_merge( extra_info_t *dst, extra_info_t *src ) { } } +static int acquire_allowed_to_block(xine_ticket_t *this) { + pthread_t own_id = pthread_self(); + unsigned entry; + unsigned new_size; + + for(entry = 0; entry < this->holder_thread_count; ++entry) { + if(this->holder_threads[entry].holder == own_id) { + /* This thread may already hold this ticket */ + this->holder_threads[entry].count++; + return (this->holder_threads[entry].count == 1); + } + } + /* If we get this far, this thread hasn't claimed this ticket before. + We need to give it a new entry in the list, then return true */ + for(entry = 0; entry < this->holder_thread_count; ++entry) { + if(this->holder_threads[entry].count == 0) { + this->holder_threads[entry].holder = own_id; + this->holder_threads[entry].count = 1; + return 1; + } + } + /* List too small. Realloc to larger size */ + new_size = this->holder_thread_count * 2; + lprintf("Reallocing from %d to %d entries\n", this->holder_thread_count, new_size); + + this->holder_threads = realloc(this->holder_threads, sizeof(*this->holder_threads) * new_size); + memset(this->holder_threads + this->holder_thread_count, 0, this->holder_thread_count); + + /* Old size is equivalent to index of first newly allocated entry*/ + this->holder_threads[this->holder_thread_count].count = 1; + this->holder_threads[this->holder_thread_count].holder = own_id; + this->holder_thread_count = new_size; + + return 1; +} + static int ticket_acquire_internal(xine_ticket_t *this, int irrevocable, int nonblocking) { int must_wait = 0; pthread_mutex_lock(&this->lock); + int allowed_to_block = acquire_allowed_to_block(this); if (this->ticket_revoked && !this->irrevocable_tickets) must_wait = !nonblocking; else if (this->atomic_revoke && !pthread_equal(this->atomic_revoker_thread, pthread_self())) must_wait = 1; - if (must_wait) { + if (must_wait && allowed_to_block) { if (nonblocking) { pthread_mutex_unlock(&this->lock); return 0; @@ -162,9 +199,25 @@ static void ticket_acquire(xine_ticket_t *this, int irrevocable) { ticket_acquire_internal(this, irrevocable, 0); } +static int release_allowed_to_block(xine_ticket_t *this) { + pthread_t own_id = pthread_self(); + unsigned entry; + + for(entry = 0; entry < this->holder_thread_count; ++entry) { + if(this->holder_threads[entry].holder == own_id) { + this->holder_threads[entry].count--; + return this->holder_threads[entry].count == 0; + } + } + lprintf("BUG! Ticket 0x%p released by a thread that never took it! Allowing code to continue\n", this); + _x_assert(0); + return 1; +} + static void ticket_release_internal(xine_ticket_t *this, int irrevocable, int nonblocking) { pthread_mutex_lock(&this->lock); + int allowed_to_block = release_allowed_to_block(this); this->tickets_granted--; if (irrevocable) @@ -172,8 +225,10 @@ static void ticket_release_internal(xine_ticket_t *this, int irrevocable, int no if (this->ticket_revoked && !this->tickets_granted) pthread_cond_broadcast(&this->revoked); - if (this->ticket_revoked && !this->irrevocable_tickets && !nonblocking) - pthread_cond_wait(&this->issued, &this->lock); + if (allowed_to_block) { + if (this->ticket_revoked && !this->irrevocable_tickets && !nonblocking) + pthread_cond_wait(&this->issued, &this->lock); + } pthread_mutex_unlock(&this->lock); } @@ -262,6 +317,8 @@ static xine_ticket_t *ticket_init(void) { port_ticket->issue = ticket_issue; port_ticket->revoke = ticket_revoke; port_ticket->dispose = ticket_dispose; + port_ticket->holder_thread_count = XINE_MAX_TICKET_HOLDER_THREADS; + port_ticket->holder_threads = calloc(XINE_MAX_TICKET_HOLDER_THREADS,sizeof(*port_ticket->holder_threads)); pthread_mutex_init(&port_ticket->lock, NULL); pthread_mutex_init(&port_ticket->revoke_lock, NULL); diff --git a/src/xine-engine/xine_internal.h b/src/xine-engine/xine_internal.h index 5f9a82f97..5523001ca 100644 --- a/src/xine-engine/xine_internal.h +++ b/src/xine-engine/xine_internal.h @@ -75,6 +75,7 @@ extern "C" { #define XINE_MAX_EVENT_LISTENERS 50 #define XINE_MAX_EVENT_TYPES 100 +#define XINE_MAX_TICKET_HOLDER_THREADS 64 /* used by plugin loader */ #define XINE_VERSION_CODE XINE_MAJOR_VERSION*10000+XINE_MINOR_VERSION*100+XINE_SUB_VERSION @@ -179,6 +180,11 @@ struct xine_ticket_s { int pending_revocations; int atomic_revoke; pthread_t atomic_revoker_thread; + struct { + int count; + pthread_t holder; + } *holder_threads; + unsigned holder_thread_count; #endif }; -- cgit v1.2.3