> -----Original Message----- > From: Olivier Matz <olivier.m...@6wind.com> > Sent: Wednesday, July 8, 2020 7:43 PM > To: Phil Yang <phil.y...@arm.com> > Cc: Stephen Hemminger <step...@networkplumber.org>; > david.march...@redhat.com; dev@dpdk.org; d...@linux.vnet.ibm.com; > Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>; Ruifeng Wang > <ruifeng.w...@arm.com>; nd <n...@arm.com> > 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 <step...@networkplumber.org> > > > Sent: Wednesday, July 8, 2020 1:13 AM > > > To: Phil Yang <phil.y...@arm.com> > > > Cc: david.march...@redhat.com; dev@dpdk.org; > d...@linux.vnet.ibm.com; > > > Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>; > > > olivier.m...@6wind.com; Ruifeng Wang <ruifeng.w...@arm.com>; nd > > > <n...@arm.com> > > > Subject: Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt > > > operations > > > > > > On Tue, 7 Jul 2020 18:10:33 +0800 > > > Phil Yang <phil.y...@arm.com> 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
