From owner-freebsd-current@FreeBSD.ORG Wed Jun 18 21:14:02 2014 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 68F00792 for ; Wed, 18 Jun 2014 21:14:02 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 3FC6C2124 for ; Wed, 18 Jun 2014 21:14:02 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 39E38B945; Wed, 18 Jun 2014 17:14:01 -0400 (EDT) From: John Baldwin To: Hans Petter Selasky Subject: Re: [RFC] Huge sysctl patch for the kernel coming - work in progress Date: Wed, 18 Jun 2014 17:13:06 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20140415; KDE/4.5.5; amd64; ; ) References: <53A179D5.8020604@selasky.org> <201406180944.17762.jhb@freebsd.org> <53A1F848.2020902@selasky.org> In-Reply-To: <53A1F848.2020902@selasky.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201406181713.06579.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Wed, 18 Jun 2014 17:14:01 -0400 (EDT) Cc: freebsd-current@freebsd.org X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Jun 2014 21:14:02 -0000 On Wednesday, June 18, 2014 4:36:24 pm Hans Petter Selasky wrote: > On 06/18/14 15:44, John Baldwin wrote: > > On Wednesday, June 18, 2014 7:36:53 am Hans Petter Selasky wrote: > >> Hi, > >> > >> Sometimes sysctl's default value needs to be setup at boot time and not > >> when the rc.d/sysctl is running. Currently this is done by having two > >> statements in the kernel: > >> > >> TUNABLE_INT("net.graph.mppe.log_max_rekey", &mppe_log_max_rekey); > >> SYSCTL_INT(_net_graph_mppe, OID_AUTO, log_max_rekey, CTLFLAG_RW, > >> > >> I want to simplify this to: > >> > >> SYSCTL_INT(_net_graph_mppe, OID_AUTO, log_max_rekey, CTLFLAG_RWTUN, > >> > >> In other words if the existing CTLFLAG_TUN is set, the sysctl will > >> automatically be pre-loaded with values from /boot/loader.conf. > >> > >> The reason we don't want the current approach is: > >> > >> 1) It duplicates the sysctl path in the TUNABLE statement. > >> 2) It does not work very well for dynamically attached sysctls. There is > >> a lot of code overhead computing the TUNABLE() path before the TUNABLE() > >> can be fetched. > >> > >> Here is a work in progress: > >> > >> http://home.selasky.org:8192/sysctl_tunable.diff > >> > >> In most cases my patch is fine, but in some other cases I need some > >> input, like in the VM subsystem when doing init, I'm not sure if the > >> SYSINIT() for subsystem SI_SUB_KMEM, which sysctl's are using, has > >> already been executed. > > > > I think this is a good idea, but it's also true you can just leave separate > > TUNABLE_ statements without setting the CTLFLAG_TUN flag for cases you aren't > > sure about for now. It probably makes sense to do these changes in stages. > > > > I was going to suggest using sbuf() for building the tunable name, but that > > doesn't work since you have to build it in reverse. > > > > Hi, > > After going through a lot of existing code, I've decided to make a new > flag, CTLFLAG_FETCH rather than overload CTLFLAG_TUN, so that the new > functionality can be added to drivers and tested. For example sysctls > which implement function callbacks and are not trivial, this might cause > locking of non-initialized mutexes and so on. And also I see some > dependencies, that values are fetched at a certain point in the boot > process and that existing CTLFLAG_TUN might confuse existing logic. > > I've updated my patch (same link): > > http://home.selasky.org:8192/sysctl_tunable.diff > > BTW: Can someone which have a beefy machine run a universe with this > patch applied? > > I'll probably put it into the tree next week. I think having CTLFLAG_TUN do this by default is probably correct in the long term. The vast majority of places that use a tunable to prime a sysctl are safe. Why not do this for the initial patch: - Add your change to auto-fetch values when CTLFLAG_TUN is set. - Instead of adding a CTLFLAG_FETCH, add a CTLFLAG_NOFETCH to disable getenv(). - Make a pass over the existing places that use CTLFLAG_TUN seeing which ones are safe (so TUNABLE_* can just be removed), and which ones aren't (in which case add CTLFLAG_NOFETCH). Followup changes can work on converting other places that don't currently use CTLFLAG_TUN but have a SYSCTL + TUNABLE to use CTLFLAG_TUN instead as well as fixing places that use CTLFLAG_NOFETCH to not need them. I would suggest you commit some of the style changes (like using explicit initializers in SYSCTL_OID()) as a separate change beforehand. -- John Baldwin