Subject: Re: Add "last record" and "last mbuf" pointers to sockbuf
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
From: Jason R Thorpe <thorpej@wasabisystems.com>
List: tech-kern
Date: 07/03/2002 08:37:04
On Wed, Jul 03, 2002 at 06:30:58PM +0900, YAMAMOTO Takashi wrote:

 > i made a patch. (attached)
 > both tcp and udp seems to work with it.

Wow, great!

I have some questions for you, though...

 > --- bak/uipc_socket.c	Wed Jul  3 13:40:41 2002
 > +++ uipc_socket.c	Wed Jul  3 18:07:00 2002
 > @@ -894,6 +894,8 @@
 >  			error = EWOULDBLOCK;
 >  			goto release;
 >  		}
 > +		SBLASTRECORDCHK(&so->so_rcv, "soreceive sbwait 1");
 > +		SBLASTMBUFCHK(&so->so_rcv, "soreceive sbwait 1");
 >  		sbunlock(&so->so_rcv);
 >  		error = sbwait(&so->so_rcv);
 >  		splx(s);
 > @@ -931,12 +933,10 @@
 >  			if (paddr) {
 >  				*paddr = m;
 >  				so->so_rcv.sb_mb = m->m_next;
 > -				SB_UPDATE_TAIL(&so->so_rcv);
 >  				m->m_next = 0;
 >  				m = so->so_rcv.sb_mb;
 >  			} else {
 >  				MFREE(m, so->so_rcv.sb_mb);
 > -				SB_UPDATE_TAIL(&so->so_rcv);
 >  				m = so->so_rcv.sb_mb;
 >  			}
 >  		}
 > @@ -956,12 +956,10 @@
 >  					error = (*pr->pr_domain->dom_externalize)(m);
 >  				*controlp = m;
 >  				so->so_rcv.sb_mb = m->m_next;
 > -				SB_UPDATE_TAIL(&so->so_rcv);
 >  				m->m_next = 0;
 >  				m = so->so_rcv.sb_mb;
 >  			} else {
 >  				MFREE(m, so->so_rcv.sb_mb);
 > -				SB_UPDATE_TAIL(&so->so_rcv);
 >  				m = so->so_rcv.sb_mb;
 >  			}
 >  		}
 > @@ -984,6 +982,13 @@
 >  		if (type == MT_OOBDATA)
 >  			flags |= MSG_OOB;
 >  	}
 > +	else {
 > +		if ((flags & MSG_PEEK) == 0) {
 > +			KASSERT(so->so_rcv.sb_mb == m);
 > +			so->so_rcv.sb_mb = nextrecord;
 > +			SB_UPDATE_TAIL(&so->so_rcv);
 > +		}
 > +	}

With this change, I think the next block of:

        /*
         * If nextrecord == NULL (this is a single chain), then
         * sb_lastrecord may not be valid here if m was changed
         * earlier.
         */
        if (nextrecord == NULL && (flags & MSG_PEEK) == 0) {
                KASSERT(so->so_rcv.sb_mb == m); 
                so->so_rcv.sb_lastrecord = m;
        }

is partially redundant.  I think it can be removed, and the the previous
block changed to:

        if (m) {        
                if ((flags & MSG_PEEK) == 0) {
                        m->m_nextpkt = nextrecord;
                        /*
                         * If nextrecord == NULL (this is a single chain),
                         * then sb_lastrecord may not be valid here if m
                         * was changed earlier.
                         */
                        if (nextrecord == NULL) {
                                KASSERT(so->so_rcv.sb_mb == m);
                                so->so_rcv.sb_lastrecord = m;
                        }
                }
                type = m->m_type;
                if (type == MT_OOBDATA)
                        flags |= MSG_OOB; 
        } else {
                if ((flags & MSG_PEEK) == 0) {
                        KASSERT(so->so_rcv.sb_mb == m);
                        so->so_rcv.sb_mb = nextrecord; 
                        SB_UPDATE_TAIL(&so->so_rcv);
                }
        }

Do you agree?

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>