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.
Unified · Split
| @@ -23,6 +23,7 @@ · 22 unmodified lines | |||
| 23 | #include "buffer_internal.h" | 23 | #include "buffer_internal.h" |
| 24 | #include "common.h" | 24 | #include "common.h" |
| 25 | #include "mem.h" | 25 | #include "mem.h" |
| 26 | #include "thread.h" | ||
| 26 | 27 | ||
| 27 | AVBufferRef *av_buffer_create(uint8_t *data, int size, | 28 | AVBufferRef *av_buffer_create(uint8_t *data, int size, |
| 28 | void (*free)(void *opaque, uint8_t *data), | 29 | void (*free)(void *opaque, uint8_t *data), |
| @@ -199,6 +200,8 @@ AVBufferPool *av_buffer_pool_init(int size, AVBufferRef* (*alloc)(int size)) · 170 unmodified lines | |||
| 199 | if (!pool) | 200 | if (!pool) |
| 200 | return NULL; | 201 | return NULL; |
| 201 | 202 | ||
| 203 | ff_mutex_init(&pool->mutex, NULL); | ||
| 204 | |||
| 202 | pool->size = size; | 205 | pool->size = size; |
| 203 | pool->alloc = alloc ? alloc : av_buffer_alloc; | 206 | pool->alloc = alloc ? alloc : av_buffer_alloc; |
| 204 | 207 | ||
| @@ -220,6 +223,7 @@ static void buffer_pool_free(AVBufferPool *pool) · 15 unmodified lines | |||
| 220 | buf->free(buf->opaque, buf->data); | 223 | buf->free(buf->opaque, buf->data); |
| 221 | av_freep(&buf); | 224 | av_freep(&buf); |
| 222 | } | 225 | } |
| 226 | ff_mutex_destroy(&pool->mutex); | ||
| 223 | av_freep(&pool); | 227 | av_freep(&pool); |
| 224 | } | 228 | } |
| 225 | 229 | ||
| @@ -236,47 +240,16 @@ void av_buffer_pool_uninit(AVBufferPool **ppool) · 10 unmodified lines | |||
| 236 | buffer_pool_free(pool); | 240 | buffer_pool_free(pool); |
| 237 | } | 241 | } |
| 238 | 242 | ||
| 239 | /* remove the whole buffer list from the pool and return it */ | ||
| 240 | static BufferPoolEntry *get_pool(AVBufferPool *pool) | ||
| 241 | { | ||
| 242 | BufferPoolEntry *cur = NULL, *last = NULL; | ||
| 243 | |||
| 244 | do { | ||
| 245 | FFSWAP(BufferPoolEntry*, cur, last); | ||
| 246 | cur = avpriv_atomic_ptr_cas((void * volatile *)&pool->pool, last, NULL); | ||
| 247 | if (!cur) | ||
| 248 | return NULL; | ||
| 249 | } while (cur != last); | ||
| 250 | |||
| 251 | return cur; | ||
| 252 | } | ||
| 253 | |||
| 254 | static void add_to_pool(BufferPoolEntry *buf) | ||
| 255 | { | ||
| 256 | AVBufferPool *pool; | ||
| 257 | BufferPoolEntry *cur, *end = buf; | ||
| 258 | |||
| 259 | if (!buf) | ||
| 260 | return; | ||
| 261 | pool = buf->pool; | ||
| 262 | |||
| 263 | while (end->next) | ||
| 264 | end = end->next; | ||
| 265 | |||
| 266 | while ((cur = avpriv_atomic_ptr_cas((void * volatile *)&pool->pool, NULL, buf))) { | ||
| 267 | /* pool is not empty, retrieve it and append it to our list */ | ||
| 268 | cur = get_pool(pool); | ||
| 269 | end->next = cur; | ||
| 270 | while (end->next) | ||
| 271 | end = end->next; | ||
| 272 | } | ||
| 273 | } | ||
| 274 | |||
| 275 | static void pool_release_buffer(void *opaque, uint8_t *data) | 243 | static void pool_release_buffer(void *opaque, uint8_t *data) |
| 276 | { | 244 | { |
| 277 | BufferPoolEntry *buf = opaque; | 245 | BufferPoolEntry *buf = opaque; |
| 278 | AVBufferPool *pool = buf->pool; | 246 | AVBufferPool *pool = buf->pool; |
| 279 | add_to_pool(buf); | 247 | |
| 248 | ff_mutex_lock(&pool->mutex); | ||
| 249 | buf->next = pool->pool; | ||
| 250 | pool->pool = buf; | ||
| 251 | ff_mutex_unlock(&pool->mutex); | ||
| 252 | |||
| 280 | if (!avpriv_atomic_int_add_and_fetch(&pool->refcount, -1)) | 253 | if (!avpriv_atomic_int_add_and_fetch(&pool->refcount, -1)) |
| 281 | buffer_pool_free(pool); | 254 | buffer_pool_free(pool); |
| 282 | } | 255 | } |
| @@ -306,8 +279,6 @@ static AVBufferRef *pool_alloc_buffer(AVBufferPool *pool) · 23 unmodified lines | |||
| 306 | ret->buffer->opaque = buf; | 279 | ret->buffer->opaque = buf; |
| 307 | ret->buffer->free = pool_release_buffer; | 280 | ret->buffer->free = pool_release_buffer; |
| 308 | 281 | ||
| 309 | avpriv_atomic_int_add_and_fetch(&pool->refcount, 1); | ||
| 310 | |||
| 311 | return ret; | 282 | return ret; |
| 312 | } | 283 | } |
| 313 | 284 | ||
| @@ -316,22 +287,22 @@ AVBufferRef *av_buffer_pool_get(AVBufferPool *pool) · 2 unmodified lines | |||
| 316 | AVBufferRef *ret; | 287 | AVBufferRef *ret; |
| 317 | BufferPoolEntry *buf; | 288 | BufferPoolEntry *buf; |
| 318 | 289 | ||
| 319 | /* check whether the pool is empty */ | 290 | ff_mutex_lock(&pool->mutex); |
| 320 | buf = get_pool(pool); | 291 | buf = pool->pool; |
| 321 | if (!buf) | 292 | if (buf) { |
| 322 | return pool_alloc_buffer(pool); | 293 | ret = av_buffer_create(buf->data, pool->size, pool_release_buffer, |
| 323 | 294 | buf, 0); | |
| 324 | /* keep the first entry, return the rest of the list to the pool */ | 295 | if (ret) { |
| 325 | add_to_pool(buf->next); | 296 | pool->pool = buf->next; |
| 326 | buf->next = NULL; | 297 | buf->next = NULL; |
| 327 | 298 | } | |
| 328 | ret = av_buffer_create(buf->data, pool->size, pool_release_buffer, | 299 | } else { |
| 329 | buf, 0); | 300 | ret = pool_alloc_buffer(pool); |
| 330 | if (!ret) { | ||
| 331 | add_to_pool(buf); | ||
| 332 | return NULL; | ||
| 333 | } | 301 | } |
| 334 | avpriv_atomic_int_add_and_fetch(&pool->refcount, 1); | 302 | ff_mutex_unlock(&pool->mutex); |
| 303 | |||
| 304 | if (ret) | ||
| 305 | avpriv_atomic_int_add_and_fetch(&pool->refcount, 1); | ||
| 335 | 306 | ||
| 336 | return ret; | 307 | return ret; |
| 337 | } | 308 | } |
| @@ -22,6 +22,7 @@ · 21 unmodified lines | |||
| 22 | #include <stdint.h> | 22 | #include <stdint.h> |
| 23 | 23 | ||
| 24 | #include "buffer.h" | 24 | #include "buffer.h" |
| 25 | #include "thread.h" | ||
| 25 | 26 | ||
| 26 | /** | 27 | /** |
| 27 | * The buffer is always treated as read-only. | 28 | * The buffer is always treated as read-only. |
| @@ -68,11 +69,12 @@ typedef struct BufferPoolEntry { · 40 unmodified lines | |||
| 68 | void (*free)(void *opaque, uint8_t *data); | 69 | void (*free)(void *opaque, uint8_t *data); |
| 69 | 70 | ||
| 70 | AVBufferPool *pool; | 71 | AVBufferPool *pool; |
| 71 | struct BufferPoolEntry * volatile next; | 72 | struct BufferPoolEntry *next; |
| 72 | } BufferPoolEntry; | 73 | } BufferPoolEntry; |
| 73 | 74 | ||
| 74 | struct AVBufferPool { | 75 | struct AVBufferPool { |
| 75 | BufferPoolEntry * volatile pool; | 76 | AVMutex mutex; |
| 77 | BufferPoolEntry *pool; | ||
| 76 | 78 | ||
| 77 | /* | 79 | /* |
| 78 | * This is used to track when the pool is to be freed. | 80 | * This is used to track when the pool is to be freed. |