Re: Debugging features for pserialize(9)

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> 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()).


One nit: Any need to use signed int for the nread?  Why not unsigned?