From owner-freebsd-current@FreeBSD.ORG Wed Nov 9 02:22:42 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 83D13106564A; Wed, 9 Nov 2011 02:22:42 +0000 (UTC) (envelope-from julian@freebsd.org) Received: from vps1.elischer.org (vps1.elischer.org [204.109.63.16]) by mx1.freebsd.org (Postfix) with ESMTP id 59E818FC14; Wed, 9 Nov 2011 02:22:39 +0000 (UTC) Received: from julian-mac.elischer.org (home-nat.elischer.org [67.100.89.137]) (authenticated bits=0) by vps1.elischer.org (8.14.4/8.14.4) with ESMTP id pA92MVVZ022642 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Tue, 8 Nov 2011 18:22:32 -0800 (PST) (envelope-from julian@freebsd.org) Message-ID: <4EB9E3E2.9060502@freebsd.org> Date: Tue, 08 Nov 2011 18:22:26 -0800 From: Julian Elischer User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9.2.23) Gecko/20110920 Thunderbird/3.1.15 MIME-Version: 1.0 To: Arnaud Lacombe References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: "K. Macy" , Alan Cox , Andriy Gapon , Attilio Rao , freebsd-current@freebsd.org, Benjamin Kaduk , Kostik Belousov , Penta Upa Subject: Re: 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: Wed, 09 Nov 2011 02:22:42 -0000 On 11/8/11 5:22 PM, Arnaud Lacombe wrote: > Hi, > > On Tue, Nov 8, 2011 at 3:34 PM, Attilio Rao wrote: >> 2011/11/8 Arnaud Lacombe: >>> 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) >>>>>>> * can only occur at the file EOF. >>>>>>> */ >>>>>>> VM_OBJECT_LOCK(object); >>>>>>> - if (pages[ap->a_reqpage]->valid != 0) { >>>>>>> + if (vm_page_read_valid(pages[ap->a_reqpage]) != 0) { >>>>>>> for (i = 0; i< npages; ++i) { >>>>>>> if (i != ap->a_reqpage) { >>>>>>> vm_page_lock(pages[i]); >>>>>>> @@ -198,16 +198,16 @@ nfs_getpages(struct vop_getpages_args *ap) >>>>>>> /* >>>>>>> * Read operation filled an entire page >>>>>>> */ >>>>>>> - m->valid = VM_PAGE_BITS_ALL; >>>>>>> - KASSERT(m->dirty == 0, >>>>>>> + vm_page_write_valid(m, VM_PAGE_BITS_ALL); >>>>>>> + KASSERT(vm_page_read_dirty(m) == 0, >>>>>>> ("nfs_getpages: page %p is dirty", m)); >>>>>>> } else if (size> toff) { >>>>>>> /* >>>>>>> * Read operation filled a partial page. >>>>>>> */ >>>>>>> - m->valid = 0; >>>>>>> + vm_page_write_valid(m, 0); >>>>>>> vm_page_set_valid(m, 0, size - toff); >>>>>>> - KASSERT(m->dirty == 0, >>>>>>> + KASSERT(vm_page_read_dirty(m) == 0, >>>>>>> ("nfs_getpages: page %p is dirty", m)); >>>>>>> } else { >>>>>>> /* >>>>>>> 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) >>>>>>> vm_page_dirty(m); >>>>>>> } >>>>>>> >>>>>>> +void >>>>>>> +vm_page_lock_func(vm_page_t m, const char *file, int line) >>>>>>> +{ >>>>>>> + >>>>>>> +#if LOCK_DEBUG> 0 || defined(MUTEX_NOINLINE) >>>>>>> + _mtx_lock_flags(vm_page_lockptr(m), 0, file, line); >>>>>>> +#else >>>>>>> + __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 got 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: >> I really think that this is way too dependent by the good health of >> your tool, thus that is highly fragile. >> > then fix the tools to be more robust. > >> However, you may have more luck by just the core of your kernel >> changes here, for comment and leave alone all the (ptr -> >> LOCK_FILE/LOCK_LINE conversion). >> >> Said that, I think this logic is too fragile and likely won't be as >> effective as __FILE__/__LINE__ in many cases. >> > Let point out a contradiction; if __FILE__/__LINE__ are so robust, and > if tools to inspect kernel are so broken and fragile, why don't you > make ddb/kdb reports those locations, by default, instead of > symbol+offset when it displays a backtrace ? gdb does, and ddb doesn't have the information available. > - Arnaud > _______________________________________________ > freebsd-current@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-current > To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org" > >