Re: Debugging features for pserialize(9)

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Mon, Nov 13, 2017 at 12:44 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> On Sat, Nov 11, 2017 at 1:14 AM, Taylor R Campbell
> <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>>> Date: Fri, 10 Nov 2017 18:54:58 +0900
>>> From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
>>>
>>> Hi,
>>>
>>> I implemented two debugging features for pserialize(9):
>>>   http://www.netbsd.org/~ozaki-r/debugging-pserialize.diff
>>>
>>> It detects:
>>> - if a context switch happens in a read section, and
>>> - if a sleepable function is called in a read section.
>>>
>>> Any comments or suggestions? Is the implementation correct?
>>>
>>> Anyway the features have found out two new bugs :)
>>
>> Looks great!  I'm not surprised it found bugs.  We should have done
>> something like this a long time ago.
>>
>> One bug: LOCKDEBUG is not suposed to change the kernel ABI.  So you
>> should unconditionally define two different functions:
>>
>> pserialize_in_read_section()
>>
>>    True unless we can _guarantee_ the caller is not in a pserialize
>>    read section.  To be used only for diagnostic assertions like:
>>
>>    KASSERT(pserialize_in_read_section());
>>
>> pserialize_not_in_read_section()
>>
>>    True unless we can _guarantee_ the caller is in a pserialize read
>>    section.  To be used only for diagnostic assertions like:
>>
>>    KASSERT(pserialize_not_in_read_section());
>>
>> That way, modules will continue to work with or without LOCKDEBUG.
>>
>> assert_sleepable would then use !pserialize_not_in_read_section(),
>> which is a slightly confusing double-negative, but it would be as if
>> you had written KASSERT(pserialize_not_in_read_section()) -- the
>> KASSERT internally produces the same `if (!...)'.
>> pserialize_switchpoint can then also just
>> KASSERT(pserialize_not_in_read_section()).
>
> That really makes sense! I revised the patch:
>   http://www.netbsd.org/~ozaki-r/debugging-pserialize.revised.diff
>
>>
>>
>> One nit: Any need to use signed int for the nread?  Why not unsigned?
>
> Just I want to detect the counter being negative naturally, though
> using signed int is not must.

So I've changed the patch to use uint32_t:
  http://www.netbsd.org/~ozaki-r/debugging-pserialize.uint32_t.diff

  ozaki-r