From owner-freebsd-arch Sat Jul 20 14:39:47 2002 Delivered-To: freebsd-arch@freebsd.org Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 92C1737B400 for ; Sat, 20 Jul 2002 14:39:39 -0700 (PDT) Received: from gw.catspoiler.org (217-ip-163.nccn.net [209.79.217.163]) by mx1.FreeBSD.org (Postfix) with ESMTP id C158143E3B for ; Sat, 20 Jul 2002 14:39:38 -0700 (PDT) (envelope-from dl-slicex@catspoiler.org) Received: from mousie.catspoiler.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.12.5/8.12.5) with ESMTP id g6KLdRwr041480; Sat, 20 Jul 2002 14:39:32 -0700 (PDT) (envelope-from dl-slicex@catspoiler.org) Message-Id: <200207202139.g6KLdRwr041480@gw.catspoiler.org> Date: Sat, 20 Jul 2002 14:39:27 -0700 (PDT) From: Don Lewis Subject: Re: wiring the sysctl output buffer To: bde@zeta.org.au Cc: arch@FreeBSD.ORG In-Reply-To: <200207170755.g6H7t1wr029257@gw.catspoiler.org> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=US-ASCII Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On 17 Jul, Don Lewis wrote: > My initial solution was to allocate a temporary buffer, but that > complicates the code and the similar code might have to be added to > other parts of the kernel where similar situations might arise. It was > not until later that I thought of pre-wiring the buffer, which looks > like a much cleaner solution. In the latter case, I decided that it > would be best to abstact the interface so that the handler doesn't need > to become intimate with the internals of the sysctl system. If this > solution allows us to avoid the call to vslock() in the majority of > cases, then so much the better. > > The initial implementation of the interface to vslock() would probably > just ignore the size argument, but defining the API this way would allow > it to be used later if necessary without the necessity of retrofitting > the handlers that use it. Attached is another iteration of the patch. I added a length parameter to the vslock() wrapper, though this parameter is currently ignored. I modified the standard handlers to either make a temporary copy of the data or to wire the buffer as appropriate. I also added an assertion to sysctl_old_user() to catch calls to SYSCTL_OUT() that may block while locks are held. This assertion has already turned up some other calls to SYSCTL_OUT() where locks are held. This patch also includes the sysctl_kern_function_list() fix. What's the scoop with CTLFLAG_NOLOCK? Should it be used to retrieve static data? If so, should the default handlers skip the extra copy if this flag is set? Index: kern_sysctl.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_sysctl.c,v retrieving revision 1.127 diff -u -r1.127 kern_sysctl.c --- kern_sysctl.c 15 Jul 2002 17:28:34 -0000 1.127 +++ kern_sysctl.c 20 Jul 2002 21:10:42 -0000 @@ -57,6 +57,7 @@ static MALLOC_DEFINE(M_SYSCTL, "sysctl", "sysctl internal magic"); static MALLOC_DEFINE(M_SYSCTLOID, "sysctloid", "sysctl dynamic oids"); +static MALLOC_DEFINE(M_SYSCTLTMP, "sysctltmp", "sysctl temp output buffer"); /* * Locking - this locks the sysctl tree in memory. @@ -751,12 +752,16 @@ int sysctl_handle_int(SYSCTL_HANDLER_ARGS) { - int error = 0; + int tmpout, error = 0; + /* + * Attempt to get a coherent snapshot by making a copy of the data. + */ if (arg1) - error = SYSCTL_OUT(req, arg1, sizeof(int)); + tmpout = *(int *)arg1; else - error = SYSCTL_OUT(req, &arg2, sizeof(int)); + tmpout = arg2; + error = SYSCTL_OUT(req, &tmpout, sizeof(int)); if (error || !req->newptr) return (error); @@ -776,10 +781,15 @@ sysctl_handle_long(SYSCTL_HANDLER_ARGS) { int error = 0; + long tmpout; + /* + * Attempt to get a coherent snapshot by making a copy of the data. + */ if (!arg1) return (EINVAL); - error = SYSCTL_OUT(req, arg1, sizeof(long)); + tmpout = *(long *)arg1; + error = SYSCTL_OUT(req, &tmpout, sizeof(long)); if (error || !req->newptr) return (error); @@ -799,8 +809,23 @@ sysctl_handle_string(SYSCTL_HANDLER_ARGS) { int error=0; + char *tmparg; + size_t outlen; - error = SYSCTL_OUT(req, arg1, strlen((char *)arg1)+1); + /* + * Attempt to get a coherent snapshot by copying to a + * temporary kernel buffer. + */ +retry: + outlen = strlen((char *)arg1)+1; + tmparg = malloc(outlen, M_SYSCTLTMP, M_WAITOK); + strncpy(tmparg, (char *)arg1, outlen); + if (tmparg[outlen-1] != '\0') { + free(tmparg, M_SYSCTLTMP); + goto retry; + } + error = SYSCTL_OUT(req, tmparg, outlen); + free(tmparg, M_SYSCTLTMP); if (error || !req->newptr) return (error); @@ -825,8 +850,23 @@ sysctl_handle_opaque(SYSCTL_HANDLER_ARGS) { int error; + void *tmparg; - error = SYSCTL_OUT(req, arg1, arg2); + /* + * Attempt to get a coherent snapshot, either by wiring the + * user space buffer or copying to a temporary kernel buffer + * depending on the size of the data. + */ + if (arg2 > PAGE_SIZE) { + sysctl_wire_old_buffer(req, arg2); + error = SYSCTL_OUT(req, arg1, arg2); + } else { + tmparg = malloc(arg2, M_SYSCTLTMP, M_WAITOK); + strcpy(tmparg, (char *)arg1); + bcopy(arg1, tmparg, arg2); + error = SYSCTL_OUT(req, tmparg, arg2); + free(tmparg, M_SYSCTLTMP); + } if (error || !req->newptr) return (error); @@ -953,10 +993,8 @@ int error = 0; size_t i = 0; - if (req->lock == 1 && req->oldptr) { - vslock(req->oldptr, req->oldlen); - req->lock = 2; - } + if (req->lock == 1 && req->oldptr) + WITNESS_SLEEP(1, NULL); if (req->oldptr) { i = l; if (req->oldlen <= req->oldidx) @@ -988,6 +1026,23 @@ error = copyin((char *)req->newptr + req->newidx, p, l); req->newidx += l; return (error); +} + +/* + * Wire the user space destination buffer. If set to a value greater than + * zero, the len parameter limits the maximum amount of wired memory. + * + * XXX - The len parameter is currently ignored due to the lack of + * a place to save it in the sysctl_req structure so that the matching + * amount of memory can be unwired in the sysctl exit code. + */ +void +sysctl_wire_old_buffer(struct sysctl_req *req, size_t len) +{ + if (req->lock == 1 && req->oldptr && req->oldfunc == sysctl_old_user) { + vslock(req->oldptr, req->oldlen); + req->lock = 2; + } } int Index: kern_linker.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_linker.c,v retrieving revision 1.91 diff -u -r1.91 kern_linker.c --- kern_linker.c 7 Jul 2002 22:35:47 -0000 1.91 +++ kern_linker.c 17 Jul 2002 10:37:50 -0000 @@ -1794,6 +1794,7 @@ linker_file_t lf; int error; + sysctl_wire_old_buffer(req, 0); mtx_lock(&kld_mtx); TAILQ_FOREACH(lf, &linker_files, link) { error = LINKER_EACH_FUNCTION_NAME(lf, To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message