Date: Tue, 8 Nov 2011 20:22:09 -0500 From: Arnaud Lacombe <lacombar@gmail.com> To: Attilio Rao <attilio@freebsd.org> Cc: "K. Macy" <kmacy@freebsd.org>, Alan Cox <alc@rice.edu>, Andriy Gapon <avg@freebsd.org>, freebsd-current@freebsd.org, Benjamin Kaduk <kaduk@mit.edu>, Kostik Belousov <kostikbel@gmail.com>, Penta Upa <bsdboot@gmail.com> 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]] Message-ID: <CACqU3MUqc82R381NFJ-8=k9OZFd3GswFKEFQ3Bc1T=M_1Fq0Jg@mail.gmail.com> In-Reply-To: <CAJ-FndDOWpotphH4pmn0S6QqJybx74A2Kt4a8aiT96x0f2cRZA@mail.gmail.com> References: <CACqU3MU_Bk%2BcObCiUa2XtM7fLkSpSDzOZqoZ=khNOR-_6ptYYQ@mail.gmail.com> <CAJ-FndDOWpotphH4pmn0S6QqJybx74A2Kt4a8aiT96x0f2cRZA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi, On Tue, Nov 8, 2011 at 3:34 PM, Attilio Rao <attilio@freebsd.org> wrote: > 2011/11/8 Arnaud Lacombe <lacombar@gmail.com>: >> Hi, >> >> On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe <lacombar@gmail.com> wrot= e: >>> On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao <attilio@freebsd.org> wrote= : >>>> 2011/11/7 Arnaud Lacombe <lacombar@gmail.com>: >>>>> Hi, >>>>> >>>>> On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov <kostikbel@gmail.com= > 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_reqp= age) { >>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vm_pa= ge_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 fil= led 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_B= ITS_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,= VM_PAGE_BITS_ALL); >>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 KASSERT(vm_page_read_d= irty(m) =3D=3D 0, >>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0("nfs_getpage= s: 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 fil= led 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_d= irty(m) =3D=3D 0, >>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0("nfs_getpage= s: 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 t= o >>>>> 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 ? - Arnaud
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACqU3MUqc82R381NFJ-8=k9OZFd3GswFKEFQ3Bc1T=M_1Fq0Jg>