From owner-freebsd-net@FreeBSD.ORG Fri Jan 19 15:17:50 2007 Return-Path: X-Original-To: freebsd-net@freebsd.org Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 69EEF16A400 for ; Fri, 19 Jan 2007 15:17:50 +0000 (UTC) (envelope-from rizzo@icir.org) Received: from xorpc.icir.org (xorpc.icir.org [192.150.187.68]) by mx1.freebsd.org (Postfix) with ESMTP id 5683413C480 for ; Fri, 19 Jan 2007 15:17:50 +0000 (UTC) (envelope-from rizzo@icir.org) Received: from xorpc.icir.org (localhost [127.0.0.1]) by xorpc.icir.org (8.12.11/8.13.6) with ESMTP id l0JEhxFd054369; Fri, 19 Jan 2007 06:43:59 -0800 (PST) (envelope-from rizzo@xorpc.icir.org) Received: (from rizzo@localhost) by xorpc.icir.org (8.12.11/8.12.3/Submit) id l0JEhxU2054368; Fri, 19 Jan 2007 06:43:59 -0800 (PST) (envelope-from rizzo) Date: Fri, 19 Jan 2007 06:43:59 -0800 From: Luigi Rizzo To: Randall Stewart Message-ID: <20070119064359.A54272@xorpc.icir.org> References: <45B0D2E3.9050203@cisco.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5.1i In-Reply-To: <45B0D2E3.9050203@cisco.com>; from rrs@cisco.com on Fri, Jan 19, 2007 at 09:17:07AM -0500 Cc: freebsd-net Subject: Re: kern_mbuf.c patch X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Jan 2007 15:17:50 -0000 On Fri, Jan 19, 2007 at 09:17:07AM -0500, Randall Stewart wrote: > All: > > George and I have been discussing making the attached change > to limit kernel large cluster growth. > > Note I also have added a fix so that if the limit > is 0 (unlimited), then you cannot change the limit > (back downward)... This is in response to Li Xin's comments > about the code I posted earlier.. > > I have no idea if the "default numbers" for these are > correct... it was a wild swag guess. > > Maybe someone might have a better idea of what > the default limits for 4k/9k and 16k clusters should > be :) > > Comments would be appreciated :-) not specific to this patch, but i see that there is no bound check on the sysctl values, and probably there should be. Given that probably there are many other places in the kernel that have similar bound-checking requirements, if would be good if you write your 3 sysctl handlers using a helper function like this err = sysctl_int_checked(oidp, &val, min, max) that returns EINVAL on error (new value < min or > max) leaving val untouched, and 0 on success updating the value. This way the error checking becomes simpler here and for all the lazy programmer who did not bother to implement that in the existing code. cheers luigi > R > -- > Randall Stewart > NSSTG - Cisco Systems Inc. > 803-345-0369 803-317-4952 (cell) > Index: kern_mbuf.c > =================================================================== > RCS file: /usr/FreeBSD/src/sys/kern/kern_mbuf.c,v > retrieving revision 1.27 > diff -u -r1.27 kern_mbuf.c > --- kern_mbuf.c 22 Oct 2006 11:52:13 -0000 1.27 > +++ kern_mbuf.c 19 Jan 2007 14:06:35 -0000 > @@ -107,6 +107,9 @@ > > /* This has to be done before VM init. */ > nmbclusters = 1024 + maxusers * 64; > + nmbjumbop = 100 + (maxusers * 4); > + nmbjumbo9 = 100 + (maxusers * 2); > + nmbjumbo16 = 100 + (maxusers * 2); > TUNABLE_INT_FETCH("kern.ipc.nmbclusters", &nmbclusters); > } > SYSINIT(tunable_mbinit, SI_SUB_TUNABLES, SI_ORDER_ANY, tunable_mbinit, NULL); > @@ -120,7 +123,7 @@ > newnmbclusters = nmbclusters; > error = sysctl_handle_int(oidp, &newnmbclusters, sizeof(int), req); > if (error == 0 && req->newptr) { > - if (newnmbclusters > nmbclusters) { > + if (nmbclusters && (newnmbclusters > nmbclusters)) { > nmbclusters = newnmbclusters; > uma_zone_set_max(zone_clust, nmbclusters); > EVENTHANDLER_INVOKE(nmbclusters_change); > @@ -129,15 +132,75 @@ > } > return (error); > } > + > +static int > +sysctl_nmbjclusters(SYSCTL_HANDLER_ARGS) > +{ > + int error, newnmbjclusters; > + > + newnmbjclusters = nmbjumbop; > + error = sysctl_handle_int(oidp, &newnmbjclusters, sizeof(int), req); > + if (error == 0 && req->newptr) { > + if (nmbjumbop && (newnmbjclusters > nmbjumbop)) { > + nmbjumbop = newnmbjclusters; > + uma_zone_set_max(zone_jumbop, nmbjumbop); > + } else > + error = EINVAL; > + } > + return (error); > +} > + > + > +static int > +sysctl_nmbj9clusters(SYSCTL_HANDLER_ARGS) > +{ > + int error, newnmbj9clusters; > + > + newnmbj9clusters = nmbjumbo9; > + error = sysctl_handle_int(oidp, &newnmbj9clusters, sizeof(int), req); > + if (error == 0 && req->newptr) { > + if (nmbjumbo9 && (newnmbj9clusters > nmbjumbo9)) { > + nmbjumbo9 = newnmbj9clusters; > + uma_zone_set_max(zone_jumbo9, nmbjumbo9); > + } else > + error = EINVAL; > + } > + return (error); > +} > + > +static int > +sysctl_nmbj16clusters(SYSCTL_HANDLER_ARGS) > +{ > + int error, newnmbj16clusters; > + > + newnmbj16clusters = nmbjumbo16; > + error = sysctl_handle_int(oidp, &newnmbj16clusters, sizeof(int), req); > + if (error == 0 && req->newptr) { > + if (nmbjumbo16 && (newnmbj16clusters > nmbjumbo16)) { > + nmbjumbo16 = newnmbj16clusters; > + uma_zone_set_max(zone_jumbo16, nmbjumbo16); > + } else > + error = EINVAL; > + } > + return (error); > +} > + > SYSCTL_PROC(_kern_ipc, OID_AUTO, nmbclusters, CTLTYPE_INT|CTLFLAG_RW, > &nmbclusters, 0, sysctl_nmbclusters, "IU", > "Maximum number of mbuf clusters allowed"); > -SYSCTL_INT(_kern_ipc, OID_AUTO, nmbjumbop, CTLFLAG_RW, &nmbjumbop, 0, > + > +SYSCTL_PROC(_kern_ipc, OID_AUTO, nmbjumbop, CTLTYPE_INT|CTLFLAG_RW, > +&nmbjumbop, 0, sysctl_nmbjclusters, "IU", > "Maximum number of mbuf page size jumbo clusters allowed"); > -SYSCTL_INT(_kern_ipc, OID_AUTO, nmbjumbo9, CTLFLAG_RW, &nmbjumbo9, 0, > + > +SYSCTL_PROC(_kern_ipc, OID_AUTO, nmbjumbo9, CTLTYPE_INT|CTLFLAG_RW, > +&nmbjumbo9, 0, sysctl_nmbj9clusters, "IU", > "Maximum number of mbuf 9k jumbo clusters allowed"); > -SYSCTL_INT(_kern_ipc, OID_AUTO, nmbjumbo16, CTLFLAG_RW, &nmbjumbo16, 0, > + > +SYSCTL_PROC(_kern_ipc, OID_AUTO, nmbjumbo16, CTLTYPE_INT|CTLFLAG_RW, > +&nmbjumbo16, 0, sysctl_nmbj16clusters, "IU", > "Maximum number of mbuf 16k jumbo clusters allowed"); > + > SYSCTL_STRUCT(_kern_ipc, OID_AUTO, mbstat, CTLFLAG_RD, &mbstat, mbstat, > "Mbuf general information and statistics"); > > _______________________________________________ > freebsd-net@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"