From owner-freebsd-net Tue Jun 19 14:34:12 2001 Delivered-To: freebsd-net@freebsd.org Received: from technokratis.com (modemcable052.174-202-24.mtl.mc.videotron.ca [24.202.174.52]) by hub.freebsd.org (Postfix) with ESMTP id E7F8037B401 for ; Tue, 19 Jun 2001 14:33:57 -0700 (PDT) (envelope-from bmilekic@technokratis.com) Received: (from bmilekic@localhost) by technokratis.com (8.11.3/8.11.3) id f5JLYPZ08516; Tue, 19 Jun 2001 17:34:25 -0400 (EDT) (envelope-from bmilekic) Date: Tue, 19 Jun 2001 17:34:24 -0400 From: Bosko Milekic To: Mike Silbersack Cc: freebsd-net@FreeBSD.ORG Subject: Re: tcp template removal / scalability patch Message-ID: <20010619173424.A8399@technokratis.com> References: <20010619121337.C77484-300000@achilles.silby.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20010619121337.C77484-300000@achilles.silby.com>; from silby@silby.com on Tue, Jun 19, 2001 at 12:19:19PM -0500 Sender: owner-freebsd-net@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org On Tue, Jun 19, 2001 at 12:19:19PM -0500, Mike Silbersack wrote: > As suggested by Terry, I've cooked up a patch which halts the use of mbufs > for storing tcp template structures. The structure was only used in two > places; tcp_output.c when sending packets, and tcp_timer.c when sending > keepalives. tcp_output now pulls the info directly from the tcpcb, while > tcp_timer creates a short-term tcp template that is destroyed after use. > > The end result is that rather than 1 mbuf being the minimum used per > connection, 0 mbufs is now the minimum. As a result, those with boxes > handling a lot of connections should see greatly reduced mbuf usage. Cool! > I've attached two patches; one for current, and one for stable. Please > review / test, _especially_ if you're using IPv6 or IPSec - while those > cases look correct, I'm not running either and haven't tested them. I've spotted some patch-related (not conceptual) things worth mentionning, so I'll do that now and give you the conceptual review a little later, hopefully before I leave. > Thanks, > > Mike "Silby" Silbersack > Only in netinet.old/: icmp_var.h.orig > Only in netinet.old/: ip_icmp.c.orig Dirty. :-) > diff -u -r netinet.old/tcp_input.c netinet/tcp_input.c > --- netinet.old/tcp_input.c Tue Jun 19 11:53:16 2001 > +++ netinet/tcp_input.c Tue Jun 19 11:53:25 2001 > @@ -1066,12 +1066,7 @@ > } > FREE(sin, M_SONAME); > } > - tp->t_template = tcp_template(tp); > - if (tp->t_template == 0) { > - tp = tcp_drop(tp, ENOBUFS); > - dropsocket = 0; /* socket is already gone */ > - goto drop; > - } > + tp->t_template = NULL; > if ((taop = tcp_gettaocache(inp)) == NULL) { > taop = &tao_noncached; > bzero(taop, sizeof(*taop)); > Only in netinet.old/: tcp_input.c.orig > Only in netinet.old/: tcp_input.c.rej > diff -u -r netinet.old/tcp_output.c netinet/tcp_output.c > --- netinet.old/tcp_output.c Tue Jun 19 11:53:16 2001 > +++ netinet/tcp_output.c Tue Jun 19 11:53:25 2001 > @@ -630,16 +630,12 @@ > m->m_len = hdrlen; > } > m->m_pkthdr.rcvif = (struct ifnet *)0; > - if (tp->t_template == 0) > - panic("tcp_output"); > + > #ifdef INET6 > if (isipv6) { > ip6 = mtod(m, struct ip6_hdr *); > th = (struct tcphdr *)(ip6 + 1); > - bcopy((caddr_t)tp->t_template->tt_ipgen, (caddr_t)ip6, > - sizeof(struct ip6_hdr)); > - bcopy((caddr_t)&tp->t_template->tt_t, (caddr_t)th, > - sizeof(struct tcphdr)); > + tcp_fillheaders(tp, ip6, th); > } else > #endif /* INET6 */ > { > @@ -647,10 +643,7 @@ > ipov = (struct ipovly *)ip; > th = (struct tcphdr *)(ip + 1); > /* this picks up the pseudo header (w/o the length) */ > - bcopy((caddr_t)tp->t_template->tt_ipgen, (caddr_t)ip, > - sizeof(struct ip)); > - bcopy((caddr_t)&tp->t_template->tt_t, (caddr_t)th, > - sizeof(struct tcphdr)); > + tcp_fillheaders(tp, ip, th); > } > > /* > Only in netinet.old/: tcp_output.c.orig > Only in netinet.old/: tcp_seq.h.orig > diff -u -r netinet.old/tcp_subr.c netinet/tcp_subr.c > --- netinet.old/tcp_subr.c Tue Jun 19 11:53:16 2001 > +++ netinet/tcp_subr.c Tue Jun 19 11:57:56 2001 > @@ -220,32 +220,27 @@ > #undef TCP_MINPROTOHDR > } > > + Unnecessary newline. > /* > - * Create template to be used to send tcp packets on a connection. > - * Call after host entry created, allocates an mbuf and fills > - * in a skeletal tcp/ip header, minimizing the amount of work > - * necessary when the connection is used. > + * Fill in the IP and TCP headers for an outgoing packet, given the tcpcb. > + * tcp_template used to store this data in mbufs, but we now recopy it out > + * of the tcpcb each time to conserve mbufs. > */ > -struct tcptemp * > -tcp_template(tp) > + Unnecessary newline. > +void > +tcp_fillheaders(tp, ip_ptr, tcp_ptr) > struct tcpcb *tp; > + void *ip_ptr; > + void *tcp_ptr; > { > - register struct inpcb *inp = tp->t_inpcb; > - register struct mbuf *m; > - register struct tcptemp *n; > + struct inpcb *inp = tp->t_inpcb; > + struct tcphdr *tcp_hdr = (struct tcphdr *)tcp_ptr; > > - if ((n = tp->t_template) == 0) { > - m = m_get(M_DONTWAIT, MT_HEADER); > - if (m == NULL) > - return (0); > - m->m_len = sizeof (struct tcptemp); > - n = mtod(m, struct tcptemp *); > - } > #ifdef INET6 > if ((inp->inp_vflag & INP_IPV6) != 0) { > - register struct ip6_hdr *ip6; > + struct ip6_hdr *ip6; Cool. > - ip6 = (struct ip6_hdr *)n->tt_ipgen; > + ip6 = (struct ip6_hdr *)ip_ptr; > ip6->ip6_flow = (ip6->ip6_flow & ~IPV6_FLOWINFO_MASK) | > (inp->in6p_flowinfo & IPV6_FLOWINFO_MASK); > ip6->ip6_vfc = (ip6->ip6_vfc & ~IPV6_VERSION_MASK) | > @@ -254,29 +249,52 @@ > ip6->ip6_plen = sizeof(struct tcphdr); > ip6->ip6_src = inp->in6p_laddr; > ip6->ip6_dst = inp->in6p_faddr; > - n->tt_t.th_sum = 0; > + tcp_hdr->th_sum = 0; > } else > #endif > - { > - struct ip *ip = (struct ip *)n->tt_ipgen; > + { > + struct ip *ip = (struct ip *) ip_ptr; Something seems stylistically wrong here. I think the "{" brace should be on the same line as the "} else" above. > - bzero(ip, sizeof(struct ip)); /* XXX overkill? */ > + bzero(ip, sizeof(struct ip)); /* XXX overkill? */ Unnecessary whitespace. You changed the tabs separating the comment from the statement to spaces. > ip->ip_vhl = IP_VHL_BORING; > ip->ip_p = IPPROTO_TCP; > ip->ip_src = inp->inp_laddr; > ip->ip_dst = inp->inp_faddr; > - n->tt_t.th_sum = in_pseudo(ip->ip_src.s_addr, ip->ip_dst.s_addr, > - htons(sizeof(struct tcphdr) + IPPROTO_TCP)); > - } > - n->tt_t.th_sport = inp->inp_lport; > - n->tt_t.th_dport = inp->inp_fport; > - n->tt_t.th_seq = 0; > - n->tt_t.th_ack = 0; > - n->tt_t.th_x2 = 0; > - n->tt_t.th_off = 5; > - n->tt_t.th_flags = 0; > - n->tt_t.th_win = 0; > - n->tt_t.th_urp = 0; > + tcp_hdr->th_sum = in_pseudo(ip->ip_src.s_addr, ip->ip_dst.s_addr, > + htons(sizeof(struct tcphdr) + IPPROTO_TCP)); > + } > + > + tcp_hdr->th_sport = inp->inp_lport; > + tcp_hdr->th_dport = inp->inp_fport; > + tcp_hdr->th_seq = 0; > + tcp_hdr->th_ack = 0; > + tcp_hdr->th_x2 = 0; > + tcp_hdr->th_off = 5; > + tcp_hdr->th_flags = 0; > + tcp_hdr->th_win = 0; > + tcp_hdr->th_urp = 0; > +} > + > + I think one line too many. I could be wrong. > +/* > + * Create template to be used to send tcp packets on a connection. > + * Allocates an mbuf and fills in a skeletal tcp/ip header. The only > + * use for this function is in keepalives, who like to use tcp_respond. > + */ > +struct tcptemp * > +tcp_maketemplate(tp) > + struct tcpcb *tp; > +{ > + struct mbuf *m; > + struct tcptemp *n; > + > + m = m_get(M_DONTWAIT, MT_HEADER); > + if (m == NULL) > + return (0); > + m->m_len = sizeof (struct tcptemp); No space after sizeof, unless you get rid of the parantheses (style(9)). > + n = mtod(m, struct tcptemp *); > + > + tcp_fillheaders(tp, (void *)&n->tt_ipgen, (void *)&n->tt_t); > return (n); > } > > @@ -706,7 +724,7 @@ > FREE(q, M_TSEGQ); > } > if (tp->t_template) > - (void) m_free(dtom(tp->t_template)); > + panic("t_template non-null!"); Perhaps this should be an INVARIANTS-enabled KASSERT()? Is this something that can only happen due to programming error? It seems to me like it's the case here (assuming we're not dealing with a huge-assed memory corruption). > inp->inp_ppcb = NULL; > soisdisconnected(so); > #ifdef INET6 > @@ -1347,7 +1365,7 @@ > #endif /* INET6 */ > struct tcphdr *th; > > - if (!tp || !tp->t_template || !(inp = tp->t_inpcb)) > + if (!tp || !(inp = tp->t_inpcb)) Hmmm, this is gross. can we do something like, assuming tp is a pointer: if (tp == NULL || (inp = tp->t_inpcb) == NULL) (especially now that we have enough space in the line to do it?) > return 0; > MGETHDR(m, M_DONTWAIT, MT_DATA); > if (!m) > @@ -1359,10 +1377,7 @@ > th = (struct tcphdr *)(ip6 + 1); > m->m_pkthdr.len = m->m_len = > sizeof(struct ip6_hdr) + sizeof(struct tcphdr); > - bcopy((caddr_t)tp->t_template->tt_ipgen, (caddr_t)ip6, > - sizeof(struct ip6_hdr)); > - bcopy((caddr_t)&tp->t_template->tt_t, (caddr_t)th, > - sizeof(struct tcphdr)); > + tcp_fillheaders(tp, ip6, th); > hdrsiz = ipsec6_hdrsiz(m, IPSEC_DIR_OUTBOUND, inp); > } else > #endif /* INET6 */ > @@ -1370,10 +1385,7 @@ > ip = mtod(m, struct ip *); > th = (struct tcphdr *)(ip + 1); > m->m_pkthdr.len = m->m_len = sizeof(struct tcpiphdr); > - bcopy((caddr_t)tp->t_template->tt_ipgen, (caddr_t)ip, > - sizeof(struct ip)); > - bcopy((caddr_t)&tp->t_template->tt_t, (caddr_t)th, > - sizeof(struct tcphdr)); > + tcp_fillheaders(tp, ip, th); > hdrsiz = ipsec4_hdrsiz(m, IPSEC_DIR_OUTBOUND, inp); > } > > Only in netinet.old/: tcp_subr.c.orig > diff -u -r netinet.old/tcp_timer.c netinet/tcp_timer.c > --- netinet.old/tcp_timer.c Tue Jun 19 11:53:16 2001 > +++ netinet/tcp_timer.c Tue Jun 19 11:56:13 2001 > @@ -41,6 +41,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -227,6 +228,7 @@ > void *xtp; > { > struct tcpcb *tp = xtp; > + struct tcptemp *t_template; > int s; > #ifdef TCPDEBUG > int ostate; > @@ -273,9 +275,14 @@ > &tp->t_template->tt_t, (struct mbuf *)NULL, > tp->rcv_nxt - 1, tp->snd_una - 1, 0); > #else > - tcp_respond(tp, tp->t_template->tt_ipgen, > - &tp->t_template->tt_t, (struct mbuf *)NULL, > - tp->rcv_nxt, tp->snd_una - 1, 0); > + > + t_template = tcp_maketemplate(tp); > + if (t_template) { > + tcp_respond(tp, t_template->tt_ipgen, > + &t_template->tt_t, (struct mbuf *)NULL, > + tp->rcv_nxt, tp->snd_una - 1, 0); > + (void) m_free(dtom(t_template)); > + } > #endif > callout_reset(tp->tt_keep, tcp_keepintvl, tcp_timer_keep, tp); > } else > Only in netinet.old/: tcp_timer.c.orig > diff -u -r netinet.old/tcp_usrreq.c netinet/tcp_usrreq.c > --- netinet.old/tcp_usrreq.c Tue Jun 19 11:53:16 2001 > +++ netinet/tcp_usrreq.c Tue Jun 19 11:53:27 2001 > @@ -744,11 +744,7 @@ > inp->inp_fport = sin->sin_port; > in_pcbrehash(inp); > > - tp->t_template = tcp_template(tp); > - if (tp->t_template == 0) { > - in_pcbdisconnect(inp); > - return ENOBUFS; > - } > + tp->t_template = NULL; > > /* Compute window scaling to request. */ > while (tp->request_r_scale < TCP_MAX_WINSHIFT && > @@ -841,11 +837,7 @@ > inp->in6p_flowinfo = sin6->sin6_flowinfo; > in_pcbrehash(inp); > > - tp->t_template = tcp_template(tp); > - if (tp->t_template == 0) { > - in6_pcbdisconnect(inp); > - return ENOBUFS; > - } > + tp->t_template = NULL; > > /* Compute window scaling to request. */ > while (tp->request_r_scale < TCP_MAX_WINSHIFT && > Only in netinet.old/: tcp_usrreq.c.orig > Only in netinet.old/: tcp_usrreq.c.rej > diff -u -r netinet.old/tcp_var.h netinet/tcp_var.h > --- netinet.old/tcp_var.h Tue Jun 19 11:53:16 2001 > +++ netinet/tcp_var.h Tue Jun 19 11:53:27 2001 > @@ -400,7 +400,8 @@ > void tcp_setpersist __P((struct tcpcb *)); > void tcp_slowtimo __P((void)); > struct tcptemp * > - tcp_template __P((struct tcpcb *)); > + tcp_maketemplate __P((struct tcpcb *)); > +void tcp_fillheaders __P((struct tcpcb *, void *, void *)); > struct tcpcb * > tcp_timers __P((struct tcpcb *, int)); > void tcp_trace __P((int, int, struct tcpcb *, void *, struct tcphdr *, > Only in netinet.old/: tcp_var.h.orig > Only in netinet.old/: udp_usrreq.c.orig [snipped second posting of patch - it was posted twice] Cheers, -- Bosko Milekic bmilekic@technokratis.com To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message