-----Original Message-----
From: Thomas Monjalon <redacted>
Sent: Monday, July 6, 2020 6:04 PM
To: Phil Yang <redacted>
Cc: erik.g.carrillo@intel.com; dev@dpdk.org; jerinj@marvell.com; Honnappa
Nagarahalli [off-list ref]; drc@linux.vnet.ibm.com;
Ruifeng Wang [off-list ref]; Dharmik Thakkar
[off-list ref]; nd [off-list ref];
david.marchand@redhat.com; mdr@ashroe.eu; Neil Horman
[off-list ref]; Dodji Seketeli [off-list ref]
Subject: Re: [dpdk-dev] [PATCH v2 4/4] eventdev: relax smp barriers with c11
atomics

02/07/2020 07:26, Phil Yang:

quoted

The implementation-specific opaque data is shared between arm and

cancel

quoted

operations. The state flag acts as a guard variable to make sure the
update of opaque data is synchronized. This patch uses c11 atomics with
explicit one way memory barrier instead of full barriers rte_smp_w/rmb()
to synchronize the opaque data between timer arm and cancel threads.

I think we should write C11 (uppercase).

Agreed. 
I will change it in the next version.

Please, in your explanations, try to be more specific.
Naming fields may help to make things clear.

OK. Thanks.

[...]

quoted

--- a/lib/librte_eventdev/rte_event_timer_adapter.h
+++ b/lib/librte_eventdev/rte_event_timer_adapter.h

@@ -467,7 +467,7 @@ struct rte_event_timer {
 	 *  - op: RTE_EVENT_OP_NEW
 	 *  - event_type: RTE_EVENT_TYPE_TIMER
 	 */
-	volatile enum rte_event_timer_state state;
+	enum rte_event_timer_state state;
 	/**< State of the event timer. */

Why do you remove the volatile keyword?
It is not explained in the commit log.

By using the C11 atomic operations, it will generate the same instructions for non-volatile and volatile version.
Please check the sample code here: https://gcc.godbolt.org/z/8x5rWs

This change is triggering a warning in the ABI check:
http://mails.dpdk.org/archives/test-report/2020-July/140440.html
Moving from volatile to non-volatile is probably not an issue.
I expect the code generated for the volatile case to work the same
in non-volatile case. Do you confirm?

They generate the same instructions, so either way will work.
Do I need to revert it to the volatile version?


Thanks,
Phil

In any case, we need an explanation and an ABI check exception.