From owner-freebsd-net@FreeBSD.ORG Fri Aug 17 14:28:16 2012 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id ECC571065676; Fri, 17 Aug 2012 14:28:15 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id C36158FC18; Fri, 17 Aug 2012 14:28:15 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 1BD77B9B2; Fri, 17 Aug 2012 10:28:15 -0400 (EDT) From: John Baldwin To: freebsd-net@freebsd.org Date: Fri, 17 Aug 2012 10:09:45 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p17; KDE/4.5.5; amd64; ; ) References: In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201208171009.45212.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 17 Aug 2012 10:28:15 -0400 (EDT) Cc: net@freebsd.org, Vijay Singh Subject: Re: a query in mb_free_ext() 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: Fri, 17 Aug 2012 14:28:16 -0000 On Thursday, August 16, 2012 11:33:12 pm Vijay Singh wrote: > Does anyone here understand this bit of code in mb_free_ext()? > > /* Free attached storage if this mbuf is the only reference to it. */ > if (*(m->m_ext.ref_cnt) == 1 || > atomic_fetchadd_int(m->m_ext.ref_cnt, -1) == 1) { > switch (m->m_ext.ext_type) { > case EXT_PACKET: /* The packet zone is special. */ > if (*(m->m_ext.ref_cnt) == 0) > *(m->m_ext.ref_cnt) = 1; > uma_zfree(zone_pack, m); > return; /* Job done. */ > > Why would *(m->m_ext.ref_cnt) == 0 be true when the if condition > checks only for ref_cnt to be 1? Should the atomic_fetchadd_int() be > checked for <= 1? atomic_fetchadd_int() returns the old value (so the value before the decrement). It would be simpler to read if the code was written as: if (atomic_fetchadd_int(m->m_ext.ref_cnt, -1) == 1) { switch (m->m_ext.ext_type) { case EXT_PACKET: /* Keep a reference on the cluster while it is in the packet zone. */ *m->m_ext.ref_cnt = 1; uma_zfree(zone_pack, m); return; ... } The check for == 1 and then a check against == 0 is a (possibly dubious) micro-optimization to handle the case where we are not racing with another CPU to drop the reference count on the cluster and can see that we have the sole reference in that it avoids the atomic_fetchadd_int() and the store in that case. OTOH, the check to see if the store should be done (and the resulting branch) is probably worse than just always doing the store. > Also, why is the packet zone special? Any history here? To my knowledge, the packet zone stores mbufs that have an attached cluster so that an mbuf+cluster can be allocated in one operation when needed rather than requiring separate allocations of an mbuf and a cluster and then tying the two together. m_getcl() pulls items out of this zone, and this zone builds mbuf+cluster items from the backing mbuf and cluster zones when it is depleted. -- John Baldwin