Re: [FFmpeg-devel] [PATCH 3/3] avutil/threadmessage: fix condition broadcasting
On Wed, Dec 02, 2015 at 03:57:31PM +0100, Clément Bœsch wrote: > From: Clément Bœsch > > Fix a dead lock under certain conditions. Let's assume we have a queue of 1 > message max, 2 senders, and 1 receiver. [...] Pushed with commit message adjusted -- Clément B. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avutil/threadmessage: fix condition broadcasting
Le quintidi 15 frimaire, an CCXXIV, Clement Boesch a écrit : > This was before providing the 2 cond version, but OK Sorry, missed that. Patch looks good to me, but you forgot to update the commit message. > I'd like to push the test as well, but it depends on the first patch > (flush func). I clarified a bit the use of the flush in the doxy. Are you > still uncomfortable about this one? I will look at it soon. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avutil/threadmessage: fix condition broadcasting
On Sat, Dec 05, 2015 at 01:19:16PM +0100, Nicolas George wrote: > Le quintidi 15 frimaire, an CCXXIV, Clement Boesch a écrit : > > ping, I'd like to apply this fix soon > > I have already answered: > > http://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/184285.html > This was before providing the 2 cond version, but OK > So please go ahead. > I'd like to push the test as well, but it depends on the first patch (flush func). I clarified a bit the use of the flush in the doxy. Are you still uncomfortable about this one? -- Clément B. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avutil/threadmessage: fix condition broadcasting
Le quintidi 15 frimaire, an CCXXIV, Clement Boesch a écrit : > ping, I'd like to apply this fix soon I have already answered: http://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/184285.html So please go ahead. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] avutil/threadmessage: fix condition broadcasting
On Wed, Dec 02, 2015 at 03:57:31PM +0100, Clément Bœsch wrote: [...] > This second solution replaces the condition with two: one to notify the > senders, and one to notify the receivers. This prevents senders from > notifying other senders instead of a reader, and the other way around. > It also avoid broadcasting to everyone like the first solution, and is, > as a result in theory more optimized. > --- > libavutil/threadmessage.c | 35 --- > 1 file changed, 24 insertions(+), 11 deletions(-) > ping, I'd like to apply this fix soon [...] -- Clément B. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/3] avutil/threadmessage: fix condition broadcasting
From: Clément Bœsch Fix a dead lock under certain conditions. Let's assume we have a queue of 1 message max, 2 senders, and 1 receiver. Scenario (real record obtained with debug added): [...] SENDER #0: acquired lock SENDER #0: queue is full, wait SENDER #1: acquired lock SENDER #1: queue is full, wait RECEIVER: acquired lock RECEIVER: reading a msg from the queue RECEIVER: signal the cond RECEIVER: acquired lock RECEIVER: queue is empty, wait SENDER #0: writing a msg the queue SENDER #0: signal the cond SENDER #0: acquired lock SENDER #0: queue is full, wait SENDER #1: queue is full, wait Translated: - initially the queue contains 1/1 message with 2 senders blocking on it, waiting to push another message. - Meanwhile the receiver is obtaining the lock, read the message, signal & release the lock. For some reason it is able to acquire the lock again before the signal wakes up one of the sender. Since it just emptied the queue, the reader waits for the queue to fill up again. - The signal finally reaches one of the sender, which writes a message and then signal the condition. Unfortunately, instead of waking up the reader, it actually wakes up the other worker (signal = notify the condition just for 1 waiter), who can't push another message in the queue because it's full. - Meanwhile, the receiver is still waiting. Deadlock. This scenario can be triggered with for example: tests/api/api-threadmessage-test 1 2 100 100 1 1000 1000 One working solution is to make av_thread_message_queue_{send,recv}() call pthread_cond_broadcast() instead of pthread_cond_signal() so both senders and receivers are unlocked when work is done (be it reading or writing). This second solution replaces the condition with two: one to notify the senders, and one to notify the receivers. This prevents senders from notifying other senders instead of a reader, and the other way around. It also avoid broadcasting to everyone like the first solution, and is, as a result in theory more optimized. --- libavutil/threadmessage.c | 35 --- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/libavutil/threadmessage.c b/libavutil/threadmessage.c index 66b5fc6..783855f 100644 --- a/libavutil/threadmessage.c +++ b/libavutil/threadmessage.c @@ -36,7 +36,8 @@ struct AVThreadMessageQueue { #if HAVE_THREADS AVFifoBuffer *fifo; pthread_mutex_t lock; -pthread_cond_t cond; +pthread_cond_t cond_recv; +pthread_cond_t cond_send; int err_send; int err_recv; unsigned elsize; @@ -63,13 +64,20 @@ int av_thread_message_queue_alloc(AVThreadMessageQueue **mq, av_free(rmq); return AVERROR(ret); } -if ((ret = pthread_cond_init(&rmq->cond, NULL))) { +if ((ret = pthread_cond_init(&rmq->cond_recv, NULL))) { +pthread_mutex_destroy(&rmq->lock); +av_free(rmq); +return AVERROR(ret); +} +if ((ret = pthread_cond_init(&rmq->cond_send, NULL))) { +pthread_cond_destroy(&rmq->cond_recv); pthread_mutex_destroy(&rmq->lock); av_free(rmq); return AVERROR(ret); } if (!(rmq->fifo = av_fifo_alloc(elsize * nelem))) { -pthread_cond_destroy(&rmq->cond); +pthread_cond_destroy(&rmq->cond_send); +pthread_cond_destroy(&rmq->cond_recv); pthread_mutex_destroy(&rmq->lock); av_free(rmq); return AVERROR(ret); @@ -99,7 +107,8 @@ void av_thread_message_queue_free(AVThreadMessageQueue **mq) av_thread_message_flush(*mq); av_freep(&(*mq)->tmp_msg); av_fifo_freep(&(*mq)->fifo); -pthread_cond_destroy(&(*mq)->cond); +pthread_cond_destroy(&(*mq)->cond_send); +pthread_cond_destroy(&(*mq)->cond_recv); pthread_mutex_destroy(&(*mq)->lock); av_freep(mq); } @@ -115,12 +124,13 @@ static int av_thread_message_queue_send_locked(AVThreadMessageQueue *mq, while (!mq->err_send && av_fifo_space(mq->fifo) < mq->elsize) { if ((flags & AV_THREAD_MESSAGE_NONBLOCK)) return AVERROR(EAGAIN); -pthread_cond_wait(&mq->cond, &mq->lock); +pthread_cond_wait(&mq->cond_send, &mq->lock); } if (mq->err_send) return mq->err_send; av_fifo_generic_write(mq->fifo, msg, mq->elsize, NULL); -pthread_cond_signal(&mq->cond); +/* one message is sent, signal one receiver */ +pthread_cond_signal(&mq->cond_recv); return 0; } @@ -131,12 +141,13 @@ static int av_thread_message_queue_recv_locked(AVThreadMessageQueue *mq, while (!mq->err_recv && av_fifo_size(mq->fifo) < mq->elsize) { if ((flags & AV_THREAD_MESSAGE_NONBLOCK)) return AVERROR(EAGAIN); -pthread_cond_wait(&mq->cond, &mq->lock); +pthread_cond_wait(&mq->cond_recv, &mq->lock); } if (av_fifo_size(mq->fifo) < mq->elsize) return mq->err_recv
[FFmpeg-devel] [PATCH 3/3] avutil/threadmessage: fix condition broadcasting
From: Clément Bœsch Fix a dead lock under certain conditions. Let's assume we have a queue of 1 message max, 2 senders, and 1 receiver. Scenario (real record obtained with debug added): [...] SENDER #0: acquired lock SENDER #0: queue is full, wait SENDER #1: acquired lock SENDER #1: queue is full, wait RECEIVER: acquired lock RECEIVER: reading a msg from the queue RECEIVER: signal the cond RECEIVER: acquired lock RECEIVER: queue is empty, wait SENDER #0: writing a msg the queue SENDER #0: signal the cond SENDER #0: acquired lock SENDER #0: queue is full, wait SENDER #1: queue is full, wait Translated: - initially the queue contains 1/1 message with 2 senders blocking on it, waiting to push another message. - Meanwhile the receiver is obtaining the lock, read the message, signal & release the lock. For some reason it is able to acquire the lock again before the signal wakes up one of the sender. Since it just emptied the queue, the reader waits for the queue to fill up again. - The signal finally reaches one of the sender, which writes a message and then signal the condition. Unfortunately, instead of waking up the reader, it actually wakes up the other worker (signal = notify the condition just for 1 waiter), who can't push another message in the queue because it's full. - Meanwhile, the receiver is still waiting. Deadlock. This scenario can be triggered with for example: tests/api/api-threadmessage-test 2 1 100 100 999 The fix is simply to make sure to notify everyone when work is done (be it reading or writing). Another solution would be to use 2 distincts conditions. --- libavutil/threadmessage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavutil/threadmessage.c b/libavutil/threadmessage.c index 87ce8dc..ec62c2d 100644 --- a/libavutil/threadmessage.c +++ b/libavutil/threadmessage.c @@ -118,7 +118,7 @@ static int av_thread_message_queue_send_locked(AVThreadMessageQueue *mq, if (mq->err_send) return mq->err_send; av_fifo_generic_write(mq->fifo, msg, mq->elsize, NULL); -pthread_cond_signal(&mq->cond); +pthread_cond_broadcast(&mq->cond); return 0; } @@ -134,7 +134,7 @@ static int av_thread_message_queue_recv_locked(AVThreadMessageQueue *mq, if (av_fifo_size(mq->fifo) < mq->elsize) return mq->err_recv; av_fifo_generic_read(mq->fifo, msg, mq->elsize, NULL); -pthread_cond_signal(&mq->cond); +pthread_cond_broadcast(&mq->cond); return 0; } -- 2.6.2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel