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]]
Julian Elischer
julian at freebsd.org
Wed Nov 9 02:22:42 UTC 2011
On 11/8/11 5:22 PM, Arnaud Lacombe wrote:
> 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
>>>>>>> @@ -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 at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe at freebsd.org"
>
>
More information about the freebsd-current
mailing list