> -----Original Message----- > From: Olivier Matz <[email protected]> > Sent: Wednesday, July 8, 2020 7:43 PM > To: Phil Yang <[email protected]> > Cc: Stephen Hemminger <[email protected]>; > [email protected]; [email protected]; [email protected]; > Honnappa Nagarahalli <[email protected]>; Ruifeng Wang > <[email protected]>; nd <[email protected]> > Subject: Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt > operations > > On Wed, Jul 08, 2020 at 04:48:33AM +0000, Phil Yang wrote: > > > -----Original Message----- > > > From: Stephen Hemminger <[email protected]> > > > Sent: Wednesday, July 8, 2020 1:13 AM > > > To: Phil Yang <[email protected]> > > > Cc: [email protected]; [email protected]; > [email protected]; > > > Honnappa Nagarahalli <[email protected]>; > > > [email protected]; Ruifeng Wang <[email protected]>; nd > > > <[email protected]> > > > Subject: Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt > > > operations > > > > > > On Tue, 7 Jul 2020 18:10:33 +0800 > > > Phil Yang <[email protected]> wrote: > > > > > > > + return (uint16_t)(__atomic_add_fetch((int16_t *)&shinfo- > > > >refcnt_atomic, > > > > + > > > > > > Why do you need so many casts here? > > > The type of refcnt_atomic is now uint16 after your patch. > > > > In the existing code, the input parameter type for this API is signed > > integer. > For example: > > drivers/net/netvsc/hn_rxtx.c:531 > > lib/librte_mbuf/rte_mbuf.h:1194 > > > > However, the output type of rte_mbuf_ext_refcnt related APIs is not > uniform. We use these typecast to consistent with the current API definition. > > Would it make sense to cast the increment instead?
Yes. It is better. Thanks. > > I mean: > > return __atomic_add_fetch(&m->refcnt, (uint16_t)value, > __ATOMIC_ACQ_REL); > > instead of: > > return (uint16_t)(__atomic_add_fetch((int16_t *)&m->refcnt, value, > __ATOMIC_ACQ_REL)); > > > The same could apply to __rte_pktmbuf_pinned_extbuf_decref() I think: > > > - if (likely(rte_atomic16_add_return > > - (&shinfo->refcnt_atomic, -1))) > > + if (likely(__atomic_add_fetch((int *)&shinfo->refcnt_atomic, -1, > > + __ATOMIC_ACQ_REL))) > > By the way, why was the cast was to (int *) in this case? I think > it can overwrite fields beside refcnt. Fixed in the next version. Thanks, Phil
