NetBSD Problem Report #3480

Received: (qmail-queue invoked from smtpd); 12 Apr 1997 09:10:59 -0000
Message-Id: <199704120909.SAA00583@ducati.imada.math.human.nagoya-u.ac.jp>
Date: Sat, 12 Apr 1997 18:09:21 +0900 (JST)
From: koji@math.human.nagoya-u.ac.jp
Reply-To: koji@math.human.nagoya-u.ac.jp
To: gnats-bugs@gnats.netbsd.org
Subject: Deletion of dtom in ip_input.c
X-Send-Pr-Version: 3.95

>Number:         3480
>Category:       kern
>Synopsis:       changes to eliminate dtom in ip reassembly code in ip_input.c
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Sat Apr 12 02:20:01 +0000 1997
>Closed-Date:    Tue Jun 24 01:31:55 +0000 1997
>Last-Modified:  Tue Jun 24 01:33:03 +0000 1997
>Originator:     Koji Imada - je4owb/2
>Release:        970409
>Organization:
Mathematics Group of Graduate School of Human
	Infomatics, Nagoya University, Japan. 
>Environment:

System: NetBSD ducati 1.2D NetBSD 1.2D (DUCATI) #1: Sat Apr 12 11:35:25 JST 1997 koji@ducati:/mnt2/NetBSD/work/src-ipv6/sys/arch/i386/compile/DUCATI i386


>Description:
	changes to eliminate dtom in ip reassembly code in
	ip_input.c. struct ipqent is modified to always keep pointer
	to mbuf. And struct ipq is allocated with MALLOC not
	m_get. These modification eliminates dtom in ip_input.c.
>How-To-Repeat:

>Fix:
	Apply following patches.

Index: sys/netinet//ip_input.c
===================================================================
RCS file: /mnt2/NetBSD/cvsroot/netbsd/sys/netinet/ip_input.c,v
retrieving revision 1.1.1.2
diff -c -r1.1.1.2 ip_input.c
*** ip_input.c	1997/04/01 07:17:24	1.1.1.2
--- ip_input.c	1997/04/12 01:31:07
***************
*** 373,378 ****
--- 373,379 ----
  	 * but it's not worth the time; just let them time out.)
  	 */
  	if (ip->ip_off & ~(IP_DF|IP_RF)) {
+ #if 0 /* this is not necessary if dtom is not used */
  		if (m->m_flags & M_EXT) {		/* XXX */
  			if ((m = m_pullup(m, sizeof (struct ip))) == 0) {
  				ipstat.ips_toosmall++;
***************
*** 380,385 ****
--- 381,387 ----
  			}
  			ip = mtod(m, struct ip *);
  		}
+ #endif
  		/*
  		 * Look for queue of fragments
  		 * of this datagram.
***************
*** 425,430 ****
--- 427,442 ----
  				ipstat.ips_rcvmemdrop++;
  				goto bad;
  			}
+ #if 1 /* don't use dtom */
+ 			ipqe->ipqe_mff = mff;
+ 			ipqe->ipqe_m = m;
+ 			ipqe->ipqe_ip = ip;
+ 			m = ip_reass(ipqe, fp);
+ 			if (m == 0)
+ 				goto next;
+ 			ipstat.ips_reassembled++;
+ 			ip = mtod(m, struct ip *);
+ #else 
  			ipqe->ipqe_mff = mff;
  			ipqe->ipqe_ip = ip;
  			ip = ip_reass(ipqe, fp);
***************
*** 432,437 ****
--- 444,450 ----
  				goto next;
  			ipstat.ips_reassembled++;
  			m = dtom(ip);
+ #endif
  		} else
  			if (fp)
  				ip_freef(fp);
***************
*** 455,466 ****
   * reassembly of this datagram already exists, then it
   * is given as fp; otherwise have to make a chain.
   */
! struct ip *
  ip_reass(ipqe, fp)
  	register struct ipqent *ipqe;
  	register struct ipq *fp;
  {
! 	register struct mbuf *m = dtom(ipqe->ipqe_ip);
  	register struct ipqent *nq, *p, *q;
  	struct ip *ip;
  	struct mbuf *t;
--- 468,479 ----
   * reassembly of this datagram already exists, then it
   * is given as fp; otherwise have to make a chain.
   */
! struct mbuf *
  ip_reass(ipqe, fp)
  	register struct ipqent *ipqe;
  	register struct ipq *fp;
  {
! 	register struct mbuf *m = ipqe->ipqe_m;
  	register struct ipqent *nq, *p, *q;
  	struct ip *ip;
  	struct mbuf *t;
***************
*** 478,486 ****
--- 491,506 ----
  	 * If first fragment to arrive, create a reassembly queue.
  	 */
  	if (fp == 0) {
+ #if 1
+ 		MALLOC(fp, struct ipq *, sizeof (struct ipq),
+ 		    M_FTABLE, M_NOWAIT);
+ 		if (fp == NULL)
+ 			goto dropfrag;
+ #else
  		if ((t = m_get(M_DONTWAIT, MT_FTABLE)) == NULL)
  			goto dropfrag;
  		fp = mtod(t, struct ipq *);
+ #endif
  		LIST_INSERT_HEAD(&ipq, fp, ipq_q);
  		fp->ipq_ttl = IPFRAGTTL;
  		fp->ipq_p = ipqe->ipqe_ip->ip_p;
***************
*** 511,517 ****
  		if (i > 0) {
  			if (i >= ipqe->ipqe_ip->ip_len)
  				goto dropfrag;
! 			m_adj(dtom(ipqe->ipqe_ip), i);
  			ipqe->ipqe_ip->ip_off += i;
  			ipqe->ipqe_ip->ip_len -= i;
  		}
--- 531,537 ----
  		if (i > 0) {
  			if (i >= ipqe->ipqe_ip->ip_len)
  				goto dropfrag;
! 			m_adj(ipqe->ipqe_m, i);
  			ipqe->ipqe_ip->ip_off += i;
  			ipqe->ipqe_ip->ip_len -= i;
  		}
***************
*** 528,538 ****
  		if (i < q->ipqe_ip->ip_len) {
  			q->ipqe_ip->ip_len -= i;
  			q->ipqe_ip->ip_off += i;
! 			m_adj(dtom(q->ipqe_ip), i);
  			break;
  		}
  		nq = q->ipqe_q.le_next;
! 		m_freem(dtom(q->ipqe_ip));
  		LIST_REMOVE(q, ipqe_q);
  		FREE(q, M_IPQ);
  	}
--- 548,558 ----
  		if (i < q->ipqe_ip->ip_len) {
  			q->ipqe_ip->ip_len -= i;
  			q->ipqe_ip->ip_off += i;
! 			m_adj(q->ipqe_m, i);
  			break;
  		}
  		nq = q->ipqe_q.le_next;
! 		m_freem(q->ipqe_m);
  		LIST_REMOVE(q, ipqe_q);
  		FREE(q, M_IPQ);
  	}
***************
*** 568,581 ****
  		ip_freef(fp);
  		return (0);
  	}
! 	m = dtom(q->ipqe_ip);
  	t = m->m_next;
  	m->m_next = 0;
  	m_cat(m, t);
  	nq = q->ipqe_q.le_next;
  	FREE(q, M_IPQ);
  	for (q = nq; q != NULL; q = nq) {
! 		t = dtom(q->ipqe_ip);
  		nq = q->ipqe_q.le_next;
  		FREE(q, M_IPQ);
  		m_cat(m, t);
--- 588,601 ----
  		ip_freef(fp);
  		return (0);
  	}
! 	m = q->ipqe_m;
  	t = m->m_next;
  	m->m_next = 0;
  	m_cat(m, t);
  	nq = q->ipqe_q.le_next;
  	FREE(q, M_IPQ);
  	for (q = nq; q != NULL; q = nq) {
! 		t = q->ipqe_m;
  		nq = q->ipqe_q.le_next;
  		FREE(q, M_IPQ);
  		m_cat(m, t);
***************
*** 591,607 ****
  	ip->ip_src = fp->ipq_src;
  	ip->ip_dst = fp->ipq_dst;
  	LIST_REMOVE(fp, ipq_q);
  	(void) m_free(dtom(fp));
  	m->m_len += (ip->ip_hl << 2);
  	m->m_data -= (ip->ip_hl << 2);
  	/* some debugging cruft by sklower, below, will go away soon */
  	if (m->m_flags & M_PKTHDR) { /* XXX this should be done elsewhere */
  		register int plen = 0;
! 		for (t = m; m; m = m->m_next)
! 			plen += m->m_len;
! 		t->m_pkthdr.len = plen;
  	}
! 	return (ip);

  dropfrag:
  	ipstat.ips_fragdropped++;
--- 611,631 ----
  	ip->ip_src = fp->ipq_src;
  	ip->ip_dst = fp->ipq_dst;
  	LIST_REMOVE(fp, ipq_q);
+ #if 1
+ 	FREE(fp, M_FTABLE);
+ #else
  	(void) m_free(dtom(fp));
+ #endif
  	m->m_len += (ip->ip_hl << 2);
  	m->m_data -= (ip->ip_hl << 2);
  	/* some debugging cruft by sklower, below, will go away soon */
  	if (m->m_flags & M_PKTHDR) { /* XXX this should be done elsewhere */
  		register int plen = 0;
! 		for (t = m; t; t = t->m_next)
! 			plen += t->m_len;
! 		m->m_pkthdr.len = plen;
  	}
! 	return (m);

  dropfrag:
  	ipstat.ips_fragdropped++;
***************
*** 622,633 ****

  	for (q = fp->ipq_fragq.lh_first; q != NULL; q = p) {
  		p = q->ipqe_q.le_next;
! 		m_freem(dtom(q->ipqe_ip));
  		LIST_REMOVE(q, ipqe_q);
  		FREE(q, M_IPQ);
  	}
  	LIST_REMOVE(fp, ipq_q);
  	(void) m_free(dtom(fp));
  }

  /*
--- 646,661 ----

  	for (q = fp->ipq_fragq.lh_first; q != NULL; q = p) {
  		p = q->ipqe_q.le_next;
! 		m_freem(q->ipqe_m);
  		LIST_REMOVE(q, ipqe_q);
  		FREE(q, M_IPQ);
  	}
  	LIST_REMOVE(fp, ipq_q);
+ #if 1
+ 	FREE(fp, M_FTABLE);
+ #else
  	(void) m_free(dtom(fp));
+ #endif
  }

  /*
Index: sys/netinet//ip_var.h
===================================================================
RCS file: /mnt2/NetBSD/cvsroot/netbsd/sys/netinet/ip_var.h,v
retrieving revision 1.1.1.2
diff -c -r1.1.1.2 ip_var.h
*** ip_var.h	1997/04/01 07:17:27	1.1.1.2
--- ip_var.h	1997/04/12 01:14:17
***************
*** 66,80 ****
  		struct ip	*_ip;
  		struct tcpiphdr *_tcp;
  	} _ipqe_u1;
! 	union {
! 		u_int8_t	_mff;	/* for IP fragmentation */
! 		struct mbuf	*_m;	/* XXX for TCP; see above */
! 	} _ipqe_u2;
  };
  #define	ipqe_ip		_ipqe_u1._ip
  #define	ipqe_tcp	_ipqe_u1._tcp
- #define	ipqe_mff	_ipqe_u2._mff
- #define	ipqe_m		_ipqe_u2._m

  /*
   * Ip reassembly queue structure.  Each fragment
--- 66,76 ----
  		struct ip	*_ip;
  		struct tcpiphdr *_tcp;
  	} _ipqe_u1;
! 	struct mbuf	*ipqe_m;	/* mbuf contains packet */
! 	u_int8_t	ipqe_mff;	/* for IP fragmentation */
  };
  #define	ipqe_ip		_ipqe_u1._ip
  #define	ipqe_tcp	_ipqe_u1._tcp

  /*
   * Ip reassembly queue structure.  Each fragment
***************
*** 172,178 ****
  int	 ip_optcopy __P((struct ip *, struct ip *));
  int	 ip_output __P((struct mbuf *, ...));
  int	 ip_pcbopts __P((struct mbuf **, struct mbuf *));
! struct ip *
  	 ip_reass __P((struct ipqent *, struct ipq *));
  struct in_ifaddr *
  	 ip_rtaddr __P((struct in_addr));
--- 168,174 ----
  int	 ip_optcopy __P((struct ip *, struct ip *));
  int	 ip_output __P((struct mbuf *, ...));
  int	 ip_pcbopts __P((struct mbuf **, struct mbuf *));
! struct mbuf *
  	 ip_reass __P((struct ipqent *, struct ipq *));
  struct in_ifaddr *
  	 ip_rtaddr __P((struct in_addr));

>Release-Note:
>Audit-Trail:
State-Changed-From-To: open->feedback 
State-Changed-By: pk 
State-Changed-When: Wed Apr 16 15:46:13 PDT 1997 
State-Changed-Why:  
Can you explain why eliminating dtom() in this particular case is a win? 
Does it have a major performance benefit? 
State-Changed-From-To: feedback->analyzed 
State-Changed-By: thorpej 
State-Changed-When: Mon Jun 23 18:30:32 PDT 1997 
State-Changed-Why:  
This change is a correct one, and should be implemented. 
State-Changed-From-To: analyzed->closed 
State-Changed-By: thorpej 
State-Changed-When: Mon Jun 23 18:31:55 PDT 1997 
State-Changed-Why:  
This is a subset of PR 3628 
>Unformatted:
 Reply from Koji Imada <koji@bimota.imada.math.human.nagoya-u.ac.jp>:

 My goal is to eliminate all dtom()'s from kernel code. Since dtom()
 restricts improvement of mbuf routines for more utilization.

 Now, dtom() is forcing to use m_pullup for mbuf clusters on
 everywhere(described in p.374 of "The Design and Implementation of the 
 4.4BSD Operating System"). I think it is useless.

 This maybe some performance penalty(now, device drivers use mbuf
 clusters for many packets?). Special handling of mbuf clusters(M_EXT)
 is not preferable.

 I have found dtom() in ip_input.c and tcp_subr.c. The dtom() in
 tcp_subr.c may be eliminated by using malloc'ed memory instead of mbuf 
 for tcp_template(). I would send-pr patch for that too.

 I want to eliminate dtom()'s in all kernel codes and improve mbuf
 routines because current mbuf routines is not enough to be used for
 IPv6 code and ipsec codes.

 There are ugly codes to avoid limitation of current mbuf routines in
 inria's IPv6 code and nrl's Ipv6 code.

Home	PR Database Search