Date: Thu, 8 Feb 2001 14:16:07 -0500 From: "Bosko Milekic" <bmilekic@technokratis.com> To: "Boris Popov" <bp@butya.kz> Cc: <freebsd-arch@FreeBSD.ORG>, <freebsd-net@FreeBSD.ORG> Subject: Re: Sequential mbuf read/write extensions Message-ID: <013201c09203$971be9c0$1f90c918@jehovah> References: <Pine.BSF.4.21.0102071516110.7952-100000@lion.butya.kz>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?013201c09203$971be9c0$1f90c918>