From owner-freebsd-net  Thu Feb  8 18:48:52 2001
Delivered-To: freebsd-net@freebsd.org
Received: from relay.butya.kz (butya-gw.butya.kz [212.154.129.94])
	by hub.freebsd.org (Postfix) with ESMTP
	id B44AF37B4EC; Thu,  8 Feb 2001 18:48:29 -0800 (PST)
Received: by relay.butya.kz (Postfix, from userid 1000)
	id 0B5A828867; Fri,  9 Feb 2001 08:48:26 +0600 (ALMT)
Received: from localhost (localhost [127.0.0.1])
	by relay.butya.kz (Postfix) with ESMTP
	id ED14D28695; Fri,  9 Feb 2001 08:48:26 +0600 (ALMT)
Date: Fri, 9 Feb 2001 08:48:26 +0600 (ALMT)
From: Boris Popov <bp@butya.kz>
To: Bosko Milekic <bmilekic@technokratis.com>
Cc: freebsd-arch@FreeBSD.ORG, freebsd-net@FreeBSD.ORG
Subject: Re: Sequential mbuf read/write extensions
In-Reply-To: <013201c09203$971be9c0$1f90c918@jehovah>
Message-ID: <Pine.BSF.4.21.0102090831010.24710-100000@lion.butya.kz>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: owner-freebsd-net@FreeBSD.ORG
Precedence: bulk
X-Loop: FreeBSD.org

On Thu, 8 Feb 2001, Bosko Milekic wrote:

> in mb_init(), the m->m_pkthdr.rcvif = NULL; can be ommitted, as
> MGETHDR() will do that. The m->m_len = 0 should stay for now.

	Ok.

> drivers that pre-allocate mbufs + clusters, they typically know the
> `count'), it turns out that it is cheaper to compute the count from
> the size than the optimal size from the count, in the mbuf case. If
> you don't mind, I would strongly recommend moving m_getm() to
> uipc_mbuf.c. Code that doesn't know the `size' but knows the `count'

	Agreed, that why this function have a prefix 'm_' :)

[code sample skipped]

> For this to work, though, m_getm() needs to be modified to free all of
> `top' chain if it can't get either a cluster or an mbuf. I don't know
> if this was intentional, but it seems to me that there is a subtle
> problem in m_getm() as it is now:
> 
> if (len > MINCLSIZE) {
>     MCLGET(m, M_TRYWAIT);
>     if ((m->m_flags & M_EXT) == 0) {
>        m_freem(m); <------ frees only one mbuf
	 ^^^^^^^^^^ cluster is not in the chain yet, so it have to be
freed.

>        return NULL;
> 			    }
> 		}
> 
> I think what may happen here is that you will leak your `top' chain if
> you fail to allocate a cluster.

	The original semantic was not to free an entire chain because
m_getm() do not reallocates original (top) mbuf(s) (which may contain
data) and only adds new mbufs/clusters if possible. So, the calls like
m_get(mb->mb_top) will not left the wild pointer. There is also simple way
to deal with such behavior:

	mtop = m_get(...);
	if (mtop == NULL)
		fail;
	if (m_getm(mtop) == NULL) {
		m_freem(mtop);
		fail;
	}

	Probably m_getm() should return error code rather than pointer to
mbuf to avoid confusion.

--
Boris Popov
http://www.butya.kz/~bp/



To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-net" in the body of the message