On Fri, 14 Nov 2014 11:38:09 +0100
Anton Khirnov <an...@khirnov.net> wrote:
> Quoting wm4 (2014-11-11 17:26:47)
> > The buffer pool has to atomically add and remove entries from the linked
> > list of available buffers. This was done by removing the entire list
> > with a CAS operation, working on it, and then setting it back again
> > (using a retry-loop in case another thread was doing the same thing).
> >
> > This could effectively cause memory leaks: while a thread was working on
> > the buffer list, other threads would allocate new buffers, increasing
> > the pool's total size. There was no real leak, but since these extra
> > buffers were not needed, but not free'd either (except when the buffer
> > pool was destroyed), this had the same effects as a real leak. For some
> > reason, growth was exponential, and could easily kill the process due
> > to OOM in real-world uses.
> >
> > Fix this by using a mutex to protect the list operations. The fancy
> > way atomics remove the whole list to work on it is not needed anymore,
> > which also avoids the situation which was causing the leak.
> >
> > Unlike with atomics, there are no pthread stubs for the case when
> > pthreads are unavailable, so the buffer code defines some manually.
> > ---
> > libavutil/buffer.c | 73
> > +++++++++++++--------------------------------
> > libavutil/buffer_internal.h | 31 +++++++++++++++++--
> > 2 files changed, 49 insertions(+), 55 deletions(-)
> >
> > diff --git a/libavutil/buffer.c b/libavutil/buffer.c
> > index 2b38081..6f8fe85 100644
> > --- a/libavutil/buffer.c
> > +++ b/libavutil/buffer.c
> > @@ -199,6 +199,8 @@ AVBufferPool *av_buffer_pool_init(int size,
> > AVBufferRef* (*alloc)(int size))
> > if (!pool)
> > return NULL;
> >
> > + pthread_mutex_init(&pool->mutex, NULL);
> > +
> > pool->size = size;
> > pool->alloc = alloc ? alloc : av_buffer_alloc;
> >
> > @@ -220,6 +222,7 @@ static void buffer_pool_free(AVBufferPool *pool)
> > buf->free(buf->opaque, buf->data);
> > av_freep(&buf);
> > }
> > + pthread_mutex_destroy(&pool->mutex);
> > av_freep(&pool);
> > }
> >
> > @@ -236,47 +239,16 @@ void av_buffer_pool_uninit(AVBufferPool **ppool)
> > buffer_pool_free(pool);
> > }
> >
> > -/* remove the whole buffer list from the pool and return it */
> > -static BufferPoolEntry *get_pool(AVBufferPool *pool)
> > -{
> > - BufferPoolEntry *cur = NULL, *last = NULL;
> > -
> > - do {
> > - FFSWAP(BufferPoolEntry*, cur, last);
> > - cur = avpriv_atomic_ptr_cas((void * volatile *)&pool->pool, last,
> > NULL);
> > - if (!cur)
> > - return NULL;
> > - } while (cur != last);
> > -
> > - return cur;
> > -}
> > -
> > -static void add_to_pool(BufferPoolEntry *buf)
> > -{
> > - AVBufferPool *pool;
> > - BufferPoolEntry *cur, *end = buf;
> > -
> > - if (!buf)
> > - return;
> > - pool = buf->pool;
> > -
> > - while (end->next)
> > - end = end->next;
> > -
> > - while ((cur = avpriv_atomic_ptr_cas((void * volatile *)&pool->pool,
> > NULL, buf))) {
> > - /* pool is not empty, retrieve it and append it to our list */
> > - cur = get_pool(pool);
> > - end->next = cur;
> > - while (end->next)
> > - end = end->next;
> > - }
> > -}
> > -
> > static void pool_release_buffer(void *opaque, uint8_t *data)
> > {
> > BufferPoolEntry *buf = opaque;
> > AVBufferPool *pool = buf->pool;
> > - add_to_pool(buf);
> > +
> > + pthread_mutex_lock(&pool->mutex);
> > + buf->next = pool->pool;
> > + pool->pool = buf;
> > + pthread_mutex_unlock(&pool->mutex);
> > +
> > if (!avpriv_atomic_int_add_and_fetch(&pool->refcount, -1))
> > buffer_pool_free(pool);
> > }
> > @@ -306,8 +278,6 @@ static AVBufferRef *pool_alloc_buffer(AVBufferPool
> > *pool)
> > ret->buffer->opaque = buf;
> > ret->buffer->free = pool_release_buffer;
> >
> > - avpriv_atomic_int_add_and_fetch(&pool->refcount, 1);
> > -
> > return ret;
> > }
> >
> > @@ -316,21 +286,18 @@ AVBufferRef *av_buffer_pool_get(AVBufferPool *pool)
> > AVBufferRef *ret;
> > BufferPoolEntry *buf;
> >
> > - /* check whether the pool is empty */
> > - buf = get_pool(pool);
> > - if (!buf)
> > - return pool_alloc_buffer(pool);
> > -
> > - /* keep the first entry, return the rest of the list to the pool */
> > - add_to_pool(buf->next);
> > - buf->next = NULL;
> > -
> > - ret = av_buffer_create(buf->data, pool->size, pool_release_buffer,
> > - buf, 0);
> > - if (!ret) {
> > - add_to_pool(buf);
> > - return NULL;
> > + pthread_mutex_lock(&pool->mutex);
> > + buf = pool->pool;
> > + if (buf) {
> > + pool->pool = buf->next;
> > + buf->next = NULL;
> > + ret = av_buffer_create(buf->data, pool->size, pool_release_buffer,
> > + buf, 0);
> > + } else {
> > + ret = pool_alloc_buffer(pool);
> > }
> > + pthread_mutex_unlock(&pool->mutex);
> > +
> > avpriv_atomic_int_add_and_fetch(&pool->refcount, 1);
> >
> > return ret;
> > diff --git a/libavutil/buffer_internal.h b/libavutil/buffer_internal.h
> > index cce83c3..9a2e6cb 100644
> > --- a/libavutil/buffer_internal.h
> > +++ b/libavutil/buffer_internal.h
> > @@ -21,6 +21,32 @@
> >
> > #include <stdint.h>
> >
> > +#if HAVE_PTHREADS
> > +#include <pthread.h>
> > +#elif HAVE_W32THREADS
> > +#include "compat/w32pthreads.h"
> > +#else
> > +typedef struct pthread_mutex_t {
> > + char dummy;
> > +} pthread_mutex_t;
> > +static inline int pthread_mutex_init(pthread_mutex_t *m, const void
> > *unused)
> > +{
> > + return 0;
> > +}
> > +static inline int pthread_mutex_lock(pthread_mutex_t *m)
> > +{
> > + return 0;
> > +}
> > +static inline int pthread_mutex_unlock(pthread_mutex_t *m)
> > +{
> > + return 0;
> > +}
> > +static inline int pthread_mutex_destroy(pthread_mutex_t *m)
> > +{
> > + return 0;
> > +}
> > +#endif
>
> Thank you for debugging this.
>
> The patch look mostly fine, the only part I'm not sure of is this.
>
> One point is that you have to call w32thread_init() with win32 threads.
>
> Another is that this header is only included in buffer.c, but in the
> future someone might include it somewhere else and miss the fact that
> he's making copies of those fallbacks.
These stubs sure are pretty ugly, but it's still better than adding
ifdeffery to the actual code. Maybe the fallbacks should be somewhere
else? (The atomics code does the same.)
I don't think the copies matter. They compile to nothing.
compat/w32pthreads.h also consists of inline functions only.
And also, compiling with no threads is a fringe configuration anyway.
> Perhaps it'd be better to handle this on the configure level -- i.e.
> provide pthread.h replacement automagically without having to handle it
> in each file? I see w32thread_init only does something on old windows
> versions -- which ones are those exactly? Do we still care about them?
Looking at w32thread_init(), it's only needed for condition variables,
and it's done only done for compatibility with older windows versions
(I'm not sure, but my guess is for before Vista). So calling the
function shouldn't be needed in this case.
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
