From owner-freebsd-current@FreeBSD.ORG Tue Nov 8 18:49:14 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 39412106566B; Tue, 8 Nov 2011 18:49:14 +0000 (UTC) (envelope-from lacombar@gmail.com) Received: from mail-wy0-f182.google.com (mail-wy0-f182.google.com [74.125.82.182]) by mx1.freebsd.org (Postfix) with ESMTP id 336FB8FC21; Tue, 8 Nov 2011 18:49:12 +0000 (UTC) Received: by wyg36 with SMTP id 36so1129744wyg.13 for ; Tue, 08 Nov 2011 10:49:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=TD0RHFqWfRztgZ/7C/LlINmD/sjFi9ebkzE4YlLtuyY=; b=giK7brY2pWlfbGL7kWbf4zqon7BGBKfaRLBq7nVgd3dD7kKXalOwBDu4IPGDIy2b7n HYV3CqndI5+czXeIaxY7Wx2mQjt+49/nvOzrurvTJkZbS3HKxKLmpZy2HjWndYCaT18c mZ7Qdp0ICQKl9E6vNbeXT7Zr63aamixtOYQ6M= MIME-Version: 1.0 Received: by 10.181.13.165 with SMTP id ez5mr14769497wid.51.1320778151716; Tue, 08 Nov 2011 10:49:11 -0800 (PST) Received: by 10.180.81.200 with HTTP; Tue, 8 Nov 2011 10:49:10 -0800 (PST) Date: Tue, 8 Nov 2011 13:49:10 -0500 Message-ID: From: Arnaud Lacombe To: Attilio Rao Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: "K. Macy" , Alan Cox , Andriy Gapon , freebsd-current@freebsd.org, Benjamin Kaduk , Kostik Belousov , Penta Upa Subject: Using Instruction Pointer address in debug interfaces [Was: Re: vm_page_t related KBI [Was: Re: panic at vm_page_wire with FreeBSD 9.0 Beta 3]] 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, 08 Nov 2011 18:49:14 -0000 Hi, On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe wrote: > On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao wrote: >> 2011/11/7 Arnaud Lacombe : >>> Hi, >>> >>> On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov = wrote: >>>> On Fri, Nov 04, 2011 at 06:03:39PM +0200, Kostik Belousov wrote: >>>> >>>> Below is the KBI patch after vm_page_bits_t merge is done. >>>> Again, I did not spent time converting all in-tree consumers >>>> from the (potentially) loadable modules to the new KPI until it >>>> is agreed upon. >>>> >>>> diff --git a/sys/nfsclient/nfs_bio.c b/sys/nfsclient/nfs_bio.c >>>> index 305c189..7264cd1 100644 >>>> --- a/sys/nfsclient/nfs_bio.c >>>> +++ b/sys/nfsclient/nfs_bio.c >>>> @@ -128,7 +128,7 @@ nfs_getpages(struct vop_getpages_args *ap) >>>> =A0 =A0 =A0 =A0 * can only occur at the file EOF. >>>> =A0 =A0 =A0 =A0 */ >>>> =A0 =A0 =A0 =A0VM_OBJECT_LOCK(object); >>>> - =A0 =A0 =A0 if (pages[ap->a_reqpage]->valid !=3D 0) { >>>> + =A0 =A0 =A0 if (vm_page_read_valid(pages[ap->a_reqpage]) !=3D 0) { >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0for (i =3D 0; i < npages; ++i) { >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (i !=3D ap->a_reqpag= e) { >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vm_page= _lock(pages[i]); >>>> @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * Read operation fille= d an entire page >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */ >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 m->valid =3D VM_PAGE_BIT= S_ALL; >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 KASSERT(m->dirty =3D=3D = 0, >>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 vm_page_write_valid(m, V= M_PAGE_BITS_ALL); >>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 KASSERT(vm_page_read_dir= ty(m) =3D=3D 0, >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0("nfs_getpages:= page %p is dirty", m)); >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else if (size > toff) { >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * Read operation fille= d a partial page. >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */ >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 m->valid =3D 0; >>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 vm_page_write_valid(m, 0= ); >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vm_page_set_valid(m, 0,= size - toff); >>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 KASSERT(m->dirty =3D=3D = 0, >>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 KASSERT(vm_page_read_dir= ty(m) =3D=3D 0, >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0("nfs_getpages:= page %p is dirty", m)); >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else { >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* >>>> diff --git a/sys/vm/vm_page.c b/sys/vm/vm_page.c >>>> index 389aea5..2f41e70 100644 >>>> --- a/sys/vm/vm_page.c >>>> +++ b/sys/vm/vm_page.c >>>> @@ -2677,6 +2677,66 @@ vm_page_test_dirty(vm_page_t m) >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vm_page_dirty(m); >>>> =A0} >>>> >>>> +void >>>> +vm_page_lock_func(vm_page_t m, const char *file, int line) >>>> +{ >>>> + >>>> +#if LOCK_DEBUG > 0 || defined(MUTEX_NOINLINE) >>>> + =A0 =A0 =A0 _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); >>>> +#else >>>> + =A0 =A0 =A0 __mtx_lock(vm_page_lockptr(m), 0, file, line); >>>> +#endif >>>> +} >>>> + >>> Why do you re-implement the wheel ? all the point of these assessors >>> is to hide implementation detail. IMO, you should restrict yourself to >>> the documented API from mutex(9), only. >>> >>> Oh, wait, you end-up using LOCK_FILE instead of just __FILE__, but >>> wait LOCK_FILE is either just __FILE__, or NULL, depending on >>> LOCK_DEBUG, but you wouldn't have those function without >>> INVARIANTS.... This whole LOCK_FILE/LOCK_LINE seem completely >>> fracked-up... If you don't want this code in INVARIANTS, but in >>> LOCK_DEBUG, only make it live only in the LOCK_DEBUG case. >>> >>> Btw, let me also question the use of non-inline functions. >> >> My impression is that you don't really understand the patch, thus your >> disrespectful words used here are really unjustified. >> > Well, unfortunately, I wasn't around to comment 10 years ago when this go= t in. > >> I think that kib@ intention is just to get "the most official way" to >> pass down file and line to locking functions from the consumers. >> His patch is "technically right", but I would prefer something >> different (see below). >> > Yes, and that's not an excuse to use the _internal_ implementation > details of mutex(9), and not the exposed API. This is especially true > when applied to someone so eager to follow "standards". > >> LOCK_FILE and LOCK_LINE exist for reducing the space in .rodata >> section. Without INVARIANTS/WITNESS/etc. they will just be NULL and >> not pointing to a lot of datas that won't be used in the opposite >> case. >> > You comment just as if __FILE__ and __LINE__ were mandatory in a debug > interface. This is _not_ true. __FILE__ and __LINE__ are just hideous > for debugging of anything but early alpha code. LOCK_FILE and > LOCK_LINE are a bad answer to a bad interface. Even if you just pass > NULL and 0 as argument, you've got the bloat of passing unused > argument. You might as well just pass a single argument that would do > the exact same job and be _always_ available, eg. the IP of the > caller, or the first return address. KDB magic still let you translate > to something humanly understandable. If the toolchain does not support > the feature on all supported platform, well, fix the toolchain. > To avoid future complaints about the fact that I would be only "talk" without "action", I did implement what I suggested above. As it is quite a large patch-set, I will not post it directly here, however, it is available on github: https://github.com/lacombar/freebsd/tree/master/topic/kern-lock-debug It convert a bunch of debug interface to use the caller instruction pointer, as well as a proof-of-concept teaching printf(9) to convert IP to symbol_name+offset. It translates in a direct saving of about +250kB on i386's GENERIC, just in kernel text size. Even the worst case, ie LOCK_DEBUG =3D=3D 0, translates to a save of +80kB. Please note that this is still WIP code. Comments welcome. - Arnaud