From owner-freebsd-arch Thu Feb 8 19: 7:44 2001 Delivered-To: freebsd-arch@freebsd.org Received: from VL-MS-MR001.sc1.videotron.ca (relais.videotron.ca [24.201.245.36]) by hub.freebsd.org (Postfix) with ESMTP id 1AC5837B401; Thu, 8 Feb 2001 19:07:22 -0800 (PST) Received: from jehovah ([24.201.144.31]) by VL-MS-MR001.sc1.videotron.ca (Netscape Messaging Server 4.15) with SMTP id G8GZC902.6Z2; Thu, 8 Feb 2001 22:07:21 -0500 Message-ID: <001301c09245$b7400a00$1f90c918@jehovah> From: "Bosko Milekic" To: "Boris Popov" Cc: , References: Subject: Re: Sequential mbuf read/write extensions Date: Thu, 8 Feb 2001 22:09:28 -0500 MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit X-Priority: 3 X-MSMail-Priority: Normal X-Mailer: Microsoft Outlook Express 5.00.2919.6700 X-MimeOLE: Produced By Microsoft MimeOLE V5.00.2919.6700 Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Boris Popov wrote: [...] > > 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. m_free() may be more appropriate than m_freem() then, but see below. > > 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. I understand this part, but what I think you missed in my comment is that m_getm() should probably free what it already allocated before finally failing. It may not need to free `top' because of the wild pointer, as you say. But think of this: m_getm() is called with a larger `size' - it decides that given the `size' it will need to allocate a total of exactly 6 mbufs and 6 clusters for each mbuf. It loops and allocates, succesfully, 5 of those mbufs and 5 clusters. So `top' chain has now grown and includes those mbufs. Then what happens in the last iteration is that it allocates the 6th mbuf OK (it has not yet placed it on the chain) and fails to allocate a cluster, so it frees just that one mbuf (and not the mbufs it allocated in prior iterations and attached to `top' chain) and returns NULL. Your code that calls m_getm() then just fails, leaving `top' with what it could allocate. Note that in my mail I said "assuming this is a leak," thus recognizing the possibility that you did this intentionally. :-) Right now, I'll assume that this _was_ intentional, as that is what I understand from the above. But in any case, if we do move this to uipc_mbuf.c, we need to do one of the following: (a) make m_getm() free what it allocated in previous loop iterations before it failed (as described above) or (b) leave m_getm() the way it is BUT write an additional function that will simply wrap the call to m_getm() and flush properly for it if it fails (EXACTLY like your code snippet above). I'll gladly settle for either, but if we do go with (b), then the m_freem() should be changed to an m_free(), as it reflects the fact that we are only freeing the one mbuf and we should document this behavior, certainly. If you want, I'll roll up a diff in a few days (once I get what is presently dragging in my "commit this" queue out) and commit it. If you prefer to do this yourself, then feel free. :-) > -- > Boris Popov > http://www.butya.kz/~bp/ Regards, Bosko. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message