Re: question about mbuf intialization

[Beverly Schwartz <bschwart%bbn.com@localhost>]

On Sep 26, 2013, at 10:41 AM, christos%astron.com@localhost (Christos Zoulas) 
wrote:

> In article <20024705-1886-4E74-A427-2F20D1479CD1%bbn.com@localhost>,
> Beverly Schwartz  <bschwart%bbn.com@localhost> wrote:
> 
>> Finally, using rump as a way to test kernel code in user space is really
>> powerful.  I did a lot of exercising of mbuf code, and initially, I did
>> a lot of crashing and burning because I didn't realize that m_len wasn't
>> initialized.  This became a pain to deal with - everytime I got an mbuf
>> I had to remember to initialize m_len and m_pkthdr.len.  So I just went
>> and changed the source code, and this discussion has resulted.
> 
> I've been looking at the code and it seems to me that not only m_get()
> needs to be changed, but also m_getcl() and MCLGET().

m_getcl (which calls MCLGET) is called with an mbuf in hand.  These
just add the external buffer and does all apropriate initializations.
They do not add data, so m_len and m_pkthdr.len do not need to be
updated.

> In essence, none
> of the allocators currently set m_len or m_pkthdr.len, and they leave
> it to the packet formation routines to do so.

Correct, and the two functions that need to change are m_get and m_gethdr.

> I am not against setting
> the length to 0 by default on construction to make things more robust

Thank-you.

> (I don't think it makes any performance difference these days);

Agreed.  Nobody objected to the previous change that set m_type to
MT_FREE for robustness sake.  This is almost the same performance
hit as that. For non-pkthdr mbufs, it is the same - one field set
per mbuf.  For those mbufs which have a pkthdr, then it's two
fields set per mbuf.

> I am
> just mentioning that if we do that we should fix it consistently (if
> anyone can call the mbuf API consistent...)

I think I got it right, within uipc_mbuf.c itself.  I did not
go through the rest of the code base to remove every other
initialization to zero.

Here is my diff:

Author: Bev Schwartz <bschwart%bbn.com@localhost>
Date:   Mon Sep 16 18:41:48 2013 -0400

    initialize mbuf lens
    
    Whereas all other mbuf hdr fields are initialized in m_get,
    m_len is not.  Iniitialize to 0.
    
    Likewise, all pkthdr fields are initialized in m_gethdr,
    but not len.  Initialize to 0.
    
    Remove superfluous m_len = 0 statements now that mbufs are
    initialized

diff --git a/netbsd/src/sys/kern/uipc_mbuf.c b/netbsd/src/sys/kern/uipc_mbuf.c
index 542347a..57ebef1 100644
--- a/netbsd/src/sys/kern/uipc_mbuf.c
+++ b/netbsd/src/sys/kern/uipc_mbuf.c
@@ -505,6 +505,7 @@ m_get(int nowait, int type)
        mowner_init(m, type);
        m->m_ext_ref = m;
        m->m_type = type;
+       m->m_len = 0;
        m->m_next = NULL;
        m->m_nextpkt = NULL;
        m->m_data = m->m_dat;
@@ -525,6 +526,7 @@ m_gethdr(int nowait, int type)
        m->m_data = m->m_pktdat;
        m->m_flags = M_PKTHDR;
        m->m_pkthdr.rcvif = NULL;
+       m->m_pkthdr.len = 0;
        m->m_pkthdr.csum_flags = 0;
        m->m_pkthdr.csum_data = 0;
        SLIST_INIT(&m->m_pkthdr.tags);
@@ -692,7 +694,6 @@ m_copym0(struct mbuf *m, int off0, int len, int wait, int 
deep)
                                 * copy into multiple MCLBYTES cluster mbufs.
                                 */
                                MCLGET(n, wait);
-                               n->m_len = 0;
                                n->m_len = M_TRAILINGSPACE(n);
                                n->m_len = min(n->m_len, len);
                                n->m_len = min(n->m_len, m->m_len - off);
@@ -942,7 +943,6 @@ m_ensure_contig(struct mbuf **m0, int len)
                        return false;
                }
                MCLAIM(m, n->m_owner);
-               m->m_len = 0;
                if (n->m_flags & M_PKTHDR) {
                        M_MOVE_PKTHDR(m, n);
                }
@@ -1006,7 +1006,6 @@ m_copyup(struct mbuf *n, int len, int dstoff)
        if (m == NULL)
                goto bad;
        MCLAIM(m, n->m_owner);
-       m->m_len = 0;
        if (n->m_flags & M_PKTHDR) {
                M_MOVE_PKTHDR(m, n);
        }
@@ -1387,7 +1386,6 @@ extend:
                        if (n == NULL) {
                                goto out;
                        }
-                       n->m_len = 0;
                        n->m_len = min(M_TRAILINGSPACE(n), off + len);
                        memset(mtod(n, char *), 0, min(n->m_len, off));
                        m->m_next = n;