From owner-freebsd-net Thu Feb 8 11:14:42 2001 Delivered-To: freebsd-net@freebsd.org Received: from VL-MS-MR002.sc1.videotron.ca (relais.videotron.ca [24.201.245.36]) by hub.freebsd.org (Postfix) with ESMTP id 79DF937B684; Thu, 8 Feb 2001 11:14:16 -0800 (PST) Received: from jehovah ([24.201.144.31]) by VL-MS-MR002.sc1.videotron.ca (Netscape Messaging Server 4.15) with SMTP id G8GDFD04.BFD; Thu, 8 Feb 2001 14:14:01 -0500 Message-ID: <013201c09203$971be9c0$1f90c918@jehovah> From: "Bosko Milekic" To: "Boris Popov" Cc: , References: Subject: Re: Sequential mbuf read/write extensions Date: Thu, 8 Feb 2001 14:16:07 -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-net@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org Boris Popov wrote: [...] > Not exactly so. 'option LIBMBUF' will just connect the source file > to kernel makefile. There is no need for any #ifdef's in the code. Right. But I assume LIBMBUF will absolutely be needed if code that uses the routines is compiled. What I just meant to say was: "when the code using these routines grows to be significant enough, then we can just remove the option." > > #define M_TRYWAIT M_WAIT is not right. > > (M_WAIT is no longer to be used in the mbuf code.) > > You omitted the surrounding "#ifndef M_TRYWAIT" which makes this > code portable to RELENG_4 (mind you, this code taken from smbfs). Of > course, this should be stripped before import. I did, you're right. I guess I saw the "ifndef" wrong... I read this with only -CURRENT in mind and was afraid that the mbuf code flags would start mixing in with the malloc code flags -- something I tried to fight off in the past while. > > The succesfull return values are 0, I don't have a problem with this, > > specifically, but I would assume that this: > > if (!mb_init(mbp)) ... would be more "logical" (I use the term > > loosely) if it meant: "if initialization fails" (now it means "if > > initialization is succesful"). > > I'm generally don't like such syntax if function or variable name > do not clearly specify which value it should have/return on success. > Nearly all functions in this file return zero or error code, so the > correct syntax of the above will be: > > error = mb_init(mbp); > if (!error) > > or > > if (error) > return error; > > or > > if (mb_init(mbp) != 0) > return ESOMETHINGEVIL; OK. > > > significantly affected. The names of source and header files are > > > questionable too and I would appreciate good suggestions (currently > > they > > > are subr_mbuf.c and subr_mbuf.h). > > > > Hmmm. Maybe subr_mblib.c and libmb.h ? I don't want to turn this > > into a bikeshed ( :-) ), so I suggest that you decide. Personally, I > > would prefer that it be something other than "subr_mbuf.c" simply > > because it may be a little misleading in some cases. > > Good point. > > > Boris, this is really a great interface and nice looking, clean code. > > I'm sure, this code can be significantly improved by mbuf gurus :) > > -- > Boris Popov > http://www.butya.kz/~bp/ Ok, I have a few things to add (although I'm sure you'll be more into reading Ian Dowse's comments) :-) in mb_append_record(), you walk all the "record" mbufs to get to the last "record." How good would be the tradeoff? i.e. keeping a pointer to the last pkt in the mbdata structure's mbuf chain? We would grow the structure by a pointer, and we may have to maintain the last record pointer; but isn't the only place where we would have to "maintain it" in mb_append_record() anyway? 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. m_getm() looks like it should belong in uipc_mbuf.c -- it looks quite a bit like the "all or nothing" allocation routine I have sitting here. The difference is that mine doesn't take size as an argument, but rather the actual count of mbufs and all it does is allocate `count' mbufs and attach a cluster to each one of them. If it can't allocate a cluster or an mbuf at any point, it frees everything and returns. Now that I think about it, I'd much rather have `size' passed in instead, even though some callers may not know the `size' (think 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' (like some driver code) can do; m = m_get(M_TRYWAIT, MT_DATA); if (m == NULL) { /* we can't even allocate one mbuf, we're really low, so don't even bother calling m_getm(). The other option would be to have m_getm() not require us to pre-allocate an mbuf at all and do all the work, but then that may interfere with code like yours which needs to pass in an existing mbuf that has already been allocated. */ m_free(m); /* fail right here */ } else { size = count * (MLEN + MCLBYTES); if (m_getm(m, size) == NULL) { /* everything has been properly freed for us, we don't have to worry about leaking mbufs. */ /* fail right here. */ } } 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 return NULL; } } I think what may happen here is that you will leak your `top' chain if you fail to allocate a cluster. Assuming that the leak does exist and that it is fixed, we have a pretty good mechanism for doing 'all or nothing' allocations. :-) Later, Bosko. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message