From owner-freebsd-net@FreeBSD.ORG Mon Sep 21 14:52:59 2009 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7C10A1065676 for ; Mon, 21 Sep 2009 14:52:59 +0000 (UTC) (envelope-from brampton@gmail.com) Received: from mail-fx0-f222.google.com (mail-fx0-f222.google.com [209.85.220.222]) by mx1.freebsd.org (Postfix) with ESMTP id 03BBE8FC0C for ; Mon, 21 Sep 2009 14:52:58 +0000 (UTC) Received: by fxm22 with SMTP id 22so1517316fxm.36 for ; Mon, 21 Sep 2009 07:52:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:received:in-reply-to :references:date:x-google-sender-auth:message-id:subject:from:to:cc :content-type:content-transfer-encoding; bh=xurNrPoGc38XE/HzYoPQbrSdTJTxKAyFKR3lWgiUYe4=; b=dvvf/tXcHeBGMO45rd0Enoqjohihcw1dVhj2Gcrdupcy2nBiIRpd1o69icP1fgolCH Du80HlJ1KZb9LYYFqanwhj7T5FtO6D8cvjg4PftDyZla/z/xB8FfakF5OA9XA6DwEYA2 anHxByau4pTZA/sXG+yUs0urvYj9tiUScCbIE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=YTgv3VCM64WpPu6JzG70HrhIn9iP9bL6o82850KUnem4RrgZ+STFj/iIVD98dTE5Ob jpOEk8S05KUx4rwKCml1AjF/095rvtX2/8MmKj2QcAfRWGkU/lu7N0n9+XEefb+nKuAr W9nH2rwVXs2SWnU8OQ4334cA4iPes2GWJlGO4= MIME-Version: 1.0 Sender: brampton@gmail.com Received: by 10.223.53.149 with SMTP id m21mr1112250fag.101.1253544777986; Mon, 21 Sep 2009 07:52:57 -0700 (PDT) In-Reply-To: <20090921235604.U12163@delplex.bde.org> References: <20090921235604.U12163@delplex.bde.org> Date: Mon, 21 Sep 2009 15:52:55 +0100 X-Google-Sender-Auth: 97c416eef3d418f0 Message-ID: From: Andrew Brampton To: Bruce Evans Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: freebsd-net@freebsd.org Subject: Re: Is this a race in mbuf's refcounting? X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Sep 2009 14:52:59 -0000 2009/9/21 Bruce Evans : > On Mon, 21 Sep 2009, Andrew Brampton wrote: > >> I've been reading the FreeBSD source code to understand how mbufs are >> reference counted. However, there are a few bits of code that I'm >> wondering if they would fail under the exactly right timing. Take for >> example in uipc_mbuf.c: >> >> 286 static void >> 287 mb_dupcl(struct mbuf *n, struct mbuf *m) >> 288 { >> ... >> 293 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (*(m->m_ext.ref_cnt) =3D=3D 1) >> 294 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*(m->m_ext.re= f_cnt) +=3D 1; >> 295 =C2=A0 =C2=A0 =C2=A0 =C2=A0else >> 296 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0atomic_add_in= t(m->m_ext.ref_cnt, 1); >> ... >> 305 } >> >> Now, the way I understand this code is, if ref_cnt is 1, then it is >> not shared. In that case non-atomically increment ref_cnt. However, if >> ref_cnt was something else, then it is shared so update the value in >> an atomic way. This seems valid, however what happens if two threads >> call mb_dupcl at the same time with a non-shared m. Could they both >> evaluate the if on line 293 at the same time, and then both >> non-atomically increment ref_cnt? >> >> If this could happen then we have a lost update and our reference >> counting is broken. I've also noticed that in other places similar >> optimisations are made to avoid the atomic operation. >> >> So is this a problem? > > I don't see how it can work. > > Also, if the count was 1, then it should become 2, but there is nothing t= o > flush the store to memory. =C2=A0This seems to mainly enlarge the race wi= ndow > for the previous problem. > > Bruce > Sorry, are you agreeing or disagreeing with my original post? If you are disagreeing I would appreciate if you could explain the error in my ways. I see the following happening: Thread 1: Reads *(m->m_ext.ref_cnt) and determines it is 1, and enters the true branch of the if Thread 1: Then reads *(m->m_ext.ref_cnt) again (since it is volatile) Thread 2: Interrupts and reads *(m->m_ext.ref_cnt) and determines it is 1, and enters the true branch of the if Thread 2: Then reads *(m->m_ext.ref_cnt), adds one to it and stores the result (ie 2) Thread 1: Resumes with the value it had (ie 1) and adds one to it, and stores the result (ie 2) Due to this sequence we have lost an update, since the value of *(m->m_ext.ref_cnt) should be 3. Now if this if wasn't there and atomic_add_int is used the result will be 3. If you find a flaw in my logic please point it out. thanks Andrew