Re: [dpdk-dev] [PATCH v2 1/3] eal/arm64: add 128-bit atomic compare exchange

["Phil Yang (Arm Technology China)" <Phil.Yang@arm.com>]

> -----Original Message-----
> From: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Sent: Tuesday, June 25, 2019 2:16 PM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> thomas@monjalon.net; Phil Yang (Arm Technology China)
> <Phil.Yang@arm.com>
> Cc: dev@dpdk.org; hemant.agrawal@nxp.com; Gavin Hu (Arm Technology
> China) <Gavin.Hu@arm.com>; nd <nd@arm.com>; gage.eads@intel.com; nd
> <nd@arm.com>
> Subject: RE: [PATCH v2 1/3] eal/arm64: add 128-bit atomic compare exchange
> 
> > -----Original Message-----
> > From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> > Sent: Monday, June 24, 2019 11:11 PM
> > To: thomas@monjalon.net; Phil Yang (Arm Technology China)
> > <Phil.Yang@arm.com>
> > Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org;
> > hemant.agrawal@nxp.com; Gavin Hu (Arm Technology China)
> > <Gavin.Hu@arm.com>; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>;
> gage.eads@intel.com;
> > nd <nd@arm.com>
> > Subject: [EXT] RE: [PATCH v2 1/3] eal/arm64: add 128-bit atomic
> > compare exchange
> > >
> > > 24/06/2019 18:12, Honnappa Nagarahalli:
> > > > > > > +	} else {
> > > > > > > +		rte_panic("Invalid memory order\n");
> > > > > >
> > > > > >
> > > > > > rte_panic should be removed from library. In this case, I
> > > > > > think, invalid mo can go for strongest barrier.
> > > >
> > > > It is added here to capture programming errors.
> > > > Memory order can be passed during compilation or during run time.
> > > > 'rte_panic' supports both of these.
> > > > Adding code with strongest memory order will mask the programming
> error.
> > >
> > > An error must return a specific code from the function.
> > > rte_panic is really forbidden in libraries.
> > > We are in the process of removing all of them.
> > Thank you for clarifying.
> > In this particular use case, the API is similar to
> '__atomic_compare_exchange'
> > built-in. Users would expect similar behavior. If we are differing
> > from the standard behavior, we should document it in the API definition.
> 
> IMO, We should not differ from the standard behavior(return type) of
> atomic_compare_exchange.
> And we should not have rte_panic in library. IMO, Best option would be
> 1) If mo is compile time constant then check with RTE_BUILD_ON for static
> assert to find invalid mo
I think we need to add static assert in the rte_atomic128_cmp_exchange to check invalid mo, rather than put it in these 3 internal functions.
Because the mo parameter is passed from the rte_atomic128_cmp_exchange.

> 2) if mo is runtime value and it is invalid then move to strongest memory
> order to make functionally correct
When the mo is runtime value, the compiler will always use the strongest memory order to predict atomic operations. Please check the URL (https://gcc.godbolt.org/z/GDx6Ob) for the details. 
So I think, in these 3 internal functions (__rte_ldx128, __rte_strx128, __rte_casp), it is sufficient to just follow this rule.

Thanks,
Phil Yang
> 
> 
> >
> > >
> > >