[dpdk-dev] [PATCH 1/2] lib/timer: protect timer subsystem initialized with lock

[dpdk-dev] [PATCH 1/2] lib/timer: protect timer subsystem initialized with lock

[Phil Yang]

From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

rte_timer_subsystem_initialized is a global variable that can be
accessed by multiple processes simultaneously. Hence, any access
to rte_timer_subsystem_initialized should be protected by
rte_mcfg_timer_lock.

Fixes: f9d6cd8bfe9e ("timer: fix resource leak in finalize")
Cc: stable@dpdk.org

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
 lib/librte_timer/rte_timer.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 89f2707..269e921 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -145,11 +145,13 @@ rte_timer_subsystem_init(void)
 	const size_t mem_size = data_arr_size + sizeof(*rte_timer_mz_refcnt);
 	bool do_full_init = true;
 
-	if (rte_timer_subsystem_initialized)
-		return -EALREADY;
-
 	rte_mcfg_timer_lock();
 
+	if (rte_timer_subsystem_initialized) {
+		rte_mcfg_timer_unlock();
+		return -EALREADY;
+	}
+
 	mz = rte_memzone_lookup(mz_name);
 	if (mz == NULL) {
 		mz = rte_memzone_reserve_aligned(mz_name, mem_size,
@@ -183,27 +185,29 @@ rte_timer_subsystem_init(void)
 	rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
 	(*rte_timer_mz_refcnt)++;
 
-	rte_mcfg_timer_unlock();
-
 	rte_timer_subsystem_initialized = 1;
 
+	rte_mcfg_timer_unlock();
+
 	return 0;
 }
 
 void
 rte_timer_subsystem_finalize(void)
 {
-	if (!rte_timer_subsystem_initialized)
-		return;
-
 	rte_mcfg_timer_lock();
 
+	if (!rte_timer_subsystem_initialized) {
+		rte_mcfg_timer_unlock();
+		return;
+	}
+
 	if (--(*rte_timer_mz_refcnt) == 0)
 		rte_memzone_free(rte_timer_data_mz);
 
-	rte_mcfg_timer_unlock();
-
 	rte_timer_subsystem_initialized = 0;
+
+	rte_mcfg_timer_unlock();
 }
 
 /* Initialize the timer handle tim for use */
-- 
2.7.4

[dpdk-dev] [PATCH 2/2] lib/timer: relax barrier for status update

[Phil Yang]

Volatile has no ordering semantics. The rte_timer structure defines
timer status as a volatile variable and uses the rte_r/wmb barrier
to guarantee inter-thread visibility.

This patch optimized the volatile operation with c11 atomic operations
and one-way barrier to save the performance penalty. According to the
timer_perf_autotest benchmarking results, this patch can uplift 10%~16%
timer appending performance, 3%~20% timer resetting performance and 45%
timer callbacks scheduling performance on aarch64 and no loss in
performance for x86.

Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 lib/librte_timer/rte_timer.c | 90 +++++++++++++++++++++++++++++++-------------
 lib/librte_timer/rte_timer.h |  2 +-
 2 files changed, 65 insertions(+), 27 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 269e921..be0262d 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -10,7 +10,6 @@
 #include <assert.h>
 #include <sys/queue.h>
 
-#include <rte_atomic.h>
 #include <rte_common.h>
 #include <rte_cycles.h>
 #include <rte_eal_memconfig.h>
@@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
 
 	status.state = RTE_TIMER_STOP;
 	status.owner = RTE_TIMER_NO_OWNER;
-	tim->status.u32 = status.u32;
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELAXED);
 }
 
 /*
@@ -239,9 +238,9 @@ timer_set_config_state(struct rte_timer *tim,
 
 	/* wait that the timer is in correct status before update,
 	 * and mark it as being configured */
-	while (success == 0) {
-		prev_status.u32 = tim->status.u32;
+	prev_status.u32 = __atomic_load_n(&tim->status.u32, __ATOMIC_RELAXED);
 
+	while (success == 0) {
 		/* timer is running on another core
 		 * or ready to run on local core, exit
 		 */
@@ -258,9 +257,20 @@ timer_set_config_state(struct rte_timer *tim,
 		 * mark it atomically as being configured */
 		status.state = RTE_TIMER_CONFIG;
 		status.owner = (int16_t)lcore_id;
-		success = rte_atomic32_cmpset(&tim->status.u32,
-					      prev_status.u32,
-					      status.u32);
+		/* If status is observed as RTE_TIMER_CONFIG earlier,
+		 * that's not going to cause any issues because the
+		 * pattern is read for status then read the other members.
+		 * In one of the callers to timer_set_config_state
+		 * (the __rte_timer_reset) we set other members to the
+		 * structure (period, expire, f, arg) we want these
+		 * changes to be observed after our change to status.
+		 * So we need __ATOMIC_ACQUIRE here.
+		 */
+		success = __atomic_compare_exchange_n(&tim->status.u32,
+					      &prev_status.u32,
+					      status.u32, 0,
+					      __ATOMIC_ACQUIRE,
+					      __ATOMIC_RELAXED);
 	}
 
 	ret_prev_status->u32 = prev_status.u32;
@@ -279,20 +289,27 @@ timer_set_running_state(struct rte_timer *tim)
 
 	/* wait that the timer is in correct status before update,
 	 * and mark it as running */
-	while (success == 0) {
-		prev_status.u32 = tim->status.u32;
+	prev_status.u32 = __atomic_load_n(&tim->status.u32, __ATOMIC_RELAXED);
 
+	while (success == 0) {
 		/* timer is not pending anymore */
 		if (prev_status.state != RTE_TIMER_PENDING)
 			return -1;
 
 		/* here, we know that timer is stopped or pending,
-		 * mark it atomically as being configured */
+		 * mark it atomically as being running
+		 */
 		status.state = RTE_TIMER_RUNNING;
 		status.owner = (int16_t)lcore_id;
-		success = rte_atomic32_cmpset(&tim->status.u32,
-					      prev_status.u32,
-					      status.u32);
+		/* RUNNING states are acting as locked states. If the
+		 * timer is in RUNNING state, the state cannot be changed
+		 * by other threads. So, we should use ACQUIRE here.
+		 */
+		success = __atomic_compare_exchange_n(&tim->status.u32,
+					      &prev_status.u32,
+					      status.u32, 0,
+					      __ATOMIC_ACQUIRE,
+					      __ATOMIC_RELAXED);
 	}
 
 	return 0;
@@ -520,10 +537,12 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t expire,
 
 	/* update state: as we are in CONFIG state, only us can modify
 	 * the state so we don't need to use cmpset() here */
-	rte_wmb();
 	status.state = RTE_TIMER_PENDING;
 	status.owner = (int16_t)tim_lcore;
-	tim->status.u32 = status.u32;
+	/* The "RELEASE" ordering guarantees the memory operations above
+	 * the status update are observed before the update by all threads
+	 */
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELEASE);
 
 	if (tim_lcore != lcore_id || !local_is_locked)
 		rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock);
@@ -600,10 +619,12 @@ __rte_timer_stop(struct rte_timer *tim, int local_is_locked,
 	}
 
 	/* mark timer as stopped */
-	rte_wmb();
 	status.state = RTE_TIMER_STOP;
 	status.owner = RTE_TIMER_NO_OWNER;
-	tim->status.u32 = status.u32;
+	/* The "RELEASE" ordering guarantees the memory operations above
+	 * the status update are observed before the update by all threads
+	 */
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELEASE);
 
 	return 0;
 }
@@ -637,7 +658,8 @@ rte_timer_stop_sync(struct rte_timer *tim)
 int
 rte_timer_pending(struct rte_timer *tim)
 {
-	return tim->status.state == RTE_TIMER_PENDING;
+	return __atomic_load_n(&tim->status.state,
+				__ATOMIC_RELAXED) == RTE_TIMER_PENDING;
 }
 
 /* must be called periodically, run all timer that expired */
@@ -739,8 +761,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
 			/* remove from done list and mark timer as stopped */
 			status.state = RTE_TIMER_STOP;
 			status.owner = RTE_TIMER_NO_OWNER;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 		}
 		else {
 			/* keep it in list and mark timer as pending */
@@ -748,8 +774,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
 			status.state = RTE_TIMER_PENDING;
 			__TIMER_STAT_ADD(priv_timer, pending, 1);
 			status.owner = (int16_t)lcore_id;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 			__rte_timer_reset(tim, tim->expire + tim->period,
 				tim->period, lcore_id, tim->f, tim->arg, 1,
 				timer_data);
@@ -919,8 +949,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
 			/* remove from done list and mark timer as stopped */
 			status.state = RTE_TIMER_STOP;
 			status.owner = RTE_TIMER_NO_OWNER;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 		} else {
 			/* keep it in list and mark timer as pending */
 			rte_spinlock_lock(
@@ -928,8 +962,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
 			status.state = RTE_TIMER_PENDING;
 			__TIMER_STAT_ADD(data->priv_timer, pending, 1);
 			status.owner = (int16_t)this_lcore;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 			__rte_timer_reset(tim, tim->expire + tim->period,
 				tim->period, this_lcore, tim->f, tim->arg, 1,
 				data);
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index c6b3d45..df533fa 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -101,7 +101,7 @@ struct rte_timer
 {
 	uint64_t expire;       /**< Time when timer expire. */
 	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
-	volatile union rte_timer_status status; /**< Status of timer. */
+	union rte_timer_status status; /**< Status of timer. */
 	uint64_t period;       /**< Period of timer (0 if not periodic). */
 	rte_timer_cb_t f;      /**< Callback function. */
 	void *arg;             /**< Argument to callback function. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH 1/2] lib/timer: protect timer subsystem initialized with lock

[Carrillo, Erik G]

> -----Original Message-----
> From: Phil Yang <phil.yang@arm.com>
> Sent: Monday, February 24, 2020 12:42 AM
> To: rsanford@akamai.com; Carrillo, Erik G <erik.g.carrillo@intel.com>;
> dev@dpdk.org
> Cc: david.marchand@redhat.com; Burakov, Anatoly
> <anatoly.burakov@intel.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Honnappa.Nagarahalli@arm.com;
> gavin.hu@arm.com; phil.yang@arm.com; nd@arm.com; Honnappa
> Nagarahalli <honnappa.nagarahalli@arm.com>; stable@dpdk.org
> Subject: [PATCH 1/2] lib/timer: protect timer subsystem initialized with lock
> 
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> 
> rte_timer_subsystem_initialized is a global variable that can be accessed by
> multiple processes simultaneously. Hence, any access to
> rte_timer_subsystem_initialized should be protected by
> rte_mcfg_timer_lock.
> 
> Fixes: f9d6cd8bfe9e ("timer: fix resource leak in finalize")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

Re: [dpdk-dev] [PATCH 2/2] lib/timer: relax barrier for status update

[Phil Yang]

> -----Original Message-----
> From: Phil Yang <phil.yang@arm.com>
> Sent: Monday, February 24, 2020 2:42 PM
> To: rsanford@akamai.com; erik.g.carrillo@intel.com; dev@dpdk.org
> Cc: david.marchand@redhat.com; anatoly.burakov@intel.com;
> thomas@monjalon.net; jerinj@marvell.com; hemant.agrawal@nxp.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Gavin Hu
> <Gavin.Hu@arm.com>; Phil Yang <Phil.Yang@arm.com>; nd <nd@arm.com>
> Subject: [PATCH 2/2] lib/timer: relax barrier for status update
> 
> Volatile has no ordering semantics. The rte_timer structure defines
> timer status as a volatile variable and uses the rte_r/wmb barrier
> to guarantee inter-thread visibility.
> 
> This patch optimized the volatile operation with c11 atomic operations
> and one-way barrier to save the performance penalty. According to the
> timer_perf_autotest benchmarking results, this patch can uplift 10%~16%
> timer appending performance, 3%~20% timer resetting performance and 45%
> timer callbacks scheduling performance on aarch64 and no loss in
> performance for x86.
> 
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
>  lib/librte_timer/rte_timer.c | 90 +++++++++++++++++++++++++++++++----

Ping. 

Thanks,
Phil

Re: [dpdk-dev] [PATCH 2/2] lib/timer: relax barrier for status update

[Carrillo, Erik G]

> -----Original Message-----
> From: Phil Yang <phil.yang@arm.com>
> Sent: Monday, February 24, 2020 12:42 AM
> To: rsanford@akamai.com; Carrillo, Erik G <erik.g.carrillo@intel.com>;
> dev@dpdk.org
> Cc: david.marchand@redhat.com; Burakov, Anatoly
> <anatoly.burakov@intel.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Honnappa.Nagarahalli@arm.com;
> gavin.hu@arm.com; phil.yang@arm.com; nd@arm.com
> Subject: [PATCH 2/2] lib/timer: relax barrier for status update
> 
> Volatile has no ordering semantics. The rte_timer structure defines timer
> status as a volatile variable and uses the rte_r/wmb barrier to guarantee
> inter-thread visibility.
> 
> This patch optimized the volatile operation with c11 atomic operations and
> one-way barrier to save the performance penalty. According to the
> timer_perf_autotest benchmarking results, this patch can uplift 10%~16%
> timer appending performance, 3%~20% timer resetting performance and
> 45% timer callbacks scheduling performance on aarch64 and no loss in
> performance for x86.
> 
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>

Hi Phil,

It seems like the consensus is to generally avoid replacing rte_atomic_* interfaces with the GCC builtins directly.   In other areas of DPDK that are being patched, are the <std_atomic.h> C11 APIs going to be investigated?   It seems like that decision will apply here as well.

Thanks,
Erik

> ---
>  lib/librte_timer/rte_timer.c | 90 +++++++++++++++++++++++++++++++----
> ---------
>  lib/librte_timer/rte_timer.h |  2 +-
>  2 files changed, 65 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c index
> 269e921..be0262d 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -10,7 +10,6 @@
>  #include <assert.h>
>  #include <sys/queue.h>
> 
> -#include <rte_atomic.h>
>  #include <rte_common.h>
>  #include <rte_cycles.h>
>  #include <rte_eal_memconfig.h>
> @@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
> 
>  	status.state = RTE_TIMER_STOP;
>  	status.owner = RTE_TIMER_NO_OWNER;
> -	tim->status.u32 = status.u32;
> +	__atomic_store_n(&tim->status.u32, status.u32,
> __ATOMIC_RELAXED);
>  }
> 
>  /*
> @@ -239,9 +238,9 @@ timer_set_config_state(struct rte_timer *tim,
> 
>  	/* wait that the timer is in correct status before update,
>  	 * and mark it as being configured */
> -	while (success == 0) {
> -		prev_status.u32 = tim->status.u32;
> +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> __ATOMIC_RELAXED);
> 
> +	while (success == 0) {
>  		/* timer is running on another core
>  		 * or ready to run on local core, exit
>  		 */
> @@ -258,9 +257,20 @@ timer_set_config_state(struct rte_timer *tim,
>  		 * mark it atomically as being configured */
>  		status.state = RTE_TIMER_CONFIG;
>  		status.owner = (int16_t)lcore_id;
> -		success = rte_atomic32_cmpset(&tim->status.u32,
> -					      prev_status.u32,
> -					      status.u32);
> +		/* If status is observed as RTE_TIMER_CONFIG earlier,
> +		 * that's not going to cause any issues because the
> +		 * pattern is read for status then read the other members.
> +		 * In one of the callers to timer_set_config_state
> +		 * (the __rte_timer_reset) we set other members to the
> +		 * structure (period, expire, f, arg) we want these
> +		 * changes to be observed after our change to status.
> +		 * So we need __ATOMIC_ACQUIRE here.
> +		 */
> +		success = __atomic_compare_exchange_n(&tim-
> >status.u32,
> +					      &prev_status.u32,
> +					      status.u32, 0,
> +					      __ATOMIC_ACQUIRE,
> +					      __ATOMIC_RELAXED);
>  	}
> 
>  	ret_prev_status->u32 = prev_status.u32; @@ -279,20 +289,27 @@
> timer_set_running_state(struct rte_timer *tim)
> 
>  	/* wait that the timer is in correct status before update,
>  	 * and mark it as running */
> -	while (success == 0) {
> -		prev_status.u32 = tim->status.u32;
> +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> __ATOMIC_RELAXED);
> 
> +	while (success == 0) {
>  		/* timer is not pending anymore */
>  		if (prev_status.state != RTE_TIMER_PENDING)
>  			return -1;
> 
>  		/* here, we know that timer is stopped or pending,
> -		 * mark it atomically as being configured */
> +		 * mark it atomically as being running
> +		 */
>  		status.state = RTE_TIMER_RUNNING;
>  		status.owner = (int16_t)lcore_id;
> -		success = rte_atomic32_cmpset(&tim->status.u32,
> -					      prev_status.u32,
> -					      status.u32);
> +		/* RUNNING states are acting as locked states. If the
> +		 * timer is in RUNNING state, the state cannot be changed
> +		 * by other threads. So, we should use ACQUIRE here.
> +		 */
> +		success = __atomic_compare_exchange_n(&tim-
> >status.u32,
> +					      &prev_status.u32,
> +					      status.u32, 0,
> +					      __ATOMIC_ACQUIRE,
> +					      __ATOMIC_RELAXED);
>  	}
> 
>  	return 0;
> @@ -520,10 +537,12 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t
> expire,
> 
>  	/* update state: as we are in CONFIG state, only us can modify
>  	 * the state so we don't need to use cmpset() here */
> -	rte_wmb();
>  	status.state = RTE_TIMER_PENDING;
>  	status.owner = (int16_t)tim_lcore;
> -	tim->status.u32 = status.u32;
> +	/* The "RELEASE" ordering guarantees the memory operations above
> +	 * the status update are observed before the update by all threads
> +	 */
> +	__atomic_store_n(&tim->status.u32, status.u32,
> __ATOMIC_RELEASE);
> 
>  	if (tim_lcore != lcore_id || !local_is_locked)
>  		rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock);
> @@ -600,10 +619,12 @@ __rte_timer_stop(struct rte_timer *tim, int
> local_is_locked,
>  	}
> 
>  	/* mark timer as stopped */
> -	rte_wmb();
>  	status.state = RTE_TIMER_STOP;
>  	status.owner = RTE_TIMER_NO_OWNER;
> -	tim->status.u32 = status.u32;
> +	/* The "RELEASE" ordering guarantees the memory operations above
> +	 * the status update are observed before the update by all threads
> +	 */
> +	__atomic_store_n(&tim->status.u32, status.u32,
> __ATOMIC_RELEASE);
> 
>  	return 0;
>  }
> @@ -637,7 +658,8 @@ rte_timer_stop_sync(struct rte_timer *tim)  int
> rte_timer_pending(struct rte_timer *tim)  {
> -	return tim->status.state == RTE_TIMER_PENDING;
> +	return __atomic_load_n(&tim->status.state,
> +				__ATOMIC_RELAXED) ==
> RTE_TIMER_PENDING;
>  }
> 
>  /* must be called periodically, run all timer that expired */ @@ -739,8
> +761,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
>  			/* remove from done list and mark timer as stopped
> */
>  			status.state = RTE_TIMER_STOP;
>  			status.owner = RTE_TIMER_NO_OWNER;
> -			rte_wmb();
> -			tim->status.u32 = status.u32;
> +			/* The "RELEASE" ordering guarantees the memory
> +			 * operations above the status update are observed
> +			 * before the update by all threads
> +			 */
> +			__atomic_store_n(&tim->status.u32, status.u32,
> +				__ATOMIC_RELEASE);
>  		}
>  		else {
>  			/* keep it in list and mark timer as pending */ @@ -
> 748,8 +774,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
>  			status.state = RTE_TIMER_PENDING;
>  			__TIMER_STAT_ADD(priv_timer, pending, 1);
>  			status.owner = (int16_t)lcore_id;
> -			rte_wmb();
> -			tim->status.u32 = status.u32;
> +			/* The "RELEASE" ordering guarantees the memory
> +			 * operations above the status update are observed
> +			 * before the update by all threads
> +			 */
> +			__atomic_store_n(&tim->status.u32, status.u32,
> +				__ATOMIC_RELEASE);
>  			__rte_timer_reset(tim, tim->expire + tim->period,
>  				tim->period, lcore_id, tim->f, tim->arg, 1,
>  				timer_data);
> @@ -919,8 +949,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
>  			/* remove from done list and mark timer as stopped
> */
>  			status.state = RTE_TIMER_STOP;
>  			status.owner = RTE_TIMER_NO_OWNER;
> -			rte_wmb();
> -			tim->status.u32 = status.u32;
> +			/* The "RELEASE" ordering guarantees the memory
> +			 * operations above the status update are observed
> +			 * before the update by all threads
> +			 */
> +			__atomic_store_n(&tim->status.u32, status.u32,
> +				__ATOMIC_RELEASE);
>  		} else {
>  			/* keep it in list and mark timer as pending */
>  			rte_spinlock_lock(
> @@ -928,8 +962,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
>  			status.state = RTE_TIMER_PENDING;
>  			__TIMER_STAT_ADD(data->priv_timer, pending, 1);
>  			status.owner = (int16_t)this_lcore;
> -			rte_wmb();
> -			tim->status.u32 = status.u32;
> +			/* The "RELEASE" ordering guarantees the memory
> +			 * operations above the status update are observed
> +			 * before the update by all threads
> +			 */
> +			__atomic_store_n(&tim->status.u32, status.u32,
> +				__ATOMIC_RELEASE);
>  			__rte_timer_reset(tim, tim->expire + tim->period,
>  				tim->period, this_lcore, tim->f, tim->arg, 1,
>  				data);
> diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h index
> c6b3d45..df533fa 100644
> --- a/lib/librte_timer/rte_timer.h
> +++ b/lib/librte_timer/rte_timer.h
> @@ -101,7 +101,7 @@ struct rte_timer
>  {
>  	uint64_t expire;       /**< Time when timer expire. */
>  	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
> -	volatile union rte_timer_status status; /**< Status of timer. */
> +	union rte_timer_status status; /**< Status of timer. */
>  	uint64_t period;       /**< Period of timer (0 if not periodic). */
>  	rte_timer_cb_t f;      /**< Callback function. */
>  	void *arg;             /**< Argument to callback function. */
> --
> 2.7.4

Re: [dpdk-dev] [PATCH 2/2] lib/timer: relax barrier for status update

[Honnappa Nagarahalli]

<snip>

> > Subject: [PATCH 2/2] lib/timer: relax barrier for status update
> >
> > Volatile has no ordering semantics. The rte_timer structure defines
> > timer status as a volatile variable and uses the rte_r/wmb barrier to
> > guarantee inter-thread visibility.
> >
> > This patch optimized the volatile operation with c11 atomic operations
> > and one-way barrier to save the performance penalty. According to the
> > timer_perf_autotest benchmarking results, this patch can uplift
> > 10%~16% timer appending performance, 3%~20% timer resetting
> > performance and 45% timer callbacks scheduling performance on aarch64
> > and no loss in performance for x86.
> >
> > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> 
> Hi Phil,
> 
> It seems like the consensus is to generally avoid replacing rte_atomic_*
> interfaces with the GCC builtins directly.   In other areas of DPDK that are
> being patched, are the <std_atomic.h> C11 APIs going to be investigated?   It
> seems like that decision will apply here as well.
Agree. The new APIs are going to be 1 to 1 mapped with the built-in intrinsics (the memory orderings used themselves will not change). We should go ahead with the review and conclude any issues. Once the decision is made on what APIs to use, we can submit the next version using the APIs decided.

> 
> Thanks,
> Erik
> 
> > ---
> >  lib/librte_timer/rte_timer.c | 90 +++++++++++++++++++++++++++++++----
> > ---------
> >  lib/librte_timer/rte_timer.h |  2 +-
> >  2 files changed, 65 insertions(+), 27 deletions(-)
> >
> > diff --git a/lib/librte_timer/rte_timer.c
> > b/lib/librte_timer/rte_timer.c index 269e921..be0262d 100644
> > --- a/lib/librte_timer/rte_timer.c
> > +++ b/lib/librte_timer/rte_timer.c
> > @@ -10,7 +10,6 @@
> >  #include <assert.h>
> >  #include <sys/queue.h>
> >
> > -#include <rte_atomic.h>
> >  #include <rte_common.h>
> >  #include <rte_cycles.h>
> >  #include <rte_eal_memconfig.h>
> > @@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
> >
> >  	status.state = RTE_TIMER_STOP;
> >  	status.owner = RTE_TIMER_NO_OWNER;
> > -	tim->status.u32 = status.u32;
> > +	__atomic_store_n(&tim->status.u32, status.u32,
> > __ATOMIC_RELAXED);
> >  }
> >
> >  /*
> > @@ -239,9 +238,9 @@ timer_set_config_state(struct rte_timer *tim,
> >
> >  	/* wait that the timer is in correct status before update,
> >  	 * and mark it as being configured */
> > -	while (success == 0) {
> > -		prev_status.u32 = tim->status.u32;
> > +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> > __ATOMIC_RELAXED);
> >
> > +	while (success == 0) {
> >  		/* timer is running on another core
> >  		 * or ready to run on local core, exit
> >  		 */
> > @@ -258,9 +257,20 @@ timer_set_config_state(struct rte_timer *tim,
> >  		 * mark it atomically as being configured */
> >  		status.state = RTE_TIMER_CONFIG;
> >  		status.owner = (int16_t)lcore_id;
> > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > -					      prev_status.u32,
> > -					      status.u32);
> > +		/* If status is observed as RTE_TIMER_CONFIG earlier,
> > +		 * that's not going to cause any issues because the
> > +		 * pattern is read for status then read the other members.
> > +		 * In one of the callers to timer_set_config_state
> > +		 * (the __rte_timer_reset) we set other members to the
> > +		 * structure (period, expire, f, arg) we want these
> > +		 * changes to be observed after our change to status.
> > +		 * So we need __ATOMIC_ACQUIRE here.
> > +		 */
> > +		success = __atomic_compare_exchange_n(&tim-
> > >status.u32,
> > +					      &prev_status.u32,
> > +					      status.u32, 0,
> > +					      __ATOMIC_ACQUIRE,
> > +					      __ATOMIC_RELAXED);
> >  	}
> >
> >  	ret_prev_status->u32 = prev_status.u32; @@ -279,20 +289,27 @@
> > timer_set_running_state(struct rte_timer *tim)
> >
> >  	/* wait that the timer is in correct status before update,
> >  	 * and mark it as running */
> > -	while (success == 0) {
> > -		prev_status.u32 = tim->status.u32;
> > +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> > __ATOMIC_RELAXED);
> >
> > +	while (success == 0) {
> >  		/* timer is not pending anymore */
> >  		if (prev_status.state != RTE_TIMER_PENDING)
> >  			return -1;
> >
> >  		/* here, we know that timer is stopped or pending,
> > -		 * mark it atomically as being configured */
> > +		 * mark it atomically as being running
> > +		 */
> >  		status.state = RTE_TIMER_RUNNING;
> >  		status.owner = (int16_t)lcore_id;
> > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > -					      prev_status.u32,
> > -					      status.u32);
> > +		/* RUNNING states are acting as locked states. If the
> > +		 * timer is in RUNNING state, the state cannot be changed
> > +		 * by other threads. So, we should use ACQUIRE here.
> > +		 */
> > +		success = __atomic_compare_exchange_n(&tim-
> > >status.u32,
> > +					      &prev_status.u32,
> > +					      status.u32, 0,
> > +					      __ATOMIC_ACQUIRE,
> > +					      __ATOMIC_RELAXED);
> >  	}
> >
> >  	return 0;
> > @@ -520,10 +537,12 @@ __rte_timer_reset(struct rte_timer *tim,
> > uint64_t expire,
> >
> >  	/* update state: as we are in CONFIG state, only us can modify
> >  	 * the state so we don't need to use cmpset() here */
> > -	rte_wmb();
> >  	status.state = RTE_TIMER_PENDING;
> >  	status.owner = (int16_t)tim_lcore;
> > -	tim->status.u32 = status.u32;
> > +	/* The "RELEASE" ordering guarantees the memory operations above
> > +	 * the status update are observed before the update by all threads
> > +	 */
> > +	__atomic_store_n(&tim->status.u32, status.u32,
> > __ATOMIC_RELEASE);
> >
> >  	if (tim_lcore != lcore_id || !local_is_locked)
> >  		rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock);
> > @@ -600,10 +619,12 @@ __rte_timer_stop(struct rte_timer *tim, int
> > local_is_locked,
> >  	}
> >
> >  	/* mark timer as stopped */
> > -	rte_wmb();
> >  	status.state = RTE_TIMER_STOP;
> >  	status.owner = RTE_TIMER_NO_OWNER;
> > -	tim->status.u32 = status.u32;
> > +	/* The "RELEASE" ordering guarantees the memory operations above
> > +	 * the status update are observed before the update by all threads
> > +	 */
> > +	__atomic_store_n(&tim->status.u32, status.u32,
> > __ATOMIC_RELEASE);
> >
> >  	return 0;
> >  }
> > @@ -637,7 +658,8 @@ rte_timer_stop_sync(struct rte_timer *tim)  int
> > rte_timer_pending(struct rte_timer *tim)  {
> > -	return tim->status.state == RTE_TIMER_PENDING;
> > +	return __atomic_load_n(&tim->status.state,
> > +				__ATOMIC_RELAXED) ==
> > RTE_TIMER_PENDING;
> >  }
> >
> >  /* must be called periodically, run all timer that expired */ @@
> > -739,8
> > +761,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
> >  			/* remove from done list and mark timer as stopped
> */
> >  			status.state = RTE_TIMER_STOP;
> >  			status.owner = RTE_TIMER_NO_OWNER;
> > -			rte_wmb();
> > -			tim->status.u32 = status.u32;
> > +			/* The "RELEASE" ordering guarantees the memory
> > +			 * operations above the status update are observed
> > +			 * before the update by all threads
> > +			 */
> > +			__atomic_store_n(&tim->status.u32, status.u32,
> > +				__ATOMIC_RELEASE);
> >  		}
> >  		else {
> >  			/* keep it in list and mark timer as pending */ @@ -
> > 748,8 +774,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
> >  			status.state = RTE_TIMER_PENDING;
> >  			__TIMER_STAT_ADD(priv_timer, pending, 1);
> >  			status.owner = (int16_t)lcore_id;
> > -			rte_wmb();
> > -			tim->status.u32 = status.u32;
> > +			/* The "RELEASE" ordering guarantees the memory
> > +			 * operations above the status update are observed
> > +			 * before the update by all threads
> > +			 */
> > +			__atomic_store_n(&tim->status.u32, status.u32,
> > +				__ATOMIC_RELEASE);
> >  			__rte_timer_reset(tim, tim->expire + tim->period,
> >  				tim->period, lcore_id, tim->f, tim->arg, 1,
> >  				timer_data);
> > @@ -919,8 +949,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
> >  			/* remove from done list and mark timer as stopped
> */
> >  			status.state = RTE_TIMER_STOP;
> >  			status.owner = RTE_TIMER_NO_OWNER;
> > -			rte_wmb();
> > -			tim->status.u32 = status.u32;
> > +			/* The "RELEASE" ordering guarantees the memory
> > +			 * operations above the status update are observed
> > +			 * before the update by all threads
> > +			 */
> > +			__atomic_store_n(&tim->status.u32, status.u32,
> > +				__ATOMIC_RELEASE);
> >  		} else {
> >  			/* keep it in list and mark timer as pending */
> >  			rte_spinlock_lock(
> > @@ -928,8 +962,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
> >  			status.state = RTE_TIMER_PENDING;
> >  			__TIMER_STAT_ADD(data->priv_timer, pending, 1);
> >  			status.owner = (int16_t)this_lcore;
> > -			rte_wmb();
> > -			tim->status.u32 = status.u32;
> > +			/* The "RELEASE" ordering guarantees the memory
> > +			 * operations above the status update are observed
> > +			 * before the update by all threads
> > +			 */
> > +			__atomic_store_n(&tim->status.u32, status.u32,
> > +				__ATOMIC_RELEASE);
> >  			__rte_timer_reset(tim, tim->expire + tim->period,
> >  				tim->period, this_lcore, tim->f, tim->arg, 1,
> >  				data);
> > diff --git a/lib/librte_timer/rte_timer.h
> > b/lib/librte_timer/rte_timer.h index c6b3d45..df533fa 100644
> > --- a/lib/librte_timer/rte_timer.h
> > +++ b/lib/librte_timer/rte_timer.h
> > @@ -101,7 +101,7 @@ struct rte_timer
> >  {
> >  	uint64_t expire;       /**< Time when timer expire. */
> >  	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
> > -	volatile union rte_timer_status status; /**< Status of timer. */
> > +	union rte_timer_status status; /**< Status of timer. */
> >  	uint64_t period;       /**< Period of timer (0 if not periodic). */
> >  	rte_timer_cb_t f;      /**< Callback function. */
> >  	void *arg;             /**< Argument to callback function. */
> > --
> > 2.7.4

Re: [dpdk-dev] [PATCH 2/2] lib/timer: relax barrier for status update

[Carrillo, Erik G]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Wednesday, April 8, 2020 4:16 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; Phil Yang
> <Phil.Yang@arm.com>; rsanford@akamai.com; dev@dpdk.org
> Cc: david.marchand@redhat.com; Burakov, Anatoly
> <anatoly.burakov@intel.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Gavin Hu <Gavin.Hu@arm.com>; nd
> <nd@arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> nd <nd@arm.com>
> Subject: RE: [PATCH 2/2] lib/timer: relax barrier for status update
> 
> <snip>
> 
> > > Subject: [PATCH 2/2] lib/timer: relax barrier for status update
> > >
> > > Volatile has no ordering semantics. The rte_timer structure defines
> > > timer status as a volatile variable and uses the rte_r/wmb barrier
> > > to guarantee inter-thread visibility.
> > >
> > > This patch optimized the volatile operation with c11 atomic
> > > operations and one-way barrier to save the performance penalty.
> > > According to the timer_perf_autotest benchmarking results, this
> > > patch can uplift 10%~16% timer appending performance, 3%~20% timer
> > > resetting performance and 45% timer callbacks scheduling performance
> > > on aarch64 and no loss in performance for x86.
> > >
> > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> >
> > Hi Phil,
> >
> > It seems like the consensus is to generally avoid replacing rte_atomic_*
> > interfaces with the GCC builtins directly.   In other areas of DPDK that are
> > being patched, are the <std_atomic.h> C11 APIs going to be investigated?
> It
> > seems like that decision will apply here as well.
> Agree. The new APIs are going to be 1 to 1 mapped with the built-in intrinsics
> (the memory orderings used themselves will not change). We should go
> ahead with the review and conclude any issues. Once the decision is made
> on what APIs to use, we can submit the next version using the APIs decided.
> 
Thanks, Honnappa.

I have reviewed the memory orderings and I see no issues with them.   I do have a question regarding a comment - I'll pose it inline:

> >
> > Thanks,
> > Erik
> >
> > > ---
> > >  lib/librte_timer/rte_timer.c | 90
> > > +++++++++++++++++++++++++++++++----
> > > ---------
> > >  lib/librte_timer/rte_timer.h |  2 +-
> > >  2 files changed, 65 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/lib/librte_timer/rte_timer.c
> > > b/lib/librte_timer/rte_timer.c index 269e921..be0262d 100644
> > > --- a/lib/librte_timer/rte_timer.c
> > > +++ b/lib/librte_timer/rte_timer.c
> > > @@ -10,7 +10,6 @@
> > >  #include <assert.h>
> > >  #include <sys/queue.h>
> > >
> > > -#include <rte_atomic.h>
> > >  #include <rte_common.h>
> > >  #include <rte_cycles.h>
> > >  #include <rte_eal_memconfig.h>
> > > @@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
> > >
> > >  	status.state = RTE_TIMER_STOP;
> > >  	status.owner = RTE_TIMER_NO_OWNER;
> > > -	tim->status.u32 = status.u32;
> > > +	__atomic_store_n(&tim->status.u32, status.u32,
> > > __ATOMIC_RELAXED);
> > >  }
> > >
> > >  /*
> > > @@ -239,9 +238,9 @@ timer_set_config_state(struct rte_timer *tim,
> > >
> > >  	/* wait that the timer is in correct status before update,
> > >  	 * and mark it as being configured */
> > > -	while (success == 0) {
> > > -		prev_status.u32 = tim->status.u32;
> > > +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> > > __ATOMIC_RELAXED);
> > >
> > > +	while (success == 0) {
> > >  		/* timer is running on another core
> > >  		 * or ready to run on local core, exit
> > >  		 */
> > > @@ -258,9 +257,20 @@ timer_set_config_state(struct rte_timer *tim,
> > >  		 * mark it atomically as being configured */
> > >  		status.state = RTE_TIMER_CONFIG;
> > >  		status.owner = (int16_t)lcore_id;
> > > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > > -					      prev_status.u32,
> > > -					      status.u32);
> > > +		/* If status is observed as RTE_TIMER_CONFIG earlier,
> > > +		 * that's not going to cause any issues because the
> > > +		 * pattern is read for status then read the other members.

I don't follow the above comment.  What is meant by "earlier"?

Thanks,
Erik

> > > +		 * In one of the callers to timer_set_config_state
> > > +		 * (the __rte_timer_reset) we set other members to the
> > > +		 * structure (period, expire, f, arg) we want these
> > > +		 * changes to be observed after our change to status.
> > > +		 * So we need __ATOMIC_ACQUIRE here.
> > > +		 */
> > > +		success = __atomic_compare_exchange_n(&tim-
> > > >status.u32,
> > > +					      &prev_status.u32,
> > > +					      status.u32, 0,
> > > +					      __ATOMIC_ACQUIRE,
> > > +					      __ATOMIC_RELAXED);
> > >  	}
> > >
> > >  	ret_prev_status->u32 = prev_status.u32; @@ -279,20 +289,27 @@
> > > timer_set_running_state(struct rte_timer *tim)
> > >
> > >  	/* wait that the timer is in correct status before update,
> > >  	 * and mark it as running */
> > > -	while (success == 0) {
> > > -		prev_status.u32 = tim->status.u32;
> > > +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> > > __ATOMIC_RELAXED);
> > >
> > > +	while (success == 0) {
> > >  		/* timer is not pending anymore */
> > >  		if (prev_status.state != RTE_TIMER_PENDING)
> > >  			return -1;
> > >
> > >  		/* here, we know that timer is stopped or pending,
> > > -		 * mark it atomically as being configured */
> > > +		 * mark it atomically as being running
> > > +		 */
> > >  		status.state = RTE_TIMER_RUNNING;
> > >  		status.owner = (int16_t)lcore_id;
> > > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > > -					      prev_status.u32,
> > > -					      status.u32);
> > > +		/* RUNNING states are acting as locked states. If the
> > > +		 * timer is in RUNNING state, the state cannot be changed
> > > +		 * by other threads. So, we should use ACQUIRE here.
> > > +		 */
> > > +		success = __atomic_compare_exchange_n(&tim-
> > > >status.u32,
> > > +					      &prev_status.u32,
> > > +					      status.u32, 0,
> > > +					      __ATOMIC_ACQUIRE,
> > > +					      __ATOMIC_RELAXED);
> > >  	}
> > >
> > >  	return 0;
> > > @@ -520,10 +537,12 @@ __rte_timer_reset(struct rte_timer *tim,
> > > uint64_t expire,
> > >
> > >  	/* update state: as we are in CONFIG state, only us can modify
> > >  	 * the state so we don't need to use cmpset() here */
> > > -	rte_wmb();
> > >  	status.state = RTE_TIMER_PENDING;
> > >  	status.owner = (int16_t)tim_lcore;
> > > -	tim->status.u32 = status.u32;
> > > +	/* The "RELEASE" ordering guarantees the memory operations above
> > > +	 * the status update are observed before the update by all threads
> > > +	 */
> > > +	__atomic_store_n(&tim->status.u32, status.u32,
> > > __ATOMIC_RELEASE);
> > >
> > >  	if (tim_lcore != lcore_id || !local_is_locked)
> > >  		rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock);
> > > @@ -600,10 +619,12 @@ __rte_timer_stop(struct rte_timer *tim, int
> > > local_is_locked,
> > >  	}
> > >
> > >  	/* mark timer as stopped */
> > > -	rte_wmb();
> > >  	status.state = RTE_TIMER_STOP;
> > >  	status.owner = RTE_TIMER_NO_OWNER;
> > > -	tim->status.u32 = status.u32;
> > > +	/* The "RELEASE" ordering guarantees the memory operations above
> > > +	 * the status update are observed before the update by all threads
> > > +	 */
> > > +	__atomic_store_n(&tim->status.u32, status.u32,
> > > __ATOMIC_RELEASE);
> > >
> > >  	return 0;
> > >  }
> > > @@ -637,7 +658,8 @@ rte_timer_stop_sync(struct rte_timer *tim)  int
> > > rte_timer_pending(struct rte_timer *tim)  {
> > > -	return tim->status.state == RTE_TIMER_PENDING;
> > > +	return __atomic_load_n(&tim->status.state,
> > > +				__ATOMIC_RELAXED) ==
> > > RTE_TIMER_PENDING;
> > >  }
> > >
> > >  /* must be called periodically, run all timer that expired */ @@
> > > -739,8
> > > +761,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
> > >  			/* remove from done list and mark timer as stopped
> > */
> > >  			status.state = RTE_TIMER_STOP;
> > >  			status.owner = RTE_TIMER_NO_OWNER;
> > > -			rte_wmb();
> > > -			tim->status.u32 = status.u32;
> > > +			/* The "RELEASE" ordering guarantees the memory
> > > +			 * operations above the status update are observed
> > > +			 * before the update by all threads
> > > +			 */
> > > +			__atomic_store_n(&tim->status.u32, status.u32,
> > > +				__ATOMIC_RELEASE);
> > >  		}
> > >  		else {
> > >  			/* keep it in list and mark timer as pending */ @@ -
> > > 748,8 +774,12 @@ __rte_timer_manage(struct rte_timer_data
> *timer_data)
> > >  			status.state = RTE_TIMER_PENDING;
> > >  			__TIMER_STAT_ADD(priv_timer, pending, 1);
> > >  			status.owner = (int16_t)lcore_id;
> > > -			rte_wmb();
> > > -			tim->status.u32 = status.u32;
> > > +			/* The "RELEASE" ordering guarantees the memory
> > > +			 * operations above the status update are observed
> > > +			 * before the update by all threads
> > > +			 */
> > > +			__atomic_store_n(&tim->status.u32, status.u32,
> > > +				__ATOMIC_RELEASE);
> > >  			__rte_timer_reset(tim, tim->expire + tim->period,
> > >  				tim->period, lcore_id, tim->f, tim->arg, 1,
> > >  				timer_data);
> > > @@ -919,8 +949,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
> > >  			/* remove from done list and mark timer as stopped
> > */
> > >  			status.state = RTE_TIMER_STOP;
> > >  			status.owner = RTE_TIMER_NO_OWNER;
> > > -			rte_wmb();
> > > -			tim->status.u32 = status.u32;
> > > +			/* The "RELEASE" ordering guarantees the memory
> > > +			 * operations above the status update are observed
> > > +			 * before the update by all threads
> > > +			 */
> > > +			__atomic_store_n(&tim->status.u32, status.u32,
> > > +				__ATOMIC_RELEASE);
> > >  		} else {
> > >  			/* keep it in list and mark timer as pending */
> > >  			rte_spinlock_lock(
> > > @@ -928,8 +962,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
> > >  			status.state = RTE_TIMER_PENDING;
> > >  			__TIMER_STAT_ADD(data->priv_timer, pending, 1);
> > >  			status.owner = (int16_t)this_lcore;
> > > -			rte_wmb();
> > > -			tim->status.u32 = status.u32;
> > > +			/* The "RELEASE" ordering guarantees the memory
> > > +			 * operations above the status update are observed
> > > +			 * before the update by all threads
> > > +			 */
> > > +			__atomic_store_n(&tim->status.u32, status.u32,
> > > +				__ATOMIC_RELEASE);
> > >  			__rte_timer_reset(tim, tim->expire + tim->period,
> > >  				tim->period, this_lcore, tim->f, tim->arg, 1,
> > >  				data);
> > > diff --git a/lib/librte_timer/rte_timer.h
> > > b/lib/librte_timer/rte_timer.h index c6b3d45..df533fa 100644
> > > --- a/lib/librte_timer/rte_timer.h
> > > +++ b/lib/librte_timer/rte_timer.h
> > > @@ -101,7 +101,7 @@ struct rte_timer  {
> > >  	uint64_t expire;       /**< Time when timer expire. */
> > >  	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
> > > -	volatile union rte_timer_status status; /**< Status of timer. */
> > > +	union rte_timer_status status; /**< Status of timer. */
> > >  	uint64_t period;       /**< Period of timer (0 if not periodic). */
> > >  	rte_timer_cb_t f;      /**< Callback function. */
> > >  	void *arg;             /**< Argument to callback function. */
> > > --
> > > 2.7.4

Re: [dpdk-dev] [PATCH 2/2] lib/timer: relax barrier for status update

[Honnappa Nagarahalli]

<snip>

> >
> > > > Subject: [PATCH 2/2] lib/timer: relax barrier for status update
> > > >
> > > > Volatile has no ordering semantics. The rte_timer structure
> > > > defines timer status as a volatile variable and uses the rte_r/wmb
> > > > barrier to guarantee inter-thread visibility.
> > > >
> > > > This patch optimized the volatile operation with c11 atomic
> > > > operations and one-way barrier to save the performance penalty.
> > > > According to the timer_perf_autotest benchmarking results, this
> > > > patch can uplift 10%~16% timer appending performance, 3%~20% timer
> > > > resetting performance and 45% timer callbacks scheduling
> > > > performance on aarch64 and no loss in performance for x86.
> > > >
> > > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > >
> > > Hi Phil,
> > >
> > > It seems like the consensus is to generally avoid replacing rte_atomic_*
> > > interfaces with the GCC builtins directly.   In other areas of DPDK that are
> > > being patched, are the <std_atomic.h> C11 APIs going to be investigated?
> > It
> > > seems like that decision will apply here as well.
> > Agree. The new APIs are going to be 1 to 1 mapped with the built-in
> > intrinsics (the memory orderings used themselves will not change). We
> > should go ahead with the review and conclude any issues. Once the
> > decision is made on what APIs to use, we can submit the next version using
> the APIs decided.
> >
> Thanks, Honnappa.
> 
> I have reviewed the memory orderings and I see no issues with them.   I do
> have a question regarding a comment - I'll pose it inline:
Fantastic, thank you.
I have an unrelated (to this patch) question for you below.

> 
> > >
> > > Thanks,
> > > Erik
> > >
> > > > ---
> > > >  lib/librte_timer/rte_timer.c | 90
> > > > +++++++++++++++++++++++++++++++----
> > > > ---------
> > > >  lib/librte_timer/rte_timer.h |  2 +-
> > > >  2 files changed, 65 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/lib/librte_timer/rte_timer.c
> > > > b/lib/librte_timer/rte_timer.c index 269e921..be0262d 100644
> > > > --- a/lib/librte_timer/rte_timer.c
> > > > +++ b/lib/librte_timer/rte_timer.c
> > > > @@ -10,7 +10,6 @@
> > > >  #include <assert.h>
> > > >  #include <sys/queue.h>
> > > >
> > > > -#include <rte_atomic.h>
> > > >  #include <rte_common.h>
> > > >  #include <rte_cycles.h>
> > > >  #include <rte_eal_memconfig.h>
> > > > @@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
> > > >
> > > >  	status.state = RTE_TIMER_STOP;
> > > >  	status.owner = RTE_TIMER_NO_OWNER;
> > > > -	tim->status.u32 = status.u32;
> > > > +	__atomic_store_n(&tim->status.u32, status.u32,
> > > > __ATOMIC_RELAXED);
> > > >  }
> > > >
> > > >  /*
> > > > @@ -239,9 +238,9 @@ timer_set_config_state(struct rte_timer *tim,
> > > >
> > > >  	/* wait that the timer is in correct status before update,
> > > >  	 * and mark it as being configured */
> > > > -	while (success == 0) {
> > > > -		prev_status.u32 = tim->status.u32;
> > > > +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> > > > __ATOMIC_RELAXED);
> > > >
> > > > +	while (success == 0) {
> > > >  		/* timer is running on another core
> > > >  		 * or ready to run on local core, exit
> > > >  		 */
> > > > @@ -258,9 +257,20 @@ timer_set_config_state(struct rte_timer *tim,
> > > >  		 * mark it atomically as being configured */
> > > >  		status.state = RTE_TIMER_CONFIG;
> > > >  		status.owner = (int16_t)lcore_id;
> > > > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > > > -					      prev_status.u32,
> > > > -					      status.u32);
> > > > +		/* If status is observed as RTE_TIMER_CONFIG earlier,
> > > > +		 * that's not going to cause any issues because the
> > > > +		 * pattern is read for status then read the other members.
> 
> I don't follow the above comment.  What is meant by "earlier"?
> 
> Thanks,
> Erik
I would rather change this comment to something similar to what is mentioned while changing to 'RUNNING' state.
'CONFIG' is also a locking state. I think it is much easier to understand.

> 
> > > > +		 * In one of the callers to timer_set_config_state
> > > > +		 * (the __rte_timer_reset) we set other members to the
> > > > +		 * structure (period, expire, f, arg) we want these
> > > > +		 * changes to be observed after our change to status.
> > > > +		 * So we need __ATOMIC_ACQUIRE here.
> > > > +		 */
> > > > +		success = __atomic_compare_exchange_n(&tim-
> > > > >status.u32,
> > > > +					      &prev_status.u32,
> > > > +					      status.u32, 0,
> > > > +					      __ATOMIC_ACQUIRE,
> > > > +					      __ATOMIC_RELAXED);
> > > >  	}
> > > >
> > > >  	ret_prev_status->u32 = prev_status.u32; @@ -279,20 +289,27 @@
> > > > timer_set_running_state(struct rte_timer *tim)
> > > >
> > > >  	/* wait that the timer is in correct status before update,
> > > >  	 * and mark it as running */
> > > > -	while (success == 0) {
> > > > -		prev_status.u32 = tim->status.u32;
> > > > +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> > > > __ATOMIC_RELAXED);
> > > >
> > > > +	while (success == 0) {
> > > >  		/* timer is not pending anymore */
> > > >  		if (prev_status.state != RTE_TIMER_PENDING)
> > > >  			return -1;
> > > >
> > > >  		/* here, we know that timer is stopped or pending,
> > > > -		 * mark it atomically as being configured */
> > > > +		 * mark it atomically as being running
> > > > +		 */
> > > >  		status.state = RTE_TIMER_RUNNING;
> > > >  		status.owner = (int16_t)lcore_id;
> > > > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > > > -					      prev_status.u32,
> > > > -					      status.u32);
> > > > +		/* RUNNING states are acting as locked states. If the
> > > > +		 * timer is in RUNNING state, the state cannot be changed
> > > > +		 * by other threads. So, we should use ACQUIRE here.
> > > > +		 */
> > > > +		success = __atomic_compare_exchange_n(&tim-
> > > > >status.u32,
> > > > +					      &prev_status.u32,
> > > > +					      status.u32, 0,
> > > > +					      __ATOMIC_ACQUIRE,
> > > > +					      __ATOMIC_RELAXED);
> > > >  	}
> > > >
> > > >  	return 0;
> > > > @@ -520,10 +537,12 @@ __rte_timer_reset(struct rte_timer *tim,
> > > > uint64_t expire,
> > > >
> > > >  	/* update state: as we are in CONFIG state, only us can modify
> > > >  	 * the state so we don't need to use cmpset() here */
> > > > -	rte_wmb();
> > > >  	status.state = RTE_TIMER_PENDING;
> > > >  	status.owner = (int16_t)tim_lcore;
> > > > -	tim->status.u32 = status.u32;
> > > > +	/* The "RELEASE" ordering guarantees the memory operations above
> > > > +	 * the status update are observed before the update by all threads
> > > > +	 */
> > > > +	__atomic_store_n(&tim->status.u32, status.u32,
> > > > __ATOMIC_RELEASE);
> > > >
> > > >  	if (tim_lcore != lcore_id || !local_is_locked)
> > > >  		rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock);
> > > > @@ -600,10 +619,12 @@ __rte_timer_stop(struct rte_timer *tim, int
> > > > local_is_locked,
> > > >  	}
> > > >
> > > >  	/* mark timer as stopped */
> > > > -	rte_wmb();
> > > >  	status.state = RTE_TIMER_STOP;
> > > >  	status.owner = RTE_TIMER_NO_OWNER;
> > > > -	tim->status.u32 = status.u32;
> > > > +	/* The "RELEASE" ordering guarantees the memory operations above
> > > > +	 * the status update are observed before the update by all threads
> > > > +	 */
> > > > +	__atomic_store_n(&tim->status.u32, status.u32,
> > > > __ATOMIC_RELEASE);
> > > >
> > > >  	return 0;
> > > >  }
> > > > @@ -637,7 +658,8 @@ rte_timer_stop_sync(struct rte_timer *tim)
> > > > int rte_timer_pending(struct rte_timer *tim)  {
> > > > -	return tim->status.state == RTE_TIMER_PENDING;
> > > > +	return __atomic_load_n(&tim->status.state,
> > > > +				__ATOMIC_RELAXED) ==
> > > > RTE_TIMER_PENDING;
> > > >  }
> > > >
> > > >  /* must be called periodically, run all timer that expired */ @@
> > > > -739,8
> > > > +761,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
> > > >  			/* remove from done list and mark timer as stopped
> > > */
> > > >  			status.state = RTE_TIMER_STOP;
> > > >  			status.owner = RTE_TIMER_NO_OWNER;
> > > > -			rte_wmb();
> > > > -			tim->status.u32 = status.u32;
> > > > +			/* The "RELEASE" ordering guarantees the memory
> > > > +			 * operations above the status update are observed
> > > > +			 * before the update by all threads
> > > > +			 */
> > > > +			__atomic_store_n(&tim->status.u32, status.u32,
> > > > +				__ATOMIC_RELEASE);
> > > >  		}
> > > >  		else {
> > > >  			/* keep it in list and mark timer as pending */ @@ -
> > > > 748,8 +774,12 @@ __rte_timer_manage(struct rte_timer_data
> > *timer_data)
> > > >  			status.state = RTE_TIMER_PENDING;
Is it better to set this to STOPPED since it is out of the run list? I think it is better for the understanding as well.

> > > >  			__TIMER_STAT_ADD(priv_timer, pending, 1);
> > > >  			status.owner = (int16_t)lcore_id;
> > > > -			rte_wmb();
> > > > -			tim->status.u32 = status.u32;
> > > > +			/* The "RELEASE" ordering guarantees the memory
> > > > +			 * operations above the status update are observed
> > > > +			 * before the update by all threads
> > > > +			 */
> > > > +			__atomic_store_n(&tim->status.u32, status.u32,
> > > > +				__ATOMIC_RELEASE);
> > > >  			__rte_timer_reset(tim, tim->expire + tim->period,
> > > >  				tim->period, lcore_id, tim->f, tim->arg, 1,
> > > >  				timer_data);
> > > > @@ -919,8 +949,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
> > > >  			/* remove from done list and mark timer as stopped
> > > */
> > > >  			status.state = RTE_TIMER_STOP;
> > > >  			status.owner = RTE_TIMER_NO_OWNER;
> > > > -			rte_wmb();
> > > > -			tim->status.u32 = status.u32;
> > > > +			/* The "RELEASE" ordering guarantees the memory
> > > > +			 * operations above the status update are observed
> > > > +			 * before the update by all threads
> > > > +			 */
> > > > +			__atomic_store_n(&tim->status.u32, status.u32,
> > > > +				__ATOMIC_RELEASE);
> > > >  		} else {
> > > >  			/* keep it in list and mark timer as pending */
> > > >  			rte_spinlock_lock(
> > > > @@ -928,8 +962,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
> > > >  			status.state = RTE_TIMER_PENDING;
> > > >  			__TIMER_STAT_ADD(data->priv_timer, pending, 1);
> > > >  			status.owner = (int16_t)this_lcore;
> > > > -			rte_wmb();
> > > > -			tim->status.u32 = status.u32;
> > > > +			/* The "RELEASE" ordering guarantees the memory
> > > > +			 * operations above the status update are observed
> > > > +			 * before the update by all threads
> > > > +			 */
> > > > +			__atomic_store_n(&tim->status.u32, status.u32,
> > > > +				__ATOMIC_RELEASE);
> > > >  			__rte_timer_reset(tim, tim->expire + tim->period,
> > > >  				tim->period, this_lcore, tim->f, tim->arg, 1,
> > > >  				data);
> > > > diff --git a/lib/librte_timer/rte_timer.h
> > > > b/lib/librte_timer/rte_timer.h index c6b3d45..df533fa 100644
> > > > --- a/lib/librte_timer/rte_timer.h
> > > > +++ b/lib/librte_timer/rte_timer.h
> > > > @@ -101,7 +101,7 @@ struct rte_timer  {
> > > >  	uint64_t expire;       /**< Time when timer expire. */
> > > >  	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
> > > > -	volatile union rte_timer_status status; /**< Status of timer. */
> > > > +	union rte_timer_status status; /**< Status of timer. */
> > > >  	uint64_t period;       /**< Period of timer (0 if not periodic). */
> > > >  	rte_timer_cb_t f;      /**< Callback function. */
> > > >  	void *arg;             /**< Argument to callback function. */
> > > > --
> > > > 2.7.4

Re: [dpdk-dev] [PATCH 2/2] lib/timer: relax barrier for status update

[Carrillo, Erik G]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Wednesday, April 8, 2020 4:56 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; Phil Yang
> <Phil.Yang@arm.com>; rsanford@akamai.com; dev@dpdk.org
> Cc: david.marchand@redhat.com; Burakov, Anatoly
> <anatoly.burakov@intel.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Gavin Hu <Gavin.Hu@arm.com>; nd
> <nd@arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> nd <nd@arm.com>
> Subject: RE: [PATCH 2/2] lib/timer: relax barrier for status update
> 
> <snip>
> 
> > >
> > > > > Subject: [PATCH 2/2] lib/timer: relax barrier for status update
> > > > >
> > > > > Volatile has no ordering semantics. The rte_timer structure
> > > > > defines timer status as a volatile variable and uses the
> > > > > rte_r/wmb barrier to guarantee inter-thread visibility.
> > > > >
> > > > > This patch optimized the volatile operation with c11 atomic
> > > > > operations and one-way barrier to save the performance penalty.
> > > > > According to the timer_perf_autotest benchmarking results, this
> > > > > patch can uplift 10%~16% timer appending performance, 3%~20%
> > > > > timer resetting performance and 45% timer callbacks scheduling
> > > > > performance on aarch64 and no loss in performance for x86.
> > > > >
> > > > > Suggested-by: Honnappa Nagarahalli
> > > > > <honnappa.nagarahalli@arm.com>
> > > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > >
> > > > Hi Phil,
> > > >
> > > > It seems like the consensus is to generally avoid replacing rte_atomic_*
> > > > interfaces with the GCC builtins directly.   In other areas of DPDK that
> are
> > > > being patched, are the <std_atomic.h> C11 APIs going to be
> investigated?
> > > It
> > > > seems like that decision will apply here as well.
> > > Agree. The new APIs are going to be 1 to 1 mapped with the built-in
> > > intrinsics (the memory orderings used themselves will not change).
> > > We should go ahead with the review and conclude any issues. Once the
> > > decision is made on what APIs to use, we can submit the next version
> > > using
> > the APIs decided.
> > >
> > Thanks, Honnappa.
> >
> > I have reviewed the memory orderings and I see no issues with them.   I do
> > have a question regarding a comment - I'll pose it inline:
> Fantastic, thank you.
> I have an unrelated (to this patch) question for you below.
> 
> >
> > > >
> > > > Thanks,
> > > > Erik
> > > >
> > > > > ---
> > > > >  lib/librte_timer/rte_timer.c | 90
> > > > > +++++++++++++++++++++++++++++++----
> > > > > ---------
> > > > >  lib/librte_timer/rte_timer.h |  2 +-
> > > > >  2 files changed, 65 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_timer/rte_timer.c
> > > > > b/lib/librte_timer/rte_timer.c index 269e921..be0262d 100644
> > > > > --- a/lib/librte_timer/rte_timer.c
> > > > > +++ b/lib/librte_timer/rte_timer.c
> > > > > @@ -10,7 +10,6 @@
> > > > >  #include <assert.h>
> > > > >  #include <sys/queue.h>
> > > > >
> > > > > -#include <rte_atomic.h>
> > > > >  #include <rte_common.h>
> > > > >  #include <rte_cycles.h>
> > > > >  #include <rte_eal_memconfig.h>
> > > > > @@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
> > > > >
> > > > >  	status.state = RTE_TIMER_STOP;
> > > > >  	status.owner = RTE_TIMER_NO_OWNER;
> > > > > -	tim->status.u32 = status.u32;
> > > > > +	__atomic_store_n(&tim->status.u32, status.u32,
> > > > > __ATOMIC_RELAXED);
> > > > >  }
> > > > >
> > > > >  /*

<... snipped ...>

> > > > > @@ -258,9 +257,20 @@ timer_set_config_state(struct rte_timer
> *tim,
> > > > >  		 * mark it atomically as being configured */
> > > > >  		status.state = RTE_TIMER_CONFIG;
> > > > >  		status.owner = (int16_t)lcore_id;
> > > > > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > > > > -					      prev_status.u32,
> > > > > -					      status.u32);
> > > > > +		/* If status is observed as RTE_TIMER_CONFIG
> earlier,
> > > > > +		 * that's not going to cause any issues because the
> > > > > +		 * pattern is read for status then read the other
> members.
> >
> > I don't follow the above comment.  What is meant by "earlier"?
> >
> > Thanks,
> > Erik
> I would rather change this comment to something similar to what is
> mentioned while changing to 'RUNNING' state.
> 'CONFIG' is also a locking state. I think it is much easier to understand.
> 

Ok, thanks - that makes sense.

< ... snipped ...>

> > > > > 748,8 +774,12 @@ __rte_timer_manage(struct rte_timer_data
> > > *timer_data)
> > > > >  			status.state = RTE_TIMER_PENDING;
> Is it better to set this to STOPPED since it is out of the run list? I think it is
> better for the understanding as well.
> 

In this location, we are dealing with periodic timers, and we are about to restart the current timer after it just expired and its callback was executed.  As I understand it, setting the state back to PENDING here will cause the timer_reset() call below to remove this timer from the list (run list) it's still in (and fix up the links from the previous to the next elements), update other bits of the data structure, and update stats.   That behavior would change if we set the state to STOPPED.  At least to me, it also seems like the PENDING state is still accurate conceptually since the periodic timer wasn't explicitly stopped by this processing.

Thanks,
Erik

> > > > >  			__TIMER_STAT_ADD(priv_timer, pending, 1);
> > > > >  			status.owner = (int16_t)lcore_id;
> > > > > -			rte_wmb();
> > > > > -			tim->status.u32 = status.u32;
> > > > > +			/* The "RELEASE" ordering guarantees the
> memory
> > > > > +			 * operations above the status update are
> observed
> > > > > +			 * before the update by all threads
> > > > > +			 */
> > > > > +			__atomic_store_n(&tim->status.u32,
> status.u32,
> > > > > +				__ATOMIC_RELEASE);
> > > > >  			__rte_timer_reset(tim, tim->expire + tim-
> >period,
> > > > >  				tim->period, lcore_id, tim->f, tim-
> >arg, 1,
> > > > >  				timer_data);
> > > > > @@ -919,8 +949,12 @@ rte_timer_alt_manage(uint32_t
> timer_data_id,
> > > > >  			/* remove from done list and mark timer as
> stopped
> > > > */
> > > > >  			status.state = RTE_TIMER_STOP;
> > > > >  			status.owner = RTE_TIMER_NO_OWNER;
> > > > > -			rte_wmb();
> > > > > -			tim->status.u32 = status.u32;
> > > > > +			/* The "RELEASE" ordering guarantees the
> memory
> > > > > +			 * operations above the status update are
> observed
> > > > > +			 * before the update by all threads
> > > > > +			 */
> > > > > +			__atomic_store_n(&tim->status.u32,
> status.u32,
> > > > > +				__ATOMIC_RELEASE);
> > > > >  		} else {
> > > > >  			/* keep it in list and mark timer as pending */
> > > > >  			rte_spinlock_lock(
> > > > > @@ -928,8 +962,12 @@ rte_timer_alt_manage(uint32_t
> timer_data_id,
> > > > >  			status.state = RTE_TIMER_PENDING;
> > > > >  			__TIMER_STAT_ADD(data->priv_timer,
> pending, 1);
> > > > >  			status.owner = (int16_t)this_lcore;
> > > > > -			rte_wmb();
> > > > > -			tim->status.u32 = status.u32;
> > > > > +			/* The "RELEASE" ordering guarantees the
> memory
> > > > > +			 * operations above the status update are
> observed
> > > > > +			 * before the update by all threads
> > > > > +			 */
> > > > > +			__atomic_store_n(&tim->status.u32,
> status.u32,
> > > > > +				__ATOMIC_RELEASE);
> > > > >  			__rte_timer_reset(tim, tim->expire + tim-
> >period,
> > > > >  				tim->period, this_lcore, tim->f, tim-
> >arg, 1,
> > > > >  				data);
> > > > > diff --git a/lib/librte_timer/rte_timer.h
> > > > > b/lib/librte_timer/rte_timer.h index c6b3d45..df533fa 100644
> > > > > --- a/lib/librte_timer/rte_timer.h
> > > > > +++ b/lib/librte_timer/rte_timer.h
> > > > > @@ -101,7 +101,7 @@ struct rte_timer  {
> > > > >  	uint64_t expire;       /**< Time when timer expire. */
> > > > >  	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
> > > > > -	volatile union rte_timer_status status; /**< Status of timer.
> */
> > > > > +	union rte_timer_status status; /**< Status of timer. */
> > > > >  	uint64_t period;       /**< Period of timer (0 if not periodic). */
> > > > >  	rte_timer_cb_t f;      /**< Callback function. */
> > > > >  	void *arg;             /**< Argument to callback function. */
> > > > > --
> > > > > 2.7.4

Re: [dpdk-dev] [PATCH 2/2] lib/timer: relax barrier for status update

[Phil Yang]

> -----Original Message-----
> From: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Sent: Friday, April 10, 2020 3:29 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Phil Yang
> <Phil.Yang@arm.com>; rsanford@akamai.com; dev@dpdk.org
> Cc: david.marchand@redhat.com; Burakov, Anatoly
> <anatoly.burakov@intel.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Gavin Hu <Gavin.Hu@arm.com>; nd
> <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH 2/2] lib/timer: relax barrier for status update
> 
> > -----Original Message-----
> > From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> > Sent: Wednesday, April 8, 2020 4:56 PM
> > To: Carrillo, Erik G <erik.g.carrillo@intel.com>; Phil Yang
> > <Phil.Yang@arm.com>; rsanford@akamai.com; dev@dpdk.org
> > Cc: david.marchand@redhat.com; Burakov, Anatoly
> > <anatoly.burakov@intel.com>; thomas@monjalon.net;
> jerinj@marvell.com;
> > hemant.agrawal@nxp.com; Gavin Hu <Gavin.Hu@arm.com>; nd
> > <nd@arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> > nd <nd@arm.com>
> > Subject: RE: [PATCH 2/2] lib/timer: relax barrier for status update
> >
> > <snip>
> >
> > > >
> > > > > > Subject: [PATCH 2/2] lib/timer: relax barrier for status update
> > > > > >
> > > > > > Volatile has no ordering semantics. The rte_timer structure
> > > > > > defines timer status as a volatile variable and uses the
> > > > > > rte_r/wmb barrier to guarantee inter-thread visibility.
> > > > > >
> > > > > > This patch optimized the volatile operation with c11 atomic
> > > > > > operations and one-way barrier to save the performance penalty.
> > > > > > According to the timer_perf_autotest benchmarking results, this
> > > > > > patch can uplift 10%~16% timer appending performance, 3%~20%
> > > > > > timer resetting performance and 45% timer callbacks scheduling
> > > > > > performance on aarch64 and no loss in performance for x86.
> > > > > >
> > > > > > Suggested-by: Honnappa Nagarahalli
> > > > > > <honnappa.nagarahalli@arm.com>
> > > > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > >
> > > > > Hi Phil,
> > > > >
> > > > > It seems like the consensus is to generally avoid replacing
> rte_atomic_*
> > > > > interfaces with the GCC builtins directly.   In other areas of DPDK that
> > are
> > > > > being patched, are the <std_atomic.h> C11 APIs going to be
> > investigated?
> > > > It
> > > > > seems like that decision will apply here as well.
> > > > Agree. The new APIs are going to be 1 to 1 mapped with the built-in
> > > > intrinsics (the memory orderings used themselves will not change).
> > > > We should go ahead with the review and conclude any issues. Once the
> > > > decision is made on what APIs to use, we can submit the next version
> > > > using
> > > the APIs decided.
> > > >
> > > Thanks, Honnappa.
> > >
> > > I have reviewed the memory orderings and I see no issues with them.   I
> do
> > > have a question regarding a comment - I'll pose it inline:
> > Fantastic, thank you.
> > I have an unrelated (to this patch) question for you below.
> >
> > >
> > > > >
> > > > > Thanks,
> > > > > Erik
> > > > >
> > > > > > ---
> > > > > >  lib/librte_timer/rte_timer.c | 90
> > > > > > +++++++++++++++++++++++++++++++----
> > > > > > ---------
> > > > > >  lib/librte_timer/rte_timer.h |  2 +-
> > > > > >  2 files changed, 65 insertions(+), 27 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/librte_timer/rte_timer.c
> > > > > > b/lib/librte_timer/rte_timer.c index 269e921..be0262d 100644
> > > > > > --- a/lib/librte_timer/rte_timer.c
> > > > > > +++ b/lib/librte_timer/rte_timer.c
> > > > > > @@ -10,7 +10,6 @@
> > > > > >  #include <assert.h>
> > > > > >  #include <sys/queue.h>
> > > > > >
> > > > > > -#include <rte_atomic.h>
> > > > > >  #include <rte_common.h>
> > > > > >  #include <rte_cycles.h>
> > > > > >  #include <rte_eal_memconfig.h>
> > > > > > @@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
> > > > > >
> > > > > >  	status.state = RTE_TIMER_STOP;
> > > > > >  	status.owner = RTE_TIMER_NO_OWNER;
> > > > > > -	tim->status.u32 = status.u32;
> > > > > > +	__atomic_store_n(&tim->status.u32, status.u32,
> > > > > > __ATOMIC_RELAXED);
> > > > > >  }
> > > > > >
> > > > > >  /*
> 
> <... snipped ...>
> 
> > > > > > @@ -258,9 +257,20 @@ timer_set_config_state(struct rte_timer
> > *tim,
> > > > > >  		 * mark it atomically as being configured */
> > > > > >  		status.state = RTE_TIMER_CONFIG;
> > > > > >  		status.owner = (int16_t)lcore_id;
> > > > > > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > > > > > -					      prev_status.u32,
> > > > > > -					      status.u32);
> > > > > > +		/* If status is observed as RTE_TIMER_CONFIG
> > earlier,
> > > > > > +		 * that's not going to cause any issues because the
> > > > > > +		 * pattern is read for status then read the other
> > members.
> > >
> > > I don't follow the above comment.  What is meant by "earlier"?
> > >
> > > Thanks,
> > > Erik
> > I would rather change this comment to something similar to what is
> > mentioned while changing to 'RUNNING' state.
> > 'CONFIG' is also a locking state. I think it is much easier to understand.
> >
> 
> Ok, thanks - that makes sense.

OK, thanks. 
I will modify the comments in V2  to: 
"CONFIG states are acting as locked states. If the timer is in CONFIG state, the state cannot be changed
by other threads. So, we should use ACQUIRE here."

Thanks,
Phil

> 
> < ... snipped ...>
> 
> > > > > > 748,8 +774,12 @@ __rte_timer_manage(struct rte_timer_data
> > > > *timer_data)
> > > > > >  			status.state = RTE_TIMER_PENDING;
> > Is it better to set this to STOPPED since it is out of the run list? I think it is
> > better for the understanding as well.
> >
> 
> In this location, we are dealing with periodic timers, and we are about to
> restart the current timer after it just expired and its callback was executed.
> As I understand it, setting the state back to PENDING here will cause the
> timer_reset() call below to remove this timer from the list (run list) it's still in
> (and fix up the links from the previous to the next elements), update other
> bits of the data structure, and update stats.   That behavior would change if
> we set the state to STOPPED.  At least to me, it also seems like the PENDING
> state is still accurate conceptually since the periodic timer wasn't explicitly
> stopped by this processing.

Yes. +1 for this.

> 
> Thanks,
> Erik
> 
> > > > > >  			__TIMER_STAT_ADD(priv_timer, pending, 1);
> > > > > >  			status.owner = (int16_t)lcore_id;
> > > > > > -			rte_wmb();
> > > > > > -			tim->status.u32 = status.u32;
> > > > > > +			/* The "RELEASE" ordering guarantees the
> > memory
> > > > > > +			 * operations above the status update are
> > observed
> > > > > > +			 * before the update by all threads
> > > > > > +			 */
> > > > > > +			__atomic_store_n(&tim->status.u32,
> > status.u32,
> > > > > > +				__ATOMIC_RELEASE);
> > > > > >  			__rte_timer_reset(tim, tim->expire + tim-
> > >period,
> > > > > >  				tim->period, lcore_id, tim->f, tim-
> > >arg, 1,
> > > > > >  				timer_data);
> > > > > > @@ -919,8 +949,12 @@ rte_timer_alt_manage(uint32_t
> > timer_data_id,
> > > > > >  			/* remove from done list and mark timer as
> > stopped
> > > > > */
> > > > > >  			status.state = RTE_TIMER_STOP;
> > > > > >  			status.owner = RTE_TIMER_NO_OWNER;
> > > > > > -			rte_wmb();
> > > > > > -			tim->status.u32 = status.u32;
> > > > > > +			/* The "RELEASE" ordering guarantees the
> > memory
> > > > > > +			 * operations above the status update are
> > observed
> > > > > > +			 * before the update by all threads
> > > > > > +			 */
> > > > > > +			__atomic_store_n(&tim->status.u32,
> > status.u32,
> > > > > > +				__ATOMIC_RELEASE);
> > > > > >  		} else {
> > > > > >  			/* keep it in list and mark timer as pending */
> > > > > >  			rte_spinlock_lock(
> > > > > > @@ -928,8 +962,12 @@ rte_timer_alt_manage(uint32_t
> > timer_data_id,
> > > > > >  			status.state = RTE_TIMER_PENDING;
> > > > > >  			__TIMER_STAT_ADD(data->priv_timer,
> > pending, 1);
> > > > > >  			status.owner = (int16_t)this_lcore;
> > > > > > -			rte_wmb();
> > > > > > -			tim->status.u32 = status.u32;
> > > > > > +			/* The "RELEASE" ordering guarantees the
> > memory
> > > > > > +			 * operations above the status update are
> > observed
> > > > > > +			 * before the update by all threads
> > > > > > +			 */
> > > > > > +			__atomic_store_n(&tim->status.u32,
> > status.u32,
> > > > > > +				__ATOMIC_RELEASE);
> > > > > >  			__rte_timer_reset(tim, tim->expire + tim-
> > >period,
> > > > > >  				tim->period, this_lcore, tim->f, tim-
> > >arg, 1,
> > > > > >  				data);
> > > > > > diff --git a/lib/librte_timer/rte_timer.h
> > > > > > b/lib/librte_timer/rte_timer.h index c6b3d45..df533fa 100644
> > > > > > --- a/lib/librte_timer/rte_timer.h
> > > > > > +++ b/lib/librte_timer/rte_timer.h
> > > > > > @@ -101,7 +101,7 @@ struct rte_timer  {
> > > > > >  	uint64_t expire;       /**< Time when timer expire. */
> > > > > >  	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
> > > > > > -	volatile union rte_timer_status status; /**< Status of timer.
> > */
> > > > > > +	union rte_timer_status status; /**< Status of timer. */
> > > > > >  	uint64_t period;       /**< Period of timer (0 if not periodic). */
> > > > > >  	rte_timer_cb_t f;      /**< Callback function. */
> > > > > >  	void *arg;             /**< Argument to callback function. */
> > > > > > --
> > > > > > 2.7.4

[dpdk-dev] [PATCH v2] lib/timer: relax barrier for status update

[Phil Yang]

Volatile has no ordering semantics. The rte_timer structure defines
timer status as a volatile variable and uses the rte_r/wmb barrier
to guarantee inter-thread visibility.

This patch optimized the volatile operation with c11 atomic operations
and one-way barrier to save the performance penalty. According to the
timer_perf_autotest benchmarking results, this patch can uplift 10%~16%
timer appending performance, 3%~20% timer resetting performance and 45%
timer callbacks scheduling performance on aarch64 and no loss in
performance for x86.

Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>

---
This patch depends on patch:
http://patchwork.dpdk.org/patch/65997/

v2:
1. Changed the memory ordering comment in timer_set_config_state.
2. It is still using built-ins as the wrapper functions for C11 built-ins
are not defined yet.

 lib/librte_timer/rte_timer.c | 85 ++++++++++++++++++++++++++++++--------------
 lib/librte_timer/rte_timer.h |  2 +-
 2 files changed, 60 insertions(+), 27 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 269e921..ba17216 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -10,7 +10,6 @@
 #include <assert.h>
 #include <sys/queue.h>
 
-#include <rte_atomic.h>
 #include <rte_common.h>
 #include <rte_cycles.h>
 #include <rte_eal_memconfig.h>
@@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
 
 	status.state = RTE_TIMER_STOP;
 	status.owner = RTE_TIMER_NO_OWNER;
-	tim->status.u32 = status.u32;
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELAXED);
 }
 
 /*
@@ -239,9 +238,9 @@ timer_set_config_state(struct rte_timer *tim,
 
 	/* wait that the timer is in correct status before update,
 	 * and mark it as being configured */
-	while (success == 0) {
-		prev_status.u32 = tim->status.u32;
+	prev_status.u32 = __atomic_load_n(&tim->status.u32, __ATOMIC_RELAXED);
 
+	while (success == 0) {
 		/* timer is running on another core
 		 * or ready to run on local core, exit
 		 */
@@ -258,9 +257,15 @@ timer_set_config_state(struct rte_timer *tim,
 		 * mark it atomically as being configured */
 		status.state = RTE_TIMER_CONFIG;
 		status.owner = (int16_t)lcore_id;
-		success = rte_atomic32_cmpset(&tim->status.u32,
-					      prev_status.u32,
-					      status.u32);
+		/* CONFIG states are acting as locked states. If the
+		 * timer is in CONFIG state, the state cannot be changed
+		 * by other threads. So, we should use ACQUIRE here.
+		 */
+		success = __atomic_compare_exchange_n(&tim->status.u32,
+					      &prev_status.u32,
+					      status.u32, 0,
+					      __ATOMIC_ACQUIRE,
+					      __ATOMIC_RELAXED);
 	}
 
 	ret_prev_status->u32 = prev_status.u32;
@@ -279,20 +284,27 @@ timer_set_running_state(struct rte_timer *tim)
 
 	/* wait that the timer is in correct status before update,
 	 * and mark it as running */
-	while (success == 0) {
-		prev_status.u32 = tim->status.u32;
+	prev_status.u32 = __atomic_load_n(&tim->status.u32, __ATOMIC_RELAXED);
 
+	while (success == 0) {
 		/* timer is not pending anymore */
 		if (prev_status.state != RTE_TIMER_PENDING)
 			return -1;
 
 		/* here, we know that timer is stopped or pending,
-		 * mark it atomically as being configured */
+		 * mark it atomically as being running
+		 */
 		status.state = RTE_TIMER_RUNNING;
 		status.owner = (int16_t)lcore_id;
-		success = rte_atomic32_cmpset(&tim->status.u32,
-					      prev_status.u32,
-					      status.u32);
+		/* RUNNING states are acting as locked states. If the
+		 * timer is in RUNNING state, the state cannot be changed
+		 * by other threads. So, we should use ACQUIRE here.
+		 */
+		success = __atomic_compare_exchange_n(&tim->status.u32,
+					      &prev_status.u32,
+					      status.u32, 0,
+					      __ATOMIC_ACQUIRE,
+					      __ATOMIC_RELAXED);
 	}
 
 	return 0;
@@ -520,10 +532,12 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t expire,
 
 	/* update state: as we are in CONFIG state, only us can modify
 	 * the state so we don't need to use cmpset() here */
-	rte_wmb();
 	status.state = RTE_TIMER_PENDING;
 	status.owner = (int16_t)tim_lcore;
-	tim->status.u32 = status.u32;
+	/* The "RELEASE" ordering guarantees the memory operations above
+	 * the status update are observed before the update by all threads
+	 */
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELEASE);
 
 	if (tim_lcore != lcore_id || !local_is_locked)
 		rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock);
@@ -600,10 +614,12 @@ __rte_timer_stop(struct rte_timer *tim, int local_is_locked,
 	}
 
 	/* mark timer as stopped */
-	rte_wmb();
 	status.state = RTE_TIMER_STOP;
 	status.owner = RTE_TIMER_NO_OWNER;
-	tim->status.u32 = status.u32;
+	/* The "RELEASE" ordering guarantees the memory operations above
+	 * the status update are observed before the update by all threads
+	 */
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELEASE);
 
 	return 0;
 }
@@ -637,7 +653,8 @@ rte_timer_stop_sync(struct rte_timer *tim)
 int
 rte_timer_pending(struct rte_timer *tim)
 {
-	return tim->status.state == RTE_TIMER_PENDING;
+	return __atomic_load_n(&tim->status.state,
+				__ATOMIC_RELAXED) == RTE_TIMER_PENDING;
 }
 
 /* must be called periodically, run all timer that expired */
@@ -739,8 +756,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
 			/* remove from done list and mark timer as stopped */
 			status.state = RTE_TIMER_STOP;
 			status.owner = RTE_TIMER_NO_OWNER;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 		}
 		else {
 			/* keep it in list and mark timer as pending */
@@ -748,8 +769,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
 			status.state = RTE_TIMER_PENDING;
 			__TIMER_STAT_ADD(priv_timer, pending, 1);
 			status.owner = (int16_t)lcore_id;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 			__rte_timer_reset(tim, tim->expire + tim->period,
 				tim->period, lcore_id, tim->f, tim->arg, 1,
 				timer_data);
@@ -919,8 +944,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
 			/* remove from done list and mark timer as stopped */
 			status.state = RTE_TIMER_STOP;
 			status.owner = RTE_TIMER_NO_OWNER;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 		} else {
 			/* keep it in list and mark timer as pending */
 			rte_spinlock_lock(
@@ -928,8 +957,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
 			status.state = RTE_TIMER_PENDING;
 			__TIMER_STAT_ADD(data->priv_timer, pending, 1);
 			status.owner = (int16_t)this_lcore;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 			__rte_timer_reset(tim, tim->expire + tim->period,
 				tim->period, this_lcore, tim->f, tim->arg, 1,
 				data);
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index c6b3d45..df533fa 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -101,7 +101,7 @@ struct rte_timer
 {
 	uint64_t expire;       /**< Time when timer expire. */
 	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
-	volatile union rte_timer_status status; /**< Status of timer. */
+	union rte_timer_status status; /**< Status of timer. */
 	uint64_t period;       /**< Period of timer (0 if not periodic). */
 	rte_timer_cb_t f;      /**< Callback function. */
 	void *arg;             /**< Argument to callback function. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] lib/timer: relax barrier for status update

[Honnappa Nagarahalli]

Hi Erik,

> Subject: [PATCH v2] lib/timer: relax barrier for status update
> 
> Volatile has no ordering semantics. The rte_timer structure defines timer
> status as a volatile variable and uses the rte_r/wmb barrier to guarantee
> inter-thread visibility.
> 
> This patch optimized the volatile operation with c11 atomic operations and
> one-way barrier to save the performance penalty. According to the
> timer_perf_autotest benchmarking results, this patch can uplift 10%~16%
> timer appending performance, 3%~20% timer resetting performance and 45%
> timer callbacks scheduling performance on aarch64 and no loss in
> performance for x86.
> 
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> 
> ---
> This patch depends on patch:
> http://patchwork.dpdk.org/patch/65997/
> 
> v2:
> 1. Changed the memory ordering comment in timer_set_config_state.
> 2. It is still using built-ins as the wrapper functions for C11 built-ins are not
> defined yet.
It is too late to get the wrapper functions done for 20.05. It was decided in yesterday's tech board meeting to go ahead with C11 atomic built-ins (since there is lot of code in DPDK that uses C11 built-ins). If there are no further comments, can you please provide your ack?

> 
>  lib/librte_timer/rte_timer.c | 85 ++++++++++++++++++++++++++++++-----------
> ---
>  lib/librte_timer/rte_timer.h |  2 +-
>  2 files changed, 60 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c index
> 269e921..ba17216 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -10,7 +10,6 @@
>  #include <assert.h>
>  #include <sys/queue.h>
> 
> -#include <rte_atomic.h>
>  #include <rte_common.h>
>  #include <rte_cycles.h>
>  #include <rte_eal_memconfig.h>
> @@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
> 
>  	status.state = RTE_TIMER_STOP;
>  	status.owner = RTE_TIMER_NO_OWNER;
> -	tim->status.u32 = status.u32;
> +	__atomic_store_n(&tim->status.u32, status.u32,
> __ATOMIC_RELAXED);
>  }
> 
>  /*
> @@ -239,9 +238,9 @@ timer_set_config_state(struct rte_timer *tim,
> 
>  	/* wait that the timer is in correct status before update,
>  	 * and mark it as being configured */
> -	while (success == 0) {
> -		prev_status.u32 = tim->status.u32;
> +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> __ATOMIC_RELAXED);
> 
> +	while (success == 0) {
>  		/* timer is running on another core
>  		 * or ready to run on local core, exit
>  		 */
> @@ -258,9 +257,15 @@ timer_set_config_state(struct rte_timer *tim,
>  		 * mark it atomically as being configured */
>  		status.state = RTE_TIMER_CONFIG;
>  		status.owner = (int16_t)lcore_id;
> -		success = rte_atomic32_cmpset(&tim->status.u32,
> -					      prev_status.u32,
> -					      status.u32);
> +		/* CONFIG states are acting as locked states. If the
> +		 * timer is in CONFIG state, the state cannot be changed
> +		 * by other threads. So, we should use ACQUIRE here.
> +		 */
> +		success = __atomic_compare_exchange_n(&tim->status.u32,
> +					      &prev_status.u32,
> +					      status.u32, 0,
> +					      __ATOMIC_ACQUIRE,
> +					      __ATOMIC_RELAXED);
>  	}
> 
>  	ret_prev_status->u32 = prev_status.u32; @@ -279,20 +284,27 @@
> timer_set_running_state(struct rte_timer *tim)
> 
>  	/* wait that the timer is in correct status before update,
>  	 * and mark it as running */
> -	while (success == 0) {
> -		prev_status.u32 = tim->status.u32;
> +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> __ATOMIC_RELAXED);
> 
> +	while (success == 0) {
>  		/* timer is not pending anymore */
>  		if (prev_status.state != RTE_TIMER_PENDING)
>  			return -1;
> 
>  		/* here, we know that timer is stopped or pending,
> -		 * mark it atomically as being configured */
> +		 * mark it atomically as being running
> +		 */
>  		status.state = RTE_TIMER_RUNNING;
>  		status.owner = (int16_t)lcore_id;
> -		success = rte_atomic32_cmpset(&tim->status.u32,
> -					      prev_status.u32,
> -					      status.u32);
> +		/* RUNNING states are acting as locked states. If the
> +		 * timer is in RUNNING state, the state cannot be changed
> +		 * by other threads. So, we should use ACQUIRE here.
> +		 */
> +		success = __atomic_compare_exchange_n(&tim->status.u32,
> +					      &prev_status.u32,
> +					      status.u32, 0,
> +					      __ATOMIC_ACQUIRE,
> +					      __ATOMIC_RELAXED);
>  	}
> 
>  	return 0;
> @@ -520,10 +532,12 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t
> expire,
> 
>  	/* update state: as we are in CONFIG state, only us can modify
>  	 * the state so we don't need to use cmpset() here */
> -	rte_wmb();
>  	status.state = RTE_TIMER_PENDING;
>  	status.owner = (int16_t)tim_lcore;
> -	tim->status.u32 = status.u32;
> +	/* The "RELEASE" ordering guarantees the memory operations above
> +	 * the status update are observed before the update by all threads
> +	 */
> +	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELEASE);
> 
>  	if (tim_lcore != lcore_id || !local_is_locked)
>  		rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock);
> @@ -600,10 +614,12 @@ __rte_timer_stop(struct rte_timer *tim, int
> local_is_locked,
>  	}
> 
>  	/* mark timer as stopped */
> -	rte_wmb();
>  	status.state = RTE_TIMER_STOP;
>  	status.owner = RTE_TIMER_NO_OWNER;
> -	tim->status.u32 = status.u32;
> +	/* The "RELEASE" ordering guarantees the memory operations above
> +	 * the status update are observed before the update by all threads
> +	 */
> +	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELEASE);
> 
>  	return 0;
>  }
> @@ -637,7 +653,8 @@ rte_timer_stop_sync(struct rte_timer *tim)  int
> rte_timer_pending(struct rte_timer *tim)  {
> -	return tim->status.state == RTE_TIMER_PENDING;
> +	return __atomic_load_n(&tim->status.state,
> +				__ATOMIC_RELAXED) ==
> RTE_TIMER_PENDING;
>  }
> 
>  /* must be called periodically, run all timer that expired */ @@ -739,8
> +756,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
>  			/* remove from done list and mark timer as stopped
> */
>  			status.state = RTE_TIMER_STOP;
>  			status.owner = RTE_TIMER_NO_OWNER;
> -			rte_wmb();
> -			tim->status.u32 = status.u32;
> +			/* The "RELEASE" ordering guarantees the memory
> +			 * operations above the status update are observed
> +			 * before the update by all threads
> +			 */
> +			__atomic_store_n(&tim->status.u32, status.u32,
> +				__ATOMIC_RELEASE);
>  		}
>  		else {
>  			/* keep it in list and mark timer as pending */ @@ -
> 748,8 +769,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
>  			status.state = RTE_TIMER_PENDING;
>  			__TIMER_STAT_ADD(priv_timer, pending, 1);
>  			status.owner = (int16_t)lcore_id;
> -			rte_wmb();
> -			tim->status.u32 = status.u32;
> +			/* The "RELEASE" ordering guarantees the memory
> +			 * operations above the status update are observed
> +			 * before the update by all threads
> +			 */
> +			__atomic_store_n(&tim->status.u32, status.u32,
> +				__ATOMIC_RELEASE);
>  			__rte_timer_reset(tim, tim->expire + tim->period,
>  				tim->period, lcore_id, tim->f, tim->arg, 1,
>  				timer_data);
> @@ -919,8 +944,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
>  			/* remove from done list and mark timer as stopped
> */
>  			status.state = RTE_TIMER_STOP;
>  			status.owner = RTE_TIMER_NO_OWNER;
> -			rte_wmb();
> -			tim->status.u32 = status.u32;
> +			/* The "RELEASE" ordering guarantees the memory
> +			 * operations above the status update are observed
> +			 * before the update by all threads
> +			 */
> +			__atomic_store_n(&tim->status.u32, status.u32,
> +				__ATOMIC_RELEASE);
>  		} else {
>  			/* keep it in list and mark timer as pending */
>  			rte_spinlock_lock(
> @@ -928,8 +957,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
>  			status.state = RTE_TIMER_PENDING;
>  			__TIMER_STAT_ADD(data->priv_timer, pending, 1);
>  			status.owner = (int16_t)this_lcore;
> -			rte_wmb();
> -			tim->status.u32 = status.u32;
> +			/* The "RELEASE" ordering guarantees the memory
> +			 * operations above the status update are observed
> +			 * before the update by all threads
> +			 */
> +			__atomic_store_n(&tim->status.u32, status.u32,
> +				__ATOMIC_RELEASE);
>  			__rte_timer_reset(tim, tim->expire + tim->period,
>  				tim->period, this_lcore, tim->f, tim->arg, 1,
>  				data);
> diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h index
> c6b3d45..df533fa 100644
> --- a/lib/librte_timer/rte_timer.h
> +++ b/lib/librte_timer/rte_timer.h
> @@ -101,7 +101,7 @@ struct rte_timer
>  {
>  	uint64_t expire;       /**< Time when timer expire. */
>  	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
> -	volatile union rte_timer_status status; /**< Status of timer. */
> +	union rte_timer_status status; /**< Status of timer. */
>  	uint64_t period;       /**< Period of timer (0 if not periodic). */
>  	rte_timer_cb_t f;      /**< Callback function. */
>  	void *arg;             /**< Argument to callback function. */
> --
> 2.7.4

Re: [dpdk-dev] [PATCH v2] lib/timer: relax barrier for status update

[Carrillo, Erik G]

Hi Honnappa,

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Thursday, April 23, 2020 3:06 PM
> To: Phil Yang <Phil.Yang@arm.com>; Carrillo, Erik G
> <erik.g.carrillo@intel.com>; rsanford@akamai.com; dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Gavin Hu <Gavin.Hu@arm.com>; nd
> <nd@arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> nd <nd@arm.com>
> Subject: RE: [PATCH v2] lib/timer: relax barrier for status update
> 
> Hi Erik,
> 
> > Subject: [PATCH v2] lib/timer: relax barrier for status update
> >
> > Volatile has no ordering semantics. The rte_timer structure defines
> > timer status as a volatile variable and uses the rte_r/wmb barrier to
> > guarantee inter-thread visibility.
> >
> > This patch optimized the volatile operation with c11 atomic operations
> > and one-way barrier to save the performance penalty. According to the
> > timer_perf_autotest benchmarking results, this patch can uplift
> > 10%~16% timer appending performance, 3%~20% timer resetting
> > performance and 45% timer callbacks scheduling performance on aarch64
> > and no loss in performance for x86.
> >
> > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> >
> > ---
> > This patch depends on patch:
> > http://patchwork.dpdk.org/patch/65997/
> >
> > v2:
> > 1. Changed the memory ordering comment in timer_set_config_state.
> > 2. It is still using built-ins as the wrapper functions for C11
> > built-ins are not defined yet.
> It is too late to get the wrapper functions done for 20.05. It was decided in
> yesterday's tech board meeting to go ahead with C11 atomic built-ins (since
> there is lot of code in DPDK that uses C11 built-ins). If there are no further
> comments, can you please provide your ack?
> 

Ok, thanks for letting me know.  Based on that decision,  I've taken another look 
and done some testing and it looks good to me.  I've made one comment in-line
below and acked it.

<... snipped ...>

> > @@ -258,9 +257,15 @@ timer_set_config_state(struct rte_timer *tim,
> >  		 * mark it atomically as being configured */
> >  		status.state = RTE_TIMER_CONFIG;
> >  		status.owner = (int16_t)lcore_id;
> > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > -					      prev_status.u32,
> > -					      status.u32);
> > +		/* CONFIG states are acting as locked states. If the
> > +		 * timer is in CONFIG state, the state cannot be changed
> > +		 * by other threads. So, we should use ACQUIRE here.
> > +		 */
> > +		success = __atomic_compare_exchange_n(&tim-
> >status.u32,
> > +					      &prev_status.u32,
> > +					      status.u32, 0,
> > +					      __ATOMIC_ACQUIRE,
> > +					      __ATOMIC_RELAXED);
> >  	}
> >
> >  	ret_prev_status->u32 = prev_status.u32; @@ -279,20 +284,27 @@
> > timer_set_running_state(struct rte_timer *tim)
> >
> >  	/* wait that the timer is in correct status before update,
> >  	 * and mark it as running */
> > -	while (success == 0) {
> > -		prev_status.u32 = tim->status.u32;
> > +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> > __ATOMIC_RELAXED);
> >
> > +	while (success == 0) {
> >  		/* timer is not pending anymore */
> >  		if (prev_status.state != RTE_TIMER_PENDING)
> >  			return -1;
> >
> >  		/* here, we know that timer is stopped or pending,

We know that the timer will be pending at this point... Since we're correcting the comment below, we can correct this part too.

With that change:
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

> > -		 * mark it atomically as being configured */
> > +		 * mark it atomically as being running
> > +		 */
> >  		status.state = RTE_TIMER_RUNNING;
> >  		status.owner = (int16_t)lcore_id;
> > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > -					      prev_status.u32,
> > -					      status.u32);
> > +		/* RUNNING states are acting as locked states. If the
> > +		 * timer is in RUNNING state, the state cannot be changed
> > +		 * by other threads. So, we should use ACQUIRE here.
> > +		 */
> > +		success = __atomic_compare_exchange_n(&tim-
> >status.u32,
> > +					      &prev_status.u32,
> > +					      status.u32, 0,
> > +					      __ATOMIC_ACQUIRE,
> > +					      __ATOMIC_RELAXED);
> >  	}
> >
> >  	return 0;

Thanks,
Erik

[dpdk-dev] [PATCH v3] lib/timer: relax barrier for status update

[Phil Yang]

Volatile has no ordering semantics. The rte_timer structure defines
timer status as a volatile variable and uses the rte_r/wmb barrier
to guarantee inter-thread visibility.

This patch optimized the volatile operation with c11 atomic operations
and one-way barrier to save the performance penalty. According to the
timer_perf_autotest benchmarking results, this patch can uplift 10%~16%
timer appending performance, 3%~20% timer resetting performance and 45%
timer callbacks scheduling performance on aarch64 and no loss in
performance for x86.

Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

---
This patch depends on patch:
http://patchwork.dpdk.org/patch/65997/

It is still using built-ins as the wrapper functions for C11 built-ins
are not defined yet

v3: fix typo.

v2:
1. Changed the memory ordering comment in timer_set_config_state.

 lib/librte_timer/rte_timer.c | 87 ++++++++++++++++++++++++++++++--------------
 lib/librte_timer/rte_timer.h |  2 +-
 2 files changed, 61 insertions(+), 28 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 269e921..6d19ce4 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -10,7 +10,6 @@
 #include <assert.h>
 #include <sys/queue.h>
 
-#include <rte_atomic.h>
 #include <rte_common.h>
 #include <rte_cycles.h>
 #include <rte_eal_memconfig.h>
@@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
 
 	status.state = RTE_TIMER_STOP;
 	status.owner = RTE_TIMER_NO_OWNER;
-	tim->status.u32 = status.u32;
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELAXED);
 }
 
 /*
@@ -239,9 +238,9 @@ timer_set_config_state(struct rte_timer *tim,
 
 	/* wait that the timer is in correct status before update,
 	 * and mark it as being configured */
-	while (success == 0) {
-		prev_status.u32 = tim->status.u32;
+	prev_status.u32 = __atomic_load_n(&tim->status.u32, __ATOMIC_RELAXED);
 
+	while (success == 0) {
 		/* timer is running on another core
 		 * or ready to run on local core, exit
 		 */
@@ -258,9 +257,15 @@ timer_set_config_state(struct rte_timer *tim,
 		 * mark it atomically as being configured */
 		status.state = RTE_TIMER_CONFIG;
 		status.owner = (int16_t)lcore_id;
-		success = rte_atomic32_cmpset(&tim->status.u32,
-					      prev_status.u32,
-					      status.u32);
+		/* CONFIG states are acting as locked states. If the
+		 * timer is in CONFIG state, the state cannot be changed
+		 * by other threads. So, we should use ACQUIRE here.
+		 */
+		success = __atomic_compare_exchange_n(&tim->status.u32,
+					      &prev_status.u32,
+					      status.u32, 0,
+					      __ATOMIC_ACQUIRE,
+					      __ATOMIC_RELAXED);
 	}
 
 	ret_prev_status->u32 = prev_status.u32;
@@ -279,20 +284,27 @@ timer_set_running_state(struct rte_timer *tim)
 
 	/* wait that the timer is in correct status before update,
 	 * and mark it as running */
-	while (success == 0) {
-		prev_status.u32 = tim->status.u32;
+	prev_status.u32 = __atomic_load_n(&tim->status.u32, __ATOMIC_RELAXED);
 
+	while (success == 0) {
 		/* timer is not pending anymore */
 		if (prev_status.state != RTE_TIMER_PENDING)
 			return -1;
 
-		/* here, we know that timer is stopped or pending,
-		 * mark it atomically as being configured */
+		/* we know that the timer will be pending at this point,
+		 * mark it atomically as being running
+		 */
 		status.state = RTE_TIMER_RUNNING;
 		status.owner = (int16_t)lcore_id;
-		success = rte_atomic32_cmpset(&tim->status.u32,
-					      prev_status.u32,
-					      status.u32);
+		/* RUNNING states are acting as locked states. If the
+		 * timer is in RUNNING state, the state cannot be changed
+		 * by other threads. So, we should use ACQUIRE here.
+		 */
+		success = __atomic_compare_exchange_n(&tim->status.u32,
+					      &prev_status.u32,
+					      status.u32, 0,
+					      __ATOMIC_ACQUIRE,
+					      __ATOMIC_RELAXED);
 	}
 
 	return 0;
@@ -520,10 +532,12 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t expire,
 
 	/* update state: as we are in CONFIG state, only us can modify
 	 * the state so we don't need to use cmpset() here */
-	rte_wmb();
 	status.state = RTE_TIMER_PENDING;
 	status.owner = (int16_t)tim_lcore;
-	tim->status.u32 = status.u32;
+	/* The "RELEASE" ordering guarantees the memory operations above
+	 * the status update are observed before the update by all threads
+	 */
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELEASE);
 
 	if (tim_lcore != lcore_id || !local_is_locked)
 		rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock);
@@ -600,10 +614,12 @@ __rte_timer_stop(struct rte_timer *tim, int local_is_locked,
 	}
 
 	/* mark timer as stopped */
-	rte_wmb();
 	status.state = RTE_TIMER_STOP;
 	status.owner = RTE_TIMER_NO_OWNER;
-	tim->status.u32 = status.u32;
+	/* The "RELEASE" ordering guarantees the memory operations above
+	 * the status update are observed before the update by all threads
+	 */
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELEASE);
 
 	return 0;
 }
@@ -637,7 +653,8 @@ rte_timer_stop_sync(struct rte_timer *tim)
 int
 rte_timer_pending(struct rte_timer *tim)
 {
-	return tim->status.state == RTE_TIMER_PENDING;
+	return __atomic_load_n(&tim->status.state,
+				__ATOMIC_RELAXED) == RTE_TIMER_PENDING;
 }
 
 /* must be called periodically, run all timer that expired */
@@ -739,8 +756,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
 			/* remove from done list and mark timer as stopped */
 			status.state = RTE_TIMER_STOP;
 			status.owner = RTE_TIMER_NO_OWNER;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 		}
 		else {
 			/* keep it in list and mark timer as pending */
@@ -748,8 +769,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
 			status.state = RTE_TIMER_PENDING;
 			__TIMER_STAT_ADD(priv_timer, pending, 1);
 			status.owner = (int16_t)lcore_id;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 			__rte_timer_reset(tim, tim->expire + tim->period,
 				tim->period, lcore_id, tim->f, tim->arg, 1,
 				timer_data);
@@ -919,8 +944,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
 			/* remove from done list and mark timer as stopped */
 			status.state = RTE_TIMER_STOP;
 			status.owner = RTE_TIMER_NO_OWNER;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 		} else {
 			/* keep it in list and mark timer as pending */
 			rte_spinlock_lock(
@@ -928,8 +957,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
 			status.state = RTE_TIMER_PENDING;
 			__TIMER_STAT_ADD(data->priv_timer, pending, 1);
 			status.owner = (int16_t)this_lcore;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 			__rte_timer_reset(tim, tim->expire + tim->period,
 				tim->period, this_lcore, tim->f, tim->arg, 1,
 				data);
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index c6b3d45..df533fa 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -101,7 +101,7 @@ struct rte_timer
 {
 	uint64_t expire;       /**< Time when timer expire. */
 	struct rte_timer *sl_next[MAX_SKIPLIST_DEPTH];
-	volatile union rte_timer_status status; /**< Status of timer. */
+	union rte_timer_status status; /**< Status of timer. */
 	uint64_t period;       /**< Period of timer (0 if not periodic). */
 	rte_timer_cb_t f;      /**< Callback function. */
 	void *arg;             /**< Argument to callback function. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] lib/timer: relax barrier for status update

[Phil Yang]

> -----Original Message-----
> From: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Sent: Friday, April 24, 2020 9:27 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Phil Yang
> <Phil.Yang@arm.com>; rsanford@akamai.com; dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Gavin Hu <Gavin.Hu@arm.com>; nd
> <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH v2] lib/timer: relax barrier for status update
> 
> Hi Honnappa,
> 
> > -----Original Message-----
> > From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> > Sent: Thursday, April 23, 2020 3:06 PM
> > To: Phil Yang <Phil.Yang@arm.com>; Carrillo, Erik G
> > <erik.g.carrillo@intel.com>; rsanford@akamai.com; dev@dpdk.org
> > Cc: thomas@monjalon.net; david.marchand@redhat.com; Ananyev,
> > Konstantin <konstantin.ananyev@intel.com>; jerinj@marvell.com;
> > hemant.agrawal@nxp.com; Gavin Hu <Gavin.Hu@arm.com>; nd
> > <nd@arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> > nd <nd@arm.com>
> > Subject: RE: [PATCH v2] lib/timer: relax barrier for status update
> >
> > Hi Erik,
> >
> > > Subject: [PATCH v2] lib/timer: relax barrier for status update
> > >
> > > Volatile has no ordering semantics. The rte_timer structure defines
> > > timer status as a volatile variable and uses the rte_r/wmb barrier to
> > > guarantee inter-thread visibility.
> > >
> > > This patch optimized the volatile operation with c11 atomic operations
> > > and one-way barrier to save the performance penalty. According to the
> > > timer_perf_autotest benchmarking results, this patch can uplift
> > > 10%~16% timer appending performance, 3%~20% timer resetting
> > > performance and 45% timer callbacks scheduling performance on aarch64
> > > and no loss in performance for x86.
> > >
> > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > >
> > > ---
> > > This patch depends on patch:
> > > http://patchwork.dpdk.org/patch/65997/
> > >
> > > v2:
> > > 1. Changed the memory ordering comment in timer_set_config_state.
> > > 2. It is still using built-ins as the wrapper functions for C11
> > > built-ins are not defined yet.
> > It is too late to get the wrapper functions done for 20.05. It was decided in
> > yesterday's tech board meeting to go ahead with C11 atomic built-ins (since
> > there is lot of code in DPDK that uses C11 built-ins). If there are no further
> > comments, can you please provide your ack?
> >
> 
> Ok, thanks for letting me know.  Based on that decision,  I've taken another
> look
> and done some testing and it looks good to me.  I've made one comment in-
> line
> below and acked it.
> 
> <... snipped ...>
> 
> > > @@ -258,9 +257,15 @@ timer_set_config_state(struct rte_timer *tim,
> > >  		 * mark it atomically as being configured */
> > >  		status.state = RTE_TIMER_CONFIG;
> > >  		status.owner = (int16_t)lcore_id;
> > > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > > -					      prev_status.u32,
> > > -					      status.u32);
> > > +		/* CONFIG states are acting as locked states. If the
> > > +		 * timer is in CONFIG state, the state cannot be changed
> > > +		 * by other threads. So, we should use ACQUIRE here.
> > > +		 */
> > > +		success = __atomic_compare_exchange_n(&tim-
> > >status.u32,
> > > +					      &prev_status.u32,
> > > +					      status.u32, 0,
> > > +					      __ATOMIC_ACQUIRE,
> > > +					      __ATOMIC_RELAXED);
> > >  	}
> > >
> > >  	ret_prev_status->u32 = prev_status.u32; @@ -279,20 +284,27 @@
> > > timer_set_running_state(struct rte_timer *tim)
> > >
> > >  	/* wait that the timer is in correct status before update,
> > >  	 * and mark it as running */
> > > -	while (success == 0) {
> > > -		prev_status.u32 = tim->status.u32;
> > > +	prev_status.u32 = __atomic_load_n(&tim->status.u32,
> > > __ATOMIC_RELAXED);
> > >
> > > +	while (success == 0) {
> > >  		/* timer is not pending anymore */
> > >  		if (prev_status.state != RTE_TIMER_PENDING)
> > >  			return -1;
> > >
> > >  		/* here, we know that timer is stopped or pending,
> 
> We know that the timer will be pending at this point... Since we're correcting
> the comment below, we can correct this part too.
> 
> With that change:
> Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

Thanks Erik.
Updated in v3.

Thanks,
Phil 

> 
> > > -		 * mark it atomically as being configured */
> > > +		 * mark it atomically as being running
> > > +		 */
> > >  		status.state = RTE_TIMER_RUNNING;
> > >  		status.owner = (int16_t)lcore_id;
> > > -		success = rte_atomic32_cmpset(&tim->status.u32,
> > > -					      prev_status.u32,
> > > -					      status.u32);
> > > +		/* RUNNING states are acting as locked states. If the
> > > +		 * timer is in RUNNING state, the state cannot be changed
> > > +		 * by other threads. So, we should use ACQUIRE here.
> > > +		 */
> > > +		success = __atomic_compare_exchange_n(&tim-
> > >status.u32,
> > > +					      &prev_status.u32,
> > > +					      status.u32, 0,
> > > +					      __ATOMIC_ACQUIRE,
> > > +					      __ATOMIC_RELAXED);
> > >  	}
> > >
> > >  	return 0;
> 
> Thanks,
> Erik

Re: [dpdk-dev] [PATCH v2] lib/timer: relax barrier for status update

[Thomas Monjalon]

20/04/2020 18:05, Phil Yang:
> This patch depends on patch:
> http://patchwork.dpdk.org/patch/65997/

In order to ease patch tracking, you should have kept the first patch
in the next version of your series. We don't split series in general.

Re: [dpdk-dev] [PATCH v2] lib/timer: relax barrier for status update

[Phil Yang]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Saturday, April 25, 2020 10:36 PM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: erik.g.carrillo@intel.com; rsanford@akamai.com;
> david.marchand@redhat.com; konstantin.ananyev@intel.com;
> jerinj@marvell.com; hemant.agrawal@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu <Gavin.Hu@arm.com>; nd
> <nd@arm.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] lib/timer: relax barrier for status update
> 
> 20/04/2020 18:05, Phil Yang:
> > This patch depends on patch:
> > http://patchwork.dpdk.org/patch/65997/
> 
> In order to ease patch tracking, you should have kept the first patch
> in the next version of your series. We don't split series in general.

Thanks for reminding me.
I will update the patch series in the new version.

Thanks,
Phil
> 
> 

Re: [dpdk-dev] [PATCH v2] lib/timer: relax barrier for status update

[Thomas Monjalon]

25/04/2020 17:51, Phil Yang:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 20/04/2020 18:05, Phil Yang:
> > > This patch depends on patch:
> > > http://patchwork.dpdk.org/patch/65997/
> > 
> > In order to ease patch tracking, you should have kept the first patch
> > in the next version of your series. We don't split series in general.
> 
> Thanks for reminding me.
> I will update the patch series in the new version.

No that's fine, I'm merging it already.
Next time :-)

Re: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status update

[Thomas Monjalon]

24/04/2020 09:24, Phil Yang:
> Volatile has no ordering semantics. The rte_timer structure defines
> timer status as a volatile variable and uses the rte_r/wmb barrier
> to guarantee inter-thread visibility.
> 
> This patch optimized the volatile operation with c11 atomic operations
> and one-way barrier to save the performance penalty. According to the
> timer_perf_autotest benchmarking results, this patch can uplift 10%~16%
> timer appending performance, 3%~20% timer resetting performance and 45%
> timer callbacks scheduling performance on aarch64 and no loss in
> performance for x86.
> 
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
[...]
> --- a/lib/librte_timer/rte_timer.h
> +++ b/lib/librte_timer/rte_timer.h
> @@ -101,7 +101,7 @@ struct rte_timer
> -	volatile union rte_timer_status status; /**< Status of timer. */
> +	union rte_timer_status status; /**< Status of timer. */

Unfortunately, I cannot merge this patch because it breaks the ABI:

  [C]'function void rte_timer_init(rte_timer*)' at rte_timer.c:214:1 has some indirect sub-type changes:
    parameter 1 of type 'rte_timer*' has sub-type changes:
      in pointed to type 'struct rte_timer' at rte_timer.h:100:1:
        type size hasn't changed
        1 data member changes (2 filtered):
         type of 'volatile rte_timer_status rte_timer::status' changed:
           entity changed from 'volatile rte_timer_status' to 'union rte_timer_status' at rte_timer.h:67:1
           type size hasn't changed

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/2] lib/timer: protect timer subsystem initialized with lock

[Thomas Monjalon]

> > rte_timer_subsystem_initialized is a global variable that can be accessed by
> > multiple processes simultaneously. Hence, any access to
> > rte_timer_subsystem_initialized should be protected by
> > rte_mcfg_timer_lock.
> > 
> > Fixes: f9d6cd8bfe9e ("timer: fix resource leak in finalize")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

Applied (without patch 2), thanks.

Re: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status update

[Phil Yang]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Sunday, April 26, 2020 1:18 AM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: erik.g.carrillo@intel.com; rsanford@akamai.com; dev@dpdk.org;
> david.marchand@redhat.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu <Gavin.Hu@arm.com>; nd
> <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status update
> 
> 24/04/2020 09:24, Phil Yang:
> > Volatile has no ordering semantics. The rte_timer structure defines
> > timer status as a volatile variable and uses the rte_r/wmb barrier
> > to guarantee inter-thread visibility.
> >
> > This patch optimized the volatile operation with c11 atomic operations
> > and one-way barrier to save the performance penalty. According to the
> > timer_perf_autotest benchmarking results, this patch can uplift 10%~16%
> > timer appending performance, 3%~20% timer resetting performance and
> 45%
> > timer callbacks scheduling performance on aarch64 and no loss in
> > performance for x86.
> >
> > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> [...]
> > --- a/lib/librte_timer/rte_timer.h
> > +++ b/lib/librte_timer/rte_timer.h
> > @@ -101,7 +101,7 @@ struct rte_timer
> > -	volatile union rte_timer_status status; /**< Status of timer. */
> > +	union rte_timer_status status; /**< Status of timer. */
> 
> Unfortunately, I cannot merge this patch because it breaks the ABI:
> 
>   [C]'function void rte_timer_init(rte_timer*)' at rte_timer.c:214:1 has some
> indirect sub-type changes:
>     parameter 1 of type 'rte_timer*' has sub-type changes:
>       in pointed to type 'struct rte_timer' at rte_timer.h:100:1:
>         type size hasn't changed
>         1 data member changes (2 filtered):
>          type of 'volatile rte_timer_status rte_timer::status' changed:
>            entity changed from 'volatile rte_timer_status' to 'union
> rte_timer_status' at rte_timer.h:67:1
>            type size hasn't changed
> 

I think we can revert it to the original definition of rte_timer and keep the union rte_timer_status volatile-qualified. 
Because with or without this 'volatile' qualify, it generates the same code on aarch64 and x86.
So it seems acceptable to ignore it to make the ABI compatible?

Thank,
Phil

Re: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status update

[Carrillo, Erik G]

> -----Original Message-----
> From: Phil Yang <Phil.Yang@arm.com>
> Sent: Sunday, April 26, 2020 2:36 AM
> To: thomas@monjalon.net
> Cc: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com;
> dev@dpdk.org; david.marchand@redhat.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu <Gavin.Hu@arm.com>; nd
> <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status update
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Sunday, April 26, 2020 1:18 AM
> > To: Phil Yang <Phil.Yang@arm.com>
> > Cc: erik.g.carrillo@intel.com; rsanford@akamai.com; dev@dpdk.org;
> > david.marchand@redhat.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; Gavin Hu <Gavin.Hu@arm.com>; nd
> > <nd@arm.com>
> > Subject: Re: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status
> > update
> >
> > 24/04/2020 09:24, Phil Yang:
> > > Volatile has no ordering semantics. The rte_timer structure defines
> > > timer status as a volatile variable and uses the rte_r/wmb barrier
> > > to guarantee inter-thread visibility.
> > >
> > > This patch optimized the volatile operation with c11 atomic
> > > operations and one-way barrier to save the performance penalty.
> > > According to the timer_perf_autotest benchmarking results, this
> > > patch can uplift 10%~16% timer appending performance, 3%~20% timer
> > > resetting performance and
> > 45%
> > > timer callbacks scheduling performance on aarch64 and no loss in
> > > performance for x86.
> > >
> > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > [...]
> > > --- a/lib/librte_timer/rte_timer.h
> > > +++ b/lib/librte_timer/rte_timer.h
> > > @@ -101,7 +101,7 @@ struct rte_timer
> > > -	volatile union rte_timer_status status; /**< Status of timer. */
> > > +	union rte_timer_status status; /**< Status of timer. */
> >
> > Unfortunately, I cannot merge this patch because it breaks the ABI:
> >
> >   [C]'function void rte_timer_init(rte_timer*)' at rte_timer.c:214:1
> > has some indirect sub-type changes:
> >     parameter 1 of type 'rte_timer*' has sub-type changes:
> >       in pointed to type 'struct rte_timer' at rte_timer.h:100:1:
> >         type size hasn't changed
> >         1 data member changes (2 filtered):
> >          type of 'volatile rte_timer_status rte_timer::status' changed:
> >            entity changed from 'volatile rte_timer_status' to 'union
> > rte_timer_status' at rte_timer.h:67:1
> >            type size hasn't changed
> >
> 
> I think we can revert it to the original definition of rte_timer and keep the
> union rte_timer_status volatile-qualified.
> Because with or without this 'volatile' qualify, it generates the same code on
> aarch64 and x86.
> So it seems acceptable to ignore it to make the ABI compatible?
> 
> Thank,
> Phil

I was wondering about this also.  Is the performance improvement on aarch64 still the same in that case?

Thanks,
Erik

Re: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status update

[Phil Yang]

> -----Original Message-----
> From: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Sent: Sunday, April 26, 2020 8:19 PM
> To: Phil Yang <Phil.Yang@arm.com>; thomas@monjalon.net
> Cc: rsanford@akamai.com; dev@dpdk.org; david.marchand@redhat.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Gavin Hu
> <Gavin.Hu@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status update
> 
> 
> 
> > -----Original Message-----
> > From: Phil Yang <Phil.Yang@arm.com>
> > Sent: Sunday, April 26, 2020 2:36 AM
> > To: thomas@monjalon.net
> > Cc: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com;
> > dev@dpdk.org; david.marchand@redhat.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; Gavin Hu <Gavin.Hu@arm.com>; nd
> > <nd@arm.com>; nd <nd@arm.com>
> > Subject: RE: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status
> update
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > Sent: Sunday, April 26, 2020 1:18 AM
> > > To: Phil Yang <Phil.Yang@arm.com>
> > > Cc: erik.g.carrillo@intel.com; rsanford@akamai.com; dev@dpdk.org;
> > > david.marchand@redhat.com; Honnappa Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; Gavin Hu <Gavin.Hu@arm.com>; nd
> > > <nd@arm.com>
> > > Subject: Re: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status
> > > update
> > >
> > > 24/04/2020 09:24, Phil Yang:
> > > > Volatile has no ordering semantics. The rte_timer structure defines
> > > > timer status as a volatile variable and uses the rte_r/wmb barrier
> > > > to guarantee inter-thread visibility.
> > > >
> > > > This patch optimized the volatile operation with c11 atomic
> > > > operations and one-way barrier to save the performance penalty.
> > > > According to the timer_perf_autotest benchmarking results, this
> > > > patch can uplift 10%~16% timer appending performance, 3%~20% timer
> > > > resetting performance and
> > > 45%
> > > > timer callbacks scheduling performance on aarch64 and no loss in
> > > > performance for x86.
> > > >
> > > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > > [...]
> > > > --- a/lib/librte_timer/rte_timer.h
> > > > +++ b/lib/librte_timer/rte_timer.h
> > > > @@ -101,7 +101,7 @@ struct rte_timer
> > > > -	volatile union rte_timer_status status; /**< Status of timer. */
> > > > +	union rte_timer_status status; /**< Status of timer. */
> > >
> > > Unfortunately, I cannot merge this patch because it breaks the ABI:
> > >
> > >   [C]'function void rte_timer_init(rte_timer*)' at rte_timer.c:214:1
> > > has some indirect sub-type changes:
> > >     parameter 1 of type 'rte_timer*' has sub-type changes:
> > >       in pointed to type 'struct rte_timer' at rte_timer.h:100:1:
> > >         type size hasn't changed
> > >         1 data member changes (2 filtered):
> > >          type of 'volatile rte_timer_status rte_timer::status' changed:
> > >            entity changed from 'volatile rte_timer_status' to 'union
> > > rte_timer_status' at rte_timer.h:67:1
> > >            type size hasn't changed
> > >
> >
> > I think we can revert it to the original definition of rte_timer and keep the
> > union rte_timer_status volatile-qualified.
> > Because with or without this 'volatile' qualify, it generates the same code
> on
> > aarch64 and x86.
> > So it seems acceptable to ignore it to make the ABI compatible?
> >
> > Thank,
> > Phil
> 
> I was wondering about this also.  Is the performance improvement on
> aarch64 still the same in that case?

Yes. it is. 
It got the same performance improvement on aarch64 and no performance loss on x86.

I will update it in v4.

[dpdk-dev] [PATCH v4] lib/timer: relax barrier for status update

[Phil Yang]

Volatile has no ordering semantics. The rte_timer structure defines
timer status as a volatile variable and uses the rte_r/wmb barrier
to guarantee inter-thread visibility.

This patch optimized the volatile operation with c11 atomic operations
and one-way barrier to save the performance penalty. According to the
timer_perf_autotest benchmarking results, this patch can uplift 10%~16%
timer appending performance, 3%~20% timer resetting performance and 45%
timer callbacks scheduling performance on aarch64 and no loss in
performance for x86.

Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
It is still using built-ins as the wrapper functions for C11 built-ins
are not defined yet

v4:
fix ABI break in rte_timer struct.

v3:
fix typo.

v2:
Changed the memory ordering comment in timer_set_config_state.

 lib/librte_timer/rte_timer.c | 87 ++++++++++++++++++++++++++++++--------------
 1 file changed, 60 insertions(+), 27 deletions(-)

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 269e921..6d19ce4 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -10,7 +10,6 @@
 #include <assert.h>
 #include <sys/queue.h>
 
-#include <rte_atomic.h>
 #include <rte_common.h>
 #include <rte_cycles.h>
 #include <rte_eal_memconfig.h>
@@ -218,7 +217,7 @@ rte_timer_init(struct rte_timer *tim)
 
 	status.state = RTE_TIMER_STOP;
 	status.owner = RTE_TIMER_NO_OWNER;
-	tim->status.u32 = status.u32;
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELAXED);
 }
 
 /*
@@ -239,9 +238,9 @@ timer_set_config_state(struct rte_timer *tim,
 
 	/* wait that the timer is in correct status before update,
 	 * and mark it as being configured */
-	while (success == 0) {
-		prev_status.u32 = tim->status.u32;
+	prev_status.u32 = __atomic_load_n(&tim->status.u32, __ATOMIC_RELAXED);
 
+	while (success == 0) {
 		/* timer is running on another core
 		 * or ready to run on local core, exit
 		 */
@@ -258,9 +257,15 @@ timer_set_config_state(struct rte_timer *tim,
 		 * mark it atomically as being configured */
 		status.state = RTE_TIMER_CONFIG;
 		status.owner = (int16_t)lcore_id;
-		success = rte_atomic32_cmpset(&tim->status.u32,
-					      prev_status.u32,
-					      status.u32);
+		/* CONFIG states are acting as locked states. If the
+		 * timer is in CONFIG state, the state cannot be changed
+		 * by other threads. So, we should use ACQUIRE here.
+		 */
+		success = __atomic_compare_exchange_n(&tim->status.u32,
+					      &prev_status.u32,
+					      status.u32, 0,
+					      __ATOMIC_ACQUIRE,
+					      __ATOMIC_RELAXED);
 	}
 
 	ret_prev_status->u32 = prev_status.u32;
@@ -279,20 +284,27 @@ timer_set_running_state(struct rte_timer *tim)
 
 	/* wait that the timer is in correct status before update,
 	 * and mark it as running */
-	while (success == 0) {
-		prev_status.u32 = tim->status.u32;
+	prev_status.u32 = __atomic_load_n(&tim->status.u32, __ATOMIC_RELAXED);
 
+	while (success == 0) {
 		/* timer is not pending anymore */
 		if (prev_status.state != RTE_TIMER_PENDING)
 			return -1;
 
-		/* here, we know that timer is stopped or pending,
-		 * mark it atomically as being configured */
+		/* we know that the timer will be pending at this point
+		 * mark it atomically as being running
+		 */
 		status.state = RTE_TIMER_RUNNING;
 		status.owner = (int16_t)lcore_id;
-		success = rte_atomic32_cmpset(&tim->status.u32,
-					      prev_status.u32,
-					      status.u32);
+		/* RUNNING states are acting as locked states. If the
+		 * timer is in RUNNING state, the state cannot be changed
+		 * by other threads. So, we should use ACQUIRE here.
+		 */
+		success = __atomic_compare_exchange_n(&tim->status.u32,
+					      &prev_status.u32,
+					      status.u32, 0,
+					      __ATOMIC_ACQUIRE,
+					      __ATOMIC_RELAXED);
 	}
 
 	return 0;
@@ -520,10 +532,12 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t expire,
 
 	/* update state: as we are in CONFIG state, only us can modify
 	 * the state so we don't need to use cmpset() here */
-	rte_wmb();
 	status.state = RTE_TIMER_PENDING;
 	status.owner = (int16_t)tim_lcore;
-	tim->status.u32 = status.u32;
+	/* The "RELEASE" ordering guarantees the memory operations above
+	 * the status update are observed before the update by all threads
+	 */
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELEASE);
 
 	if (tim_lcore != lcore_id || !local_is_locked)
 		rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock);
@@ -600,10 +614,12 @@ __rte_timer_stop(struct rte_timer *tim, int local_is_locked,
 	}
 
 	/* mark timer as stopped */
-	rte_wmb();
 	status.state = RTE_TIMER_STOP;
 	status.owner = RTE_TIMER_NO_OWNER;
-	tim->status.u32 = status.u32;
+	/* The "RELEASE" ordering guarantees the memory operations above
+	 * the status update are observed before the update by all threads
+	 */
+	__atomic_store_n(&tim->status.u32, status.u32, __ATOMIC_RELEASE);
 
 	return 0;
 }
@@ -637,7 +653,8 @@ rte_timer_stop_sync(struct rte_timer *tim)
 int
 rte_timer_pending(struct rte_timer *tim)
 {
-	return tim->status.state == RTE_TIMER_PENDING;
+	return __atomic_load_n(&tim->status.state,
+				__ATOMIC_RELAXED) == RTE_TIMER_PENDING;
 }
 
 /* must be called periodically, run all timer that expired */
@@ -739,8 +756,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
 			/* remove from done list and mark timer as stopped */
 			status.state = RTE_TIMER_STOP;
 			status.owner = RTE_TIMER_NO_OWNER;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 		}
 		else {
 			/* keep it in list and mark timer as pending */
@@ -748,8 +769,12 @@ __rte_timer_manage(struct rte_timer_data *timer_data)
 			status.state = RTE_TIMER_PENDING;
 			__TIMER_STAT_ADD(priv_timer, pending, 1);
 			status.owner = (int16_t)lcore_id;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 			__rte_timer_reset(tim, tim->expire + tim->period,
 				tim->period, lcore_id, tim->f, tim->arg, 1,
 				timer_data);
@@ -919,8 +944,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
 			/* remove from done list and mark timer as stopped */
 			status.state = RTE_TIMER_STOP;
 			status.owner = RTE_TIMER_NO_OWNER;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 		} else {
 			/* keep it in list and mark timer as pending */
 			rte_spinlock_lock(
@@ -928,8 +957,12 @@ rte_timer_alt_manage(uint32_t timer_data_id,
 			status.state = RTE_TIMER_PENDING;
 			__TIMER_STAT_ADD(data->priv_timer, pending, 1);
 			status.owner = (int16_t)this_lcore;
-			rte_wmb();
-			tim->status.u32 = status.u32;
+			/* The "RELEASE" ordering guarantees the memory
+			 * operations above the status update are observed
+			 * before the update by all threads
+			 */
+			__atomic_store_n(&tim->status.u32, status.u32,
+				__ATOMIC_RELEASE);
 			__rte_timer_reset(tim, tim->expire + tim->period,
 				tim->period, this_lcore, tim->f, tim->arg, 1,
 				data);
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status update

[Carrillo, Erik G]

> -----Original Message-----
> From: Phil Yang <Phil.Yang@arm.com>
> Sent: Sunday, April 26, 2020 9:20 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; thomas@monjalon.net
> Cc: rsanford@akamai.com; dev@dpdk.org; david.marchand@redhat.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Gavin Hu
> <Gavin.Hu@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>; nd
> <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status update
> 
> > -----Original Message-----
> > From: Carrillo, Erik G <erik.g.carrillo@intel.com>
> > Sent: Sunday, April 26, 2020 8:19 PM
> > To: Phil Yang <Phil.Yang@arm.com>; thomas@monjalon.net
> > Cc: rsanford@akamai.com; dev@dpdk.org; david.marchand@redhat.com;
> > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Gavin Hu
> > <Gavin.Hu@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> > Subject: RE: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for status
> > update
> >
> >
> >
> > > -----Original Message-----
> > > From: Phil Yang <Phil.Yang@arm.com>
> > > Sent: Sunday, April 26, 2020 2:36 AM
> > > To: thomas@monjalon.net
> > > Cc: Carrillo, Erik G <erik.g.carrillo@intel.com>;
> > > rsanford@akamai.com; dev@dpdk.org; david.marchand@redhat.com;
> > > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Gavin Hu
> > > <Gavin.Hu@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> > > Subject: RE: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for
> > > status
> > update
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > Sent: Sunday, April 26, 2020 1:18 AM
> > > > To: Phil Yang <Phil.Yang@arm.com>
> > > > Cc: erik.g.carrillo@intel.com; rsanford@akamai.com; dev@dpdk.org;
> > > > david.marchand@redhat.com; Honnappa Nagarahalli
> > > > <Honnappa.Nagarahalli@arm.com>; Gavin Hu <Gavin.Hu@arm.com>;
> nd
> > > > <nd@arm.com>
> > > > Subject: Re: [dpdk-dev] [PATCH v3] lib/timer: relax barrier for
> > > > status update
> > > >
> > > > 24/04/2020 09:24, Phil Yang:
> > > > > Volatile has no ordering semantics. The rte_timer structure
> > > > > defines timer status as a volatile variable and uses the
> > > > > rte_r/wmb barrier to guarantee inter-thread visibility.
> > > > >
> > > > > This patch optimized the volatile operation with c11 atomic
> > > > > operations and one-way barrier to save the performance penalty.
> > > > > According to the timer_perf_autotest benchmarking results, this
> > > > > patch can uplift 10%~16% timer appending performance, 3%~20%
> > > > > timer resetting performance and
> > > > 45%
> > > > > timer callbacks scheduling performance on aarch64 and no loss in
> > > > > performance for x86.
> > > > >
> > > > > Suggested-by: Honnappa Nagarahalli
> > > > > <honnappa.nagarahalli@arm.com>
> > > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > > Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > > > [...]
> > > > > --- a/lib/librte_timer/rte_timer.h
> > > > > +++ b/lib/librte_timer/rte_timer.h
> > > > > @@ -101,7 +101,7 @@ struct rte_timer
> > > > > -	volatile union rte_timer_status status; /**< Status of timer.
> */
> > > > > +	union rte_timer_status status; /**< Status of timer. */
> > > >
> > > > Unfortunately, I cannot merge this patch because it breaks the ABI:
> > > >
> > > >   [C]'function void rte_timer_init(rte_timer*)' at
> > > > rte_timer.c:214:1 has some indirect sub-type changes:
> > > >     parameter 1 of type 'rte_timer*' has sub-type changes:
> > > >       in pointed to type 'struct rte_timer' at rte_timer.h:100:1:
> > > >         type size hasn't changed
> > > >         1 data member changes (2 filtered):
> > > >          type of 'volatile rte_timer_status rte_timer::status' changed:
> > > >            entity changed from 'volatile rte_timer_status' to
> > > > 'union rte_timer_status' at rte_timer.h:67:1
> > > >            type size hasn't changed
> > > >
> > >
> > > I think we can revert it to the original definition of rte_timer and
> > > keep the union rte_timer_status volatile-qualified.
> > > Because with or without this 'volatile' qualify, it generates the
> > > same code
> > on
> > > aarch64 and x86.
> > > So it seems acceptable to ignore it to make the ABI compatible?
> > >
> > > Thank,
> > > Phil
> >
> > I was wondering about this also.  Is the performance improvement on
> > aarch64 still the same in that case?
> 
> Yes. it is.
> It got the same performance improvement on aarch64 and no performance
> loss on x86.
> 
> I will update it in v4.

Great - thanks, Phil.

Re: [dpdk-dev] [PATCH v4] lib/timer: relax barrier for status update

[Thomas Monjalon]

26/04/2020 16:45, Phil Yang:
> Volatile has no ordering semantics. The rte_timer structure defines
> timer status as a volatile variable and uses the rte_r/wmb barrier
> to guarantee inter-thread visibility.
> 
> This patch optimized the volatile operation with c11 atomic operations
> and one-way barrier to save the performance penalty. According to the
> timer_perf_autotest benchmarking results, this patch can uplift 10%~16%
> timer appending performance, 3%~20% timer resetting performance and 45%
> timer callbacks scheduling performance on aarch64 and no loss in
> performance for x86.
> 
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

Applied, thanks