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]]

From: Arnaud Lacombe <lacombar_at_gmail.com>
Date: Tue, 8 Nov 2011 20:22:09 -0500
Hi,

On Tue, Nov 8, 2011 at 3:34 PM, Attilio Rao <attilio_at_freebsd.org> wrote:
> 2011/11/8 Arnaud Lacombe <lacombar_at_gmail.com>:
>> Hi,
>>
>> On Mon, Nov 7, 2011 at 2:03 PM, Arnaud Lacombe <lacombar_at_gmail.com> wrote:
>>> On Mon, Nov 7, 2011 at 4:36 AM, Attilio Rao <attilio_at_freebsd.org> wrote:
>>>> 2011/11/7 Arnaud Lacombe <lacombar_at_gmail.com>:
>>>>> Hi,
>>>>>
>>>>> On Sat, Nov 5, 2011 at 10:13 AM, Kostik Belousov <kostikbel_at_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
>>>>>> _at__at_ -128,7 +128,7 _at__at_ 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]);
>>>>>> _at__at_ -198,16 +198,16 _at__at_ 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
>>>>>> _at__at_ -2677,6 +2677,66 _at__at_ 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_at_ 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
Received on Wed Nov 09 2011 - 00:22:11 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:20 UTC