Understood rte_mbuf_refcnt_update_blind()
should be good., it will take care the RTE_MBUF_REFCNT_ATOMIC Hanoh -----Original Message----- From: Olivier MATZ [mailto:olivier.m...@6wind.com] Sent: Thursday, November 16, 2017 10:42 AM To: Hanoch Haim (hhaim) Cc: Konstantin Ananyev; Ilya Matveychikov; dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v3] mbuf: cleanup rte_pktmbuf_lastseg(), fix atomic usage Hi Hanoh, On Thu, Nov 16, 2017 at 07:16:31AM +0000, Hanoch Haim (hhaim) wrote: > Hi Oliver, > > It's hard for me to follow this thread. Yes, here are some few tips to make it easier to follow: - avoid top-posting - prefix quoted lines with "> " - describe the problem and how you solve it in the commit log - one problem = one patch > 1) It is not about clear/not-clear, it is error prone to *replicate* code > that has the same logic. > > "I'm not convinced that: > > __rte_pktmbuf_reset_nb_segs(m); > > is clearer than: > > m->next = NULL; > m->nb_segs = 1; > > Anyway, I agree this should not be part of this patch. We should only keep > the fix. > " rte_mbuf_refcnt_update() was not used in rte_pktmbuf_prefree_seg() to avoid reading the refcount twice. The problem of having clear or unclear is fundamental. I don't see the point of having a function __rte_pktmbuf_reset_nb_segs(). Keeping the two affectations makes things explicit. > 2) This definitely does not look good. > All the point in my patch is to move the ref-cnt operations to set of > API that already taking care of RTE_MBUF_REFCNT_ATOMIC > > + /* We don't use rte_mbuf_refcnt_update() because we already > + * tested that refcnt != 1. > + */ > +#ifdef RTE_MBUF_REFCNT_ATOMIC > + ret = rte_atomic16_add_return(&m->refcnt_atomic, -1); > +#else > + ret = --m->refcnt; > +#endif > + if (ret != 0) > + return NULL; > We cannot use the existing API taking care of atomic refcount rte_mbuf_refcnt_update() because it would read the refcount twice. We cannot change the behavior of rte_mbuf_refcnt_update() because it's a public API. An option proposed by Konstantin is to introduce a new helper rte_mbuf_refcnt_update_blind() that does the same than rte_mbuf_refcnt_update() but without the first test. It think it is a bit overkill to have this function for one caller. That's why I end up with this patch. I don't see why it would be an issue to have a mbuf ifdef inside the mbuf code. Olivier
