From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [PATCH] mbuf: optimize refcnt handling during free Date: Fri, 27 Mar 2015 08:44:51 -0400 Message-ID: <20150327124451.GE5375@hmsreliant.think-freely.org> References: <1427393457-7080-1-git-send-email-zoltan.kiss@linaro.org> <20150327102533.GA5375@hmsreliant.think-freely.org> <2601191342CEEE43887BDE71AB97725821407F18@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: "dev-VfR2kkLFssw@public.gmane.org" To: "Ananyev, Konstantin" Return-path: Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB97725821407F18-pww93C2UFcwu0RiL9chJVbfspsVTdybXVpNB7YpNyf8@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Fri, Mar 27, 2015 at 10:48:20AM +0000, Ananyev, Konstantin wrote: >=20 >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Neil Horman > > Sent: Friday, March 27, 2015 10:26 AM > > To: Wiles, Keith > > Cc: dev-VfR2kkLFssw@public.gmane.org > > Subject: Re: [dpdk-dev] [PATCH] mbuf: optimize refcnt handling during= free > >=20 > > On Thu, Mar 26, 2015 at 09:00:33PM +0000, Wiles, Keith wrote: > > > > > > > > > On 3/26/15, 1:10 PM, "Zoltan Kiss" wrote: > > > > > > >The current way is not the most efficient: if m->refcnt is 1, the = second > > > >condition never evaluates, and we set it to 0. If refcnt > 1, the = 2nd > > > >condition fails again, although the code suggest otherwise to bran= ch > > > >prediction. Instead we should keep the second condition only, and = remove > > > >the > > > >duplicate set to zero. > > > > > > > >Signed-off-by: Zoltan Kiss > > > >--- > > > > lib/librte_mbuf/rte_mbuf.h | 5 +---- > > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > > >diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf= .h > > > >index 17ba791..3ec4024 100644 > > > >--- a/lib/librte_mbuf/rte_mbuf.h > > > >+++ b/lib/librte_mbuf/rte_mbuf.h > > > >@@ -764,10 +764,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m) > > > > { > > > > __rte_mbuf_sanity_check(m, 0); > > > > > > > >- if (likely (rte_mbuf_refcnt_read(m) =3D=3D 1) || > > > >- likely (rte_mbuf_refcnt_update(m, -1) =3D=3D 0)) { > > > >- > > > >- rte_mbuf_refcnt_set(m, 0); > > > >+ if (likely (rte_mbuf_refcnt_update(m, -1) =3D=3D 0)) { > > > > > > > > /* if this is an indirect mbuf, then > > > > * - detach mbuf > > > > > > I fell for this one too, but read Bruce=B9s email > > > http://dpdk.org/ml/archives/dev/2015-March/014481.html > >=20 > > This is still the right thing to do though, Bruce's reasoning is erro= neous. >=20 > No, it is not. I believe Bruce comments is absolutely correct here. >=20 You and bruce are wrong, I proved that below. > > Just because the return from rte_mbuf_refcnt_read returns 1, doesn't = mean you >=20 > It does. >=20 assertions are meaningless without evidence. > > are the last user of the mbuf, > > you are only guaranteed that if the update > > operation returns zero. > >=20 > > In other words: > > rte_mbuf_refcnt_update(m, -1) > >=20 > > is an atomic operation > >=20 > > if (likely (rte_mbuf_refcnt_read(m) =3D=3D 1) || > > likely (rte_mbuf_refcnt_update(m, -1) =3D=3D 0)) = { > >=20 > >=20 > > is not. > >=20 > > To illustrate, on two cpus, this might occur: > >=20 > > CPU0 CPU1 > > rte_mbuf_refcnt_read ... > > returns 1 rte_mbuf_refcnt_read > > ... returns 1 > > execute if clause execute if clause >=20 >=20 > If you have an mbuf with refcnt=3D=3DN and try to call free() for it N+= 1 times - > it is a bug in your code. At what point in time did I indicate this was about multiple frees? Plea= se re-read my post. > Such code wouldn't work properly doesn't matter do we use: >=20 > if (likely (rte_mbuf_refcnt_read(m) =3D=3D 1) || likely (rte_mbuf_refc= nt_update(m, -1) =3D=3D 0)) >=20 > or just:=20 > if (likely (rte_mbuf_refcnt_update(m, -1) =3D=3D 0)) >=20 > To illustrate it with your example: > Suppose m.refcnt=3D=3D1 >=20 > CPU0 executes:=20 >=20 > rte_pktmbuf_free(m1) > /*rte_mbuf_refcnt_update(m1, -1) returns 0, so we reset I'ts re= fcnt and next and put mbuf back to the pool.*/ >=20 > m2 =3D rte_pktmbuf_alloc(pool); > /*as m1 is 'free' alloc could return same mbuf here, i.e: m2 =3D=3D= m1. */ >=20 > /* m2 refcnt =3D=3D1 start using m2 */ >=20 Really missing the point here. > CPU1 executes: > rte_pktmbuf_free(m1) > /*rte_mbuf_refcnt_update(m1, -1) returns 0, so we reset I'ts re= fcnt and next and put mbuf back to the pool.*/ >=20 > We just returnend to the pool mbuf that is in use and caused silent mem= ory corruption of the mbuf's content. >=20 Still missing the point. Please see below > >=20 > > In the above scenario both cpus fell into the if clause because they = both held a > > pointer to the same buffer and both got a return value of one, so the= y skipped > > the update portion of the if clause and both executed the internal bl= ock of the > > conditional expression. you might be tempted to think thats ok, sinc= e that > > block just sets the refcnt to zero, and doing so twice isn't harmful,= but the > > entire purpose of that if conditional above was to ensure that only o= ne > > execution context ever executed the conditional for a given buffer. = Look at > > what else happens in that conditional: > >=20 > > static inline struct rte_mbuf* __attribute__((always_inline)) > > __rte_pktmbuf_prefree_seg(struct rte_mbuf *m) > > { > > __rte_mbuf_sanity_check(m, 0); > >=20 > > if (likely (rte_mbuf_refcnt_read(m) =3D=3D 1) || > > likely (rte_mbuf_refcnt_update(m, -1) =3D=3D = 0)) { > >=20 > > rte_mbuf_refcnt_set(m, 0); > >=20 > > /* if this is an indirect mbuf, then > > * - detach mbuf > > * - free attached mbuf segment > > */ > > if (RTE_MBUF_INDIRECT(m)) { > > struct rte_mbuf *md =3D RTE_MBUF_FROM_BADDR(m= ->buf_addr); > > rte_pktmbuf_detach(m); > > if (rte_mbuf_refcnt_update(md, -1) =3D=3D 0) > > __rte_mbuf_raw_free(md); > > } > > return(m); > > } > > return (NULL); > > } > >=20 > > If the buffer is indirect, another refcnt update occurs to the buf_ad= dr mbuf, > > and in the scenario I outlined above, that refcnt will underflow, lik= ely causing > > a buffer leak. Additionally, the return code of this function is des= igned to > > indicate to the caller if they were the last user of the buffer. In = the above > > scenario, two execution contexts will be told that they were, which i= s wrong. > >=20 > > Zoltans patch is a good fix >=20 > I don't think so. >=20 >=20 > >=20 > > Acked-by: Neil Horman >=20 >=20 > NACKed-by: Konstantin Ananyev >=20 Again, this has nothing to do with how many times you free an object and everything to do with why you use atomics here in the first place. The p= urpose of the if conditional in the above code is to ensure that the contents of= the conditional block only get executed a single time, correct? Ostensibly y= ou don't want to execution contexts getting in there at the same time right? If you have a single buffer with refcnt=3D1, and two cpus are executing c= ode that points to that buffer, and they both call __rte_pktmbuf_prefree_seg at ar= ound the same time, they can race and both wind up in that conditional block, = leading to underflow of the md pointer refcnt, which is bad. Lets look at another more practical example. lets imagine that that the = mbuf X is linked into a set that multiple cpus can query. X->refcnt is held by C= PU0, and is about to be freed using the above refcnt test model (a read follow= ed by an update that gets squashed, anda refcnt set in the free block. Basical= ly this pseudo code: if (refcnt_read(X) =3D=3D 1 || refcnt_update(X) =3D=3D ) { refcnt_set(X,0) mbuf_free(X) } at the same time CPU1 is preforming a lookup of our needed mbuf from the aforementioned set, finds it and takes a refcnt on it. CPU0 CPU1 if(refcnt_read(X)) search for mbuf X returns 1 get pointer to X ... refcnt_update(X,1) refcnt_set(X, 0) ... mbuf_free(X) After the following sequence X is freed but CPU1 is left thinking that it= has a valid reference to the mbuf. This is broken. As an alternate thought experiment, why use atomics here at all? X86 is = cache coherent right? (ignore that new processor support, as this code predate= s it). If all cpus are able to see a consistent state of a variable, and if ever= y context that has a pointer to a given mbuf also has a reference to an mbu= f, then it should be safe to simply use an integer here rather than an atomic, ri= ght? If you know that you have a reference to a pointer, just decrement the re= fcnt and check for 0 instead of one, that will tell you that you are the last = user of a buffer, right? The answer is you can't because there are conditions in= which you either need to make a set of conditions atomic (finding a pointer and increasing said refcnt under the protection of a lock), or you need some = method to predicate the execution of some initial or finilazation event (like in __rte_pktmbuf_prefree_seg so that you don't have multiple contexts doing = that same init/finalization and so that you don't provide small windows of inconsistency in your atomics, which is what you have above. I wrote a demonstration program to illustrate (forgive me, its pretty qui= ck and dirty), but I think it illustrates the point: #define _GNU_SOURCE #include #include #include #include atomic_uint_fast64_t refcnt; uint threads =3D 0; static void * thread_exec(void *arg) { int i; int cpu =3D (int)(arg); cpu_set_t cpuset; pthread_t thread; thread =3D pthread_self(); CPU_ZERO(&cpuset); CPU_SET(cpu, &cpuset); pthread_setaffinity_np(thread, sizeof(cpu_set_t), &cpuset); for (i=3D0; i < 1000; i++) { if (((atomic_fetch_sub(&refcnt, 0) =3D=3D 1) || atomic_fetch_sub(&refcnt, 1) =3D=3D 0)) { // There should only ever be one thread in here a= t a atomic_init(&refcnt, 0); threads |=3D cpu; printf("threads =3D %d\n", threads); threads &=3D ~cpu; // Need to reset the refcnt for future iterations // but that should be fine since no other thread // should be in here but us atomic_init(&refcnt, 1); } } pthread_exit(NULL); } int main(int argc, char **argv) { pthread_attr_t attr; pthread_t thread_id1, thread_id2; void *status; atomic_init(&refcnt, 1); pthread_attr_init(&attr); pthread_create(&thread_id1, &attr, thread_exec, (void *)1); pthread_create(&thread_id2, &attr, thread_exec, (void *)2); pthread_attr_destroy(&attr); pthread_join(thread_id1, &status); pthread_join(thread_id2, &status); exit(0); } If you run this on an smp system, you'll clearly see that, occasionally t= he value of threads is 3. That indicates that you have points where you hav= e multiple contexts executing in that conditional block that has clearly be= en coded to only expect one. You can't make the assumption that every point= er has a held refcount here, you need to incur the update penalty. Neil