From owner-freebsd-current@FreeBSD.ORG Tue Aug 9 02:52:31 2005 Return-Path: X-Original-To: current@freebsd.org Delivered-To: freebsd-current@FreeBSD.ORG Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 9392916A41F for ; Tue, 9 Aug 2005 02:52:31 +0000 (GMT) (envelope-from freebsd@lovett.com) Received: from mail.lovett.com (foo.lovett.com [67.134.38.158]) by mx1.FreeBSD.org (Postfix) with ESMTP id 472BB43D53 for ; Tue, 9 Aug 2005 02:52:31 +0000 (GMT) (envelope-from freebsd@lovett.com) Received: from hellfire.lovett.com ([67.134.38.149]:58801) by mail.lovett.com with esmtpa (Exim 4.52 (FreeBSD)) id 1E2KE8-0000qZ-N6; Mon, 08 Aug 2005 19:52:28 -0700 Message-ID: <42F81A5C.8090600@FreeBSD.org> Date: Mon, 08 Aug 2005 19:52:12 -0700 From: Ade Lovett User-Agent: Mozilla Thunderbird 1.0.6 (Macintosh/20050716) X-Accept-Language: en-us, en MIME-Version: 1.0 To: Scott Long References: <42F7D104.2020103@FreeBSD.org> <42F80A41.8050901@samsco.org> In-Reply-To: <42F80A41.8050901@samsco.org> X-Enigmail-Version: 0.92.0.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: freebsd@lovett.com X-SA-Exim-Connect-IP: 67.134.38.149 X-SA-Exim-Mail-From: freebsd@lovett.com X-SA-Exim-Scanned: No (on mail.lovett.com); SAEximRunCond expanded to false Cc: current@freebsd.org Subject: Re: Serious performance issues, broken initialization, and a likely fix X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 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: Tue, 09 Aug 2005 02:52:31 -0000 Scott Long wrote: > Ade Lovett wrote: >> To cut a long story short, the order in which nswbuf is being >> initialized is completely, totally, and utterly wrong -- this was >> introduced by revision 1.132 of vm/vnode_pager.c just over 4 years ago. >> [snip] > My vote is to revert rev 1.132 and replace the XXX comment with a more > detailed explaination of the perils involved. Do you have any kind of > easy to run regression test that could be used to quantify this problem > and guard against it in the future? Thanks very very much for looking > into it and providing such a good explaination. It's not quite as easy as reverting 1.132, since we're talking about at least two files here, vm/vnode_pager.c and kern/vfs_bio.c How to handle it properly is a little more complicated since the code that actually assigns a value to nswbuf is inside kern_vfs_bio_buffer_alloc() which can be called multiple times as part of the again goto loop in vm_ksubmap_init(). My personal preference would be to rip out lines 446-494 (RELENG_6) of kern_vfs_bio_buffer_alloc() into a separate function, which sets nbuf and nswbuf, and also have vnode_pager_init() call that routine. As to an easy to run regression test, the code clearly states that there is a minimum value of nswbuf: * swbufs are used as temporary holders for I/O, such as paging I/O. * We have no less then 16 and no more then 256. However, the check for this is based on: options NSWBUF_MIN= being defined in the kernel configuration file (and propagated to kernel/opt_swap.h as part of config(8). Defining a couple of handy constants (in sys/conf.h ?): #define NSWBUF_ABSOLUTE_MIN 16 #define NSWBUF_ABSOLUTE_MAX 256 The current check (based on NSWBUF_MIN) could be rewritten as follows: #ifdef NSWBUF_MIN if (nswbuf < NSWBUF_MIN) nswbuf = NSWBUF_MIN; #endif if (nswbuf < NSWBUF_ABSOLUTE_MIN) { printf("Warning: nswbuf too low (%d), setting to %d\n", nswbuf, NSWBUF_ABSOLUTE_MIN); nswbuf = NSWBUF_ABSOLUTE_MIN; } if (nswbuf > NSWBUF_ABSOLUTE_MAX) { printf("Warning: nswbuf too high (%d), setting to %d\n", nswbuf, NSWBUF_ABSOLUTE_MAX); nswbuf = NSWBUF_ABSOLUTE_MAX; } and then, along with the code moving outlined above, having an undefined value (or 0) of nswbuf could Never Happen[tm]. Indeed, at this point, where nswbuf is actually used in the RHS of an assignment, one could even force a panic if it somehow it got missed in future -- I believe in this case, a full blown panic would be justified, given the massive loss of performance that occurs with only one swap buffer. -aDe