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?
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.
