[dpdk-dev] [PATCH v6 1/6] lib/eal: implement the family of rte bit operation APIs

[Gavin Hu]

Hi Stephen, Honnappa,

> -----Original Message-----
> From: Stephen Hemminger <stephen at networkplumber.org>
> Sent: Tuesday, December 24, 2019 12:37 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com>
> Cc: Joyce Kong <Joyce.Kong at arm.com>; thomas at monjalon.net;
> david.marchand at redhat.com; mb at smartsharesystems.com;
> jerinj at marvell.com; bruce.richardson at intel.com; ravi1.kumar at amd.com;
> rmody at marvell.com; shshaikh at marvell.com; xuanziyang2 at huawei.com;
> cloud.wangxiaoyun at huawei.com; zhouguoyang at huawei.com; Phil Yang
> <Phil.Yang at arm.com>; Gavin Hu <Gavin.Hu at arm.com>; nd <nd at arm.com>;
> dev at dpdk.org
> Subject: Re: [PATCH v6 1/6] lib/eal: implement the family of rte bit operation
> APIs
> 
> On Mon, 23 Dec 2019 05:04:12 +0000
> Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com> wrote:
> 
> > <snip>
> >
> > >
> > > On Sat, 21 Dec 2019 16:07:23 +0000
> > > Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com> wrote:
> > >
> > > > Converting these into macros will help remove the size based duplication
> of
> > > APIs. I came up with the following macro:
> > > >
> > > > #define RTE_GET_BIT(nr, var, ret, memorder) \ ({ \
> > > >     if (sizeof(var) == sizeof(uint32_t)) { \
> > > >         uint32_t mask1 = 1U << (nr)%32; \
> > > >         ret = __atomic_load_n(&var, (memorder)) & mask1;\
> > > >     } \
> > > >     else {\
> > > >         uint64_t mask2 = 1UL << (nr)%64;\
> > > >         ret = __atomic_load_n(&var, (memorder)) & mask2;\
> > > >     } \
> > > > })
> > >
> > > Macros are more error prone. Especially because this is in exposed header
> file
> > That's another question I have. Why do we need to have these APIs in a
> public header file? These will add to the ABI burden as well. These APIs should
> be in a common-but-not-public header file. I am also not sure how helpful
> these APIs are for applications as these APIs seem to have considered
> requirements only from the PMDs.
> 
> Why do we have to wrap every C atomic builtin? What value is there in that?

The wrapping is aimed to reduce code duplication, on average 3 lines cut down to 1 line for a single core.
Overall I am thinking this bitops APIs are targeted for use by PMDs only, applications can use C11 freely.
The initial thought for the new APIs came from the idea of consolidating the scattered bit operations all over the PMDs. It is unwise to expanding to applications or libraries, as different memory orderings are required and complexity generate. 

If the use cases are limited to PMDs, a 'volatile' or a compiler barrier is sufficient therefore the number of APIs can be saved by half. 
http://inbox.dpdk.org/dev/VI1PR08MB53766C30B5CDA00FB9FCE9678F2E0@VI1PR08MB5376.eurprd08.prod.outlook.com/

Any thoughts and comments are welcome!