Date: Fri, 19 Jan 2007 06:43:59 -0800 From: Luigi Rizzo <rizzo@icir.org> To: Randall Stewart <rrs@cisco.com> Cc: freebsd-net <freebsd-net@freebsd.org> Subject: Re: kern_mbuf.c patch Message-ID: <20070119064359.A54272@xorpc.icir.org> In-Reply-To: <45B0D2E3.9050203@cisco.com>; from rrs@cisco.com on Fri, Jan 19, 2007 at 09:17:07AM -0500 References: <45B0D2E3.9050203@cisco.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <or> 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"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070119064359.A54272>