From owner-svn-src-head@FreeBSD.ORG Wed Aug 21 19:40:15 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id DB9B9CF1; Wed, 21 Aug 2013 19:40:14 +0000 (UTC) (envelope-from nparhar@gmail.com) Received: from mail-pa0-x233.google.com (mail-pa0-x233.google.com [IPv6:2607:f8b0:400e:c03::233]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 9FD972546; Wed, 21 Aug 2013 19:40:14 +0000 (UTC) Received: by mail-pa0-f51.google.com with SMTP id lf1so492040pab.38 for ; Wed, 21 Aug 2013 12:40:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=CjVw3cgCdr2fp3VTtzD1ZZB8eq348a8IS+VEQyTKIQY=; b=ZQWPUarIQbAJiTJe1uM8IyONtNsZgxCvJxVduoiBVSpg45ahbMk2qJkvY51MvybYa0 XyopTXak8pamtbqFk52xJ3vmgb7ia2Sf5t7o6swGClblpdMHLWQ/nQv9+ed6Rqmof+lw ZES0axLdXZtfjU7vaK5/Lgzvds/UhTEx31qqSo40fNMo4caTAtL/teiapnXppeBLrGH7 6dc4UeP/MsT3fQTaZjX5hqOc2oZSsDmGqCF9P/SPoZRZswy2033neCBKF+CI+UIE0Hmm UCc/uiItHx/N+3UhuN4sAL0Kx6Eh+XrNQMA5SGPHCNGNrZJIVER1RteThb/2LowKIiXW aG/g== X-Received: by 10.66.230.233 with SMTP id tb9mr11282537pac.38.1377114014184; Wed, 21 Aug 2013 12:40:14 -0700 (PDT) Received: from [10.192.166.0] (stargate.chelsio.com. [67.207.112.58]) by mx.google.com with ESMTPSA id s6sm11850237pan.12.1969.12.31.16.00.00 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 21 Aug 2013 12:40:13 -0700 (PDT) Sender: Navdeep Parhar Message-ID: <52151799.3020809@FreeBSD.org> Date: Wed, 21 Aug 2013 12:40:09 -0700 From: Navdeep Parhar User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:17.0) Gecko/20130819 Thunderbird/17.0.8 MIME-Version: 1.0 To: Andre Oppermann Subject: Re: M_NOFREE removal (was Re: svn commit: r254520 - in head/sys: kern sys) References: <201308191116.r7JBGsc6065793@svn.freebsd.org> <521256CE.6070706@FreeBSD.org> <5212587A.2080202@freebsd.org> <52128937.1010407@freebsd.org> <52129E55.30803@freebsd.org> <5214D7E8.1080106@freebsd.org> <5214ED19.40907@FreeBSD.org> <5215047C.8080107@freebsd.org> <521505A3.4020103@FreeBSD.org> <52151368.40803@freebsd.org> In-Reply-To: <52151368.40803@freebsd.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: svn-src-head@freebsd.org, freebsd-net@freebsd.org, Peter Grehan X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Aug 2013 19:40:15 -0000 On 08/21/13 12:22, Andre Oppermann wrote: > On 21.08.2013 20:23, Navdeep Parhar wrote: >> I believe we need an extra patch to get M_NOFREE correct. I've had it >> forever in some of my internal repos but never committed it upstream >> (just plain forgot). Since this stuff is fresh in your mind, can you >> review this: >> >> diff -r cd78031b7885 sys/sys/mbuf.h >> --- a/sys/sys/mbuf.h Fri Aug 16 13:35:26 2013 -0700 >> +++ b/sys/sys/mbuf.h Wed Aug 21 10:55:57 2013 -0700 >> @@ -535,6 +535,8 @@ m_free(struct mbuf *m) >> { >> struct mbuf *n = m->m_next; >> >> + if ((m->m_flags & (M_PKTHDR|M_NOFREE)) == (M_PKTHDR|M_NOFREE)) >> + m_tag_delete_chain(m, NULL); >> if (m->m_flags & M_EXT) >> mb_free_ext(m); >> else if ((m->m_flags & M_NOFREE) == 0) >> >> It prevents leaks of tags from M_NOFREE+pkthdr mbufs. > > Good point. Looks correct. > > But then I wonder if it is really a smart thing to allow single > mbufs (w/o M_EXT) to be M_NOFREE at the same time. They easily > get lost. If it doesn't have an external buffer attached there > is no way to know when and if it was freed. > > If M_NOFREE is only allowed together with M_EXT then the tag chain > delete should happen in mb_ext_free() next to 'skipmbuf' instead. > > Index: kern/uipc_mbuf.c > =================================================================== > --- kern/uipc_mbuf.c (revision 254605) > +++ kern/uipc_mbuf.c (working copy) > @@ -283,11 +283,6 @@ > KASSERT((m->m_flags & M_EXT) == M_EXT, ("%s: M_EXT not set", > __func__)); > KASSERT(m->m_ext.ref_cnt != NULL, ("%s: ref_cnt not set", > __func__)); > > - /* > - * check if the header is embedded in the cluster > - */ > - skipmbuf = (m->m_flags & M_NOFREE); > - > /* 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) { > @@ -328,8 +323,14 @@ > ("%s: unknown ext_type", __func__)); > } > } > - if (skipmbuf) > + > + /* > + * Do not free if the mbuf is embedded in the cluster. > + */ > + if (m->m_flags & M_NOFREE) { > + m_tag_delete_chain(m, NULL); > return; > + } The problem with this is that the mbuf may already be gone if it was embedded in the cluster that was just freed by the ext free. That's why skipmbuf is used to cache the M_NOFREE bit.