Re: Debugging features for pserialize(9)

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

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.

Thanks,
  ozaki-r