Date: Tue, 8 Nov 2011 13:49:10 -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: 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: <CACqU3MU_Bk%2BcObCiUa2XtM7fLkSpSDzOZqoZ=khNOR-_6ptYYQ@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
Hi,
On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> 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_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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACqU3MU_Bk%2BcObCiUa2XtM7fLkSpSDzOZqoZ=khNOR-_6ptYYQ>