diff mbox

[PATCHv3] linux-generic: pools: switch to simple locks for buf/blk synchronization

Message ID 1424878827-31541-1-git-send-email-bill.fischofer@linaro.org
State Accepted
Commit c0251a13eddbfb42212f276a3d18c8e343f38f3b
Headers show

Commit Message

Bill Fischofer Feb. 25, 2015, 3:40 p.m. UTC
Resolve ABA issue with a simple use of locks. The performance hit is
negligible due to the existing use of local buffer caching.

Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
---
 platform/linux-generic/include/odp_pool_internal.h | 95 +++++++---------------
 platform/linux-generic/odp_pool.c                  |  8 +-
 2 files changed, 35 insertions(+), 68 deletions(-)

Comments

Maxim Uvarov Feb. 26, 2015, 11:11 a.m. UTC | #1
Merged,
Maxim.

On 02/26/2015 12:25 PM, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> Reviewed-by: Petri Savolainen <petri.savolainen@linaro.org>
>
>
>> -----Original Message-----
>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>> bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer
>> Sent: Wednesday, February 25, 2015 5:40 PM
>> To: lng-odp@lists.linaro.org
>> Subject: [lng-odp] [PATCHv3] linux-generic: pools: switch to simple locks
>> for buf/blk synchronization
>>
>> Resolve ABA issue with a simple use of locks. The performance hit is
>> negligible due to the existing use of local buffer caching.
>>
>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
>> ---
>>   platform/linux-generic/include/odp_pool_internal.h | 95 +++++++----------
>> -----
>>   platform/linux-generic/odp_pool.c                  |  8 +-
>>   2 files changed, 35 insertions(+), 68 deletions(-)
>>
>> diff --git a/platform/linux-generic/include/odp_pool_internal.h
>> b/platform/linux-generic/include/odp_pool_internal.h
>> index 1b7906f..feeb284 100644
>> --- a/platform/linux-generic/include/odp_pool_internal.h
>> +++ b/platform/linux-generic/include/odp_pool_internal.h
>> @@ -76,8 +76,12 @@ typedef struct local_cache_t {
>>   struct pool_entry_s {
>>   #ifdef POOL_USE_TICKETLOCK
>>   	odp_ticketlock_t        lock ODP_ALIGNED_CACHE;
>> +	odp_ticketlock_t        buf_lock;
>> +	odp_ticketlock_t        blk_lock;
>>   #else
>>   	odp_spinlock_t          lock ODP_ALIGNED_CACHE;
>> +	odp_spinlock_t          buf_lock;
>> +	odp_spinlock_t          blk_lock;
>>   #endif
>>
>>   	char                    name[ODP_POOL_NAME_LEN];
>> @@ -103,8 +107,8 @@ struct pool_entry_s {
>>   	size_t                  pool_size;
>>   	uint32_t                buf_align;
>>   	uint32_t                buf_stride;
>> -	_odp_atomic_ptr_t       buf_freelist;
>> -	_odp_atomic_ptr_t       blk_freelist;
>> +	odp_buffer_hdr_t       *buf_freelist;
>> +	void                   *blk_freelist;
>>   	odp_atomic_u32_t        bufcount;
>>   	odp_atomic_u32_t        blkcount;
>>   	odp_atomic_u64_t        bufallocs;
>> @@ -140,58 +144,33 @@ extern void *pool_entry_ptr[];
>>   #define pool_is_secure(pool) 0
>>   #endif
>>
>> -#define TAG_ALIGN ((size_t)16)
>> -
>> -#define odp_cs(ptr, old, new) \
>> -	_odp_atomic_ptr_cmp_xchg_strong(&ptr, (void **)&old, (void *)new, \
>> -					_ODP_MEMMODEL_SC, \
>> -					_ODP_MEMMODEL_SC)
>> -
>> -/* Helper functions for pointer tagging to avoid ABA race conditions */
>> -#define odp_tag(ptr) \
>> -	(((size_t)ptr) & (TAG_ALIGN - 1))
>> -
>> -#define odp_detag(ptr) \
>> -	((void *)(((size_t)ptr) & -TAG_ALIGN))
>> -
>> -#define odp_retag(ptr, tag) \
>> -	((void *)(((size_t)ptr) | odp_tag(tag)))
>> -
>> -
>>   static inline void *get_blk(struct pool_entry_s *pool)
>>   {
>> -	void *oldhead, *myhead, *newhead;
>> -
>> -	oldhead = _odp_atomic_ptr_load(&pool->blk_freelist,
>> _ODP_MEMMODEL_ACQ);
>> +	void *myhead;
>> +	POOL_LOCK(&pool->blk_lock);
>>
>> -	do {
>> -		size_t tag = odp_tag(oldhead);
>> -		myhead = odp_detag(oldhead);
>> -		if (odp_unlikely(myhead == NULL))
>> -			break;
>> -		newhead = odp_retag(((odp_buf_blk_t *)myhead)->next, tag + 1);
>> -	} while (odp_cs(pool->blk_freelist, oldhead, newhead) == 0);
>> +	myhead = pool->blk_freelist;
>>
>> -	if (odp_unlikely(myhead == NULL))
>> +	if (odp_unlikely(myhead == NULL)) {
>> +		POOL_UNLOCK(&pool->blk_lock);
>>   		odp_atomic_inc_u64(&pool->blkempty);
>> -	else
>> +	} else {
>> +		pool->blk_freelist = ((odp_buf_blk_t *)myhead)->next;
>> +		POOL_UNLOCK(&pool->blk_lock);
>>   		odp_atomic_dec_u32(&pool->blkcount);
>> +	}
>>
>> -	return (void *)myhead;
>> +	return myhead;
>>   }
>>
>>   static inline void ret_blk(struct pool_entry_s *pool, void *block)
>>   {
>> -	void *oldhead, *myhead, *myblock;
>> +	POOL_LOCK(&pool->blk_lock);
>>
>> -	oldhead = _odp_atomic_ptr_load(&pool->blk_freelist,
>> _ODP_MEMMODEL_ACQ);
>> +	((odp_buf_blk_t *)block)->next = pool->blk_freelist;
>> +	pool->blk_freelist = block;
>>
>> -	do {
>> -		size_t tag = odp_tag(oldhead);
>> -		myhead = odp_detag(oldhead);
>> -		((odp_buf_blk_t *)block)->next = myhead;
>> -		myblock = odp_retag(block, tag + 1);
>> -	} while (odp_cs(pool->blk_freelist, oldhead, myblock) == 0);
>> +	POOL_UNLOCK(&pool->blk_lock);
>>
>>   	odp_atomic_inc_u32(&pool->blkcount);
>>   	odp_atomic_inc_u64(&pool->blkfrees);
>> @@ -199,21 +178,17 @@ static inline void ret_blk(struct pool_entry_s
>> *pool, void *block)
>>
>>   static inline odp_buffer_hdr_t *get_buf(struct pool_entry_s *pool)
>>   {
>> -	odp_buffer_hdr_t *oldhead, *myhead, *newhead;
>> +	odp_buffer_hdr_t *myhead;
>> +	POOL_LOCK(&pool->buf_lock);
>>
>> -	oldhead = _odp_atomic_ptr_load(&pool->buf_freelist,
>> _ODP_MEMMODEL_ACQ);
>> -
>> -	do {
>> -		size_t tag = odp_tag(oldhead);
>> -		myhead = odp_detag(oldhead);
>> -		if (odp_unlikely(myhead == NULL))
>> -			break;
>> -		newhead = odp_retag(myhead->next, tag + 1);
>> -	} while (odp_cs(pool->buf_freelist, oldhead, newhead) == 0);
>> +	myhead = pool->buf_freelist;
>>
>>   	if (odp_unlikely(myhead == NULL)) {
>> +		POOL_UNLOCK(&pool->buf_lock);
>>   		odp_atomic_inc_u64(&pool->bufempty);
>>   	} else {
>> +		pool->buf_freelist = myhead->next;
>> +		POOL_UNLOCK(&pool->buf_lock);
>>   		uint64_t bufcount =
>>   			odp_atomic_fetch_sub_u32(&pool->bufcount, 1) - 1;
>>
>> @@ -224,7 +199,6 @@ static inline odp_buffer_hdr_t *get_buf(struct
>> pool_entry_s *pool)
>>   		}
>>
>>   		odp_atomic_inc_u64(&pool->bufallocs);
>> -		myhead->next = myhead;  /* Mark buffer allocated */
>>   		myhead->allocator = odp_thread_id();
>>   	}
>>
>> @@ -233,10 +207,6 @@ static inline odp_buffer_hdr_t *get_buf(struct
>> pool_entry_s *pool)
>>
>>   static inline void ret_buf(struct pool_entry_s *pool, odp_buffer_hdr_t
>> *buf)
>>   {
>> -	odp_buffer_hdr_t *oldhead, *myhead, *mybuf;
>> -
>> -	buf->allocator = ODP_FREEBUF;  /* Mark buffer free */
>> -
>>   	if (!buf->flags.hdrdata && buf->type != ODP_EVENT_BUFFER) {
>>   		while (buf->segcount > 0) {
>>   			if (buffer_is_secure(buf) || pool_is_secure(pool))
>> @@ -247,14 +217,11 @@ static inline void ret_buf(struct pool_entry_s
>> *pool, odp_buffer_hdr_t *buf)
>>   		buf->size = 0;
>>   	}
>>
>> -	oldhead = _odp_atomic_ptr_load(&pool->buf_freelist,
>> _ODP_MEMMODEL_ACQ);
>> -
>> -	do {
>> -		size_t tag = odp_tag(oldhead);
>> -		myhead = odp_detag(oldhead);
>> -		buf->next = myhead;
>> -		mybuf = odp_retag(buf, tag + 1);
>> -	} while (odp_cs(pool->buf_freelist, oldhead, mybuf) == 0);
>> +	buf->allocator = ODP_FREEBUF;  /* Mark buffer free */
>> +	POOL_LOCK(&pool->buf_lock);
>> +	buf->next = pool->buf_freelist;
>> +	pool->buf_freelist = buf;
>> +	POOL_UNLOCK(&pool->buf_lock);
>>
>>   	uint64_t bufcount = odp_atomic_fetch_add_u32(&pool->bufcount, 1) +
>> 1;
>>
>> diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-
>> generic/odp_pool.c
>> index ef7d7ec..cbe3fcb 100644
>> --- a/platform/linux-generic/odp_pool.c
>> +++ b/platform/linux-generic/odp_pool.c
>> @@ -83,6 +83,8 @@ int odp_pool_init_global(void)
>>   		/* init locks */
>>   		pool_entry_t *pool = &pool_tbl->pool[i];
>>   		POOL_LOCK_INIT(&pool->s.lock);
>> +		POOL_LOCK_INIT(&pool->s.buf_lock);
>> +		POOL_LOCK_INIT(&pool->s.blk_lock);
>>   		pool->s.pool_hdl = pool_index_to_handle(i);
>>   		pool->s.pool_id = i;
>>   		pool_entry_ptr[i] = pool;
>> @@ -336,10 +338,8 @@ odp_pool_t odp_pool_create(const char *name,
>>   		pool->s.pool_mdata_addr = mdata_base_addr;
>>
>>   		pool->s.buf_stride = buf_stride;
>> -		_odp_atomic_ptr_store(&pool->s.buf_freelist, NULL,
>> -				      _ODP_MEMMODEL_RLX);
>> -		_odp_atomic_ptr_store(&pool->s.blk_freelist, NULL,
>> -				      _ODP_MEMMODEL_RLX);
>> +		pool->s.buf_freelist = NULL;
>> +		pool->s.blk_freelist = NULL;
>>
>>   		/* Initialization will increment these to their target vals */
>>   		odp_atomic_store_u32(&pool->s.bufcount, 0);
>> --
>> 2.1.0
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
diff mbox

Patch

diff --git a/platform/linux-generic/include/odp_pool_internal.h b/platform/linux-generic/include/odp_pool_internal.h
index 1b7906f..feeb284 100644
--- a/platform/linux-generic/include/odp_pool_internal.h
+++ b/platform/linux-generic/include/odp_pool_internal.h
@@ -76,8 +76,12 @@  typedef struct local_cache_t {
 struct pool_entry_s {
 #ifdef POOL_USE_TICKETLOCK
 	odp_ticketlock_t        lock ODP_ALIGNED_CACHE;
+	odp_ticketlock_t        buf_lock;
+	odp_ticketlock_t        blk_lock;
 #else
 	odp_spinlock_t          lock ODP_ALIGNED_CACHE;
+	odp_spinlock_t          buf_lock;
+	odp_spinlock_t          blk_lock;
 #endif
 
 	char                    name[ODP_POOL_NAME_LEN];
@@ -103,8 +107,8 @@  struct pool_entry_s {
 	size_t                  pool_size;
 	uint32_t                buf_align;
 	uint32_t                buf_stride;
-	_odp_atomic_ptr_t       buf_freelist;
-	_odp_atomic_ptr_t       blk_freelist;
+	odp_buffer_hdr_t       *buf_freelist;
+	void                   *blk_freelist;
 	odp_atomic_u32_t        bufcount;
 	odp_atomic_u32_t        blkcount;
 	odp_atomic_u64_t        bufallocs;
@@ -140,58 +144,33 @@  extern void *pool_entry_ptr[];
 #define pool_is_secure(pool) 0
 #endif
 
-#define TAG_ALIGN ((size_t)16)
-
-#define odp_cs(ptr, old, new) \
-	_odp_atomic_ptr_cmp_xchg_strong(&ptr, (void **)&old, (void *)new, \
-					_ODP_MEMMODEL_SC, \
-					_ODP_MEMMODEL_SC)
-
-/* Helper functions for pointer tagging to avoid ABA race conditions */
-#define odp_tag(ptr) \
-	(((size_t)ptr) & (TAG_ALIGN - 1))
-
-#define odp_detag(ptr) \
-	((void *)(((size_t)ptr) & -TAG_ALIGN))
-
-#define odp_retag(ptr, tag) \
-	((void *)(((size_t)ptr) | odp_tag(tag)))
-
-
 static inline void *get_blk(struct pool_entry_s *pool)
 {
-	void *oldhead, *myhead, *newhead;
-
-	oldhead = _odp_atomic_ptr_load(&pool->blk_freelist, _ODP_MEMMODEL_ACQ);
+	void *myhead;
+	POOL_LOCK(&pool->blk_lock);
 
-	do {
-		size_t tag = odp_tag(oldhead);
-		myhead = odp_detag(oldhead);
-		if (odp_unlikely(myhead == NULL))
-			break;
-		newhead = odp_retag(((odp_buf_blk_t *)myhead)->next, tag + 1);
-	} while (odp_cs(pool->blk_freelist, oldhead, newhead) == 0);
+	myhead = pool->blk_freelist;
 
-	if (odp_unlikely(myhead == NULL))
+	if (odp_unlikely(myhead == NULL)) {
+		POOL_UNLOCK(&pool->blk_lock);
 		odp_atomic_inc_u64(&pool->blkempty);
-	else
+	} else {
+		pool->blk_freelist = ((odp_buf_blk_t *)myhead)->next;
+		POOL_UNLOCK(&pool->blk_lock);
 		odp_atomic_dec_u32(&pool->blkcount);
+	}
 
-	return (void *)myhead;
+	return myhead;
 }
 
 static inline void ret_blk(struct pool_entry_s *pool, void *block)
 {
-	void *oldhead, *myhead, *myblock;
+	POOL_LOCK(&pool->blk_lock);
 
-	oldhead = _odp_atomic_ptr_load(&pool->blk_freelist, _ODP_MEMMODEL_ACQ);
+	((odp_buf_blk_t *)block)->next = pool->blk_freelist;
+	pool->blk_freelist = block;
 
-	do {
-		size_t tag = odp_tag(oldhead);
-		myhead = odp_detag(oldhead);
-		((odp_buf_blk_t *)block)->next = myhead;
-		myblock = odp_retag(block, tag + 1);
-	} while (odp_cs(pool->blk_freelist, oldhead, myblock) == 0);
+	POOL_UNLOCK(&pool->blk_lock);
 
 	odp_atomic_inc_u32(&pool->blkcount);
 	odp_atomic_inc_u64(&pool->blkfrees);
@@ -199,21 +178,17 @@  static inline void ret_blk(struct pool_entry_s *pool, void *block)
 
 static inline odp_buffer_hdr_t *get_buf(struct pool_entry_s *pool)
 {
-	odp_buffer_hdr_t *oldhead, *myhead, *newhead;
+	odp_buffer_hdr_t *myhead;
+	POOL_LOCK(&pool->buf_lock);
 
-	oldhead = _odp_atomic_ptr_load(&pool->buf_freelist, _ODP_MEMMODEL_ACQ);
-
-	do {
-		size_t tag = odp_tag(oldhead);
-		myhead = odp_detag(oldhead);
-		if (odp_unlikely(myhead == NULL))
-			break;
-		newhead = odp_retag(myhead->next, tag + 1);
-	} while (odp_cs(pool->buf_freelist, oldhead, newhead) == 0);
+	myhead = pool->buf_freelist;
 
 	if (odp_unlikely(myhead == NULL)) {
+		POOL_UNLOCK(&pool->buf_lock);
 		odp_atomic_inc_u64(&pool->bufempty);
 	} else {
+		pool->buf_freelist = myhead->next;
+		POOL_UNLOCK(&pool->buf_lock);
 		uint64_t bufcount =
 			odp_atomic_fetch_sub_u32(&pool->bufcount, 1) - 1;
 
@@ -224,7 +199,6 @@  static inline odp_buffer_hdr_t *get_buf(struct pool_entry_s *pool)
 		}
 
 		odp_atomic_inc_u64(&pool->bufallocs);
-		myhead->next = myhead;  /* Mark buffer allocated */
 		myhead->allocator = odp_thread_id();
 	}
 
@@ -233,10 +207,6 @@  static inline odp_buffer_hdr_t *get_buf(struct pool_entry_s *pool)
 
 static inline void ret_buf(struct pool_entry_s *pool, odp_buffer_hdr_t *buf)
 {
-	odp_buffer_hdr_t *oldhead, *myhead, *mybuf;
-
-	buf->allocator = ODP_FREEBUF;  /* Mark buffer free */
-
 	if (!buf->flags.hdrdata && buf->type != ODP_EVENT_BUFFER) {
 		while (buf->segcount > 0) {
 			if (buffer_is_secure(buf) || pool_is_secure(pool))
@@ -247,14 +217,11 @@  static inline void ret_buf(struct pool_entry_s *pool, odp_buffer_hdr_t *buf)
 		buf->size = 0;
 	}
 
-	oldhead = _odp_atomic_ptr_load(&pool->buf_freelist, _ODP_MEMMODEL_ACQ);
-
-	do {
-		size_t tag = odp_tag(oldhead);
-		myhead = odp_detag(oldhead);
-		buf->next = myhead;
-		mybuf = odp_retag(buf, tag + 1);
-	} while (odp_cs(pool->buf_freelist, oldhead, mybuf) == 0);
+	buf->allocator = ODP_FREEBUF;  /* Mark buffer free */
+	POOL_LOCK(&pool->buf_lock);
+	buf->next = pool->buf_freelist;
+	pool->buf_freelist = buf;
+	POOL_UNLOCK(&pool->buf_lock);
 
 	uint64_t bufcount = odp_atomic_fetch_add_u32(&pool->bufcount, 1) + 1;
 
diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c
index ef7d7ec..cbe3fcb 100644
--- a/platform/linux-generic/odp_pool.c
+++ b/platform/linux-generic/odp_pool.c
@@ -83,6 +83,8 @@  int odp_pool_init_global(void)
 		/* init locks */
 		pool_entry_t *pool = &pool_tbl->pool[i];
 		POOL_LOCK_INIT(&pool->s.lock);
+		POOL_LOCK_INIT(&pool->s.buf_lock);
+		POOL_LOCK_INIT(&pool->s.blk_lock);
 		pool->s.pool_hdl = pool_index_to_handle(i);
 		pool->s.pool_id = i;
 		pool_entry_ptr[i] = pool;
@@ -336,10 +338,8 @@  odp_pool_t odp_pool_create(const char *name,
 		pool->s.pool_mdata_addr = mdata_base_addr;
 
 		pool->s.buf_stride = buf_stride;
-		_odp_atomic_ptr_store(&pool->s.buf_freelist, NULL,
-				      _ODP_MEMMODEL_RLX);
-		_odp_atomic_ptr_store(&pool->s.blk_freelist, NULL,
-				      _ODP_MEMMODEL_RLX);
+		pool->s.buf_freelist = NULL;
+		pool->s.blk_freelist = NULL;
 
 		/* Initialization will increment these to their target vals */
 		odp_atomic_store_u32(&pool->s.bufcount, 0);