> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Monday, July 6, 2020 6:04 PM
> To: Phil Yang <phil.y...@arm.com>
> Cc: erik.g.carri...@intel.com; dev@dpdk.org; jer...@marvell.com; Honnappa
> Nagarahalli <honnappa.nagaraha...@arm.com>; d...@linux.vnet.ibm.com;
> Ruifeng Wang <ruifeng.w...@arm.com>; Dharmik Thakkar
> <dharmik.thak...@arm.com>; nd <n...@arm.com>;
> david.march...@redhat.com; m...@ashroe.eu; Neil Horman
> <nhor...@tuxdriver.com>; Dodji Seketeli <do...@redhat.com>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] eventdev: relax smp barriers with c11
> atomics
>
> 02/07/2020 07:26, Phil Yang:
> > The implementation-specific opaque data is shared between arm and
> cancel
> > 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.
>
> [...]
> > --- 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.
>
