On Sat, Nov 11, 2017 at 1:14 AM, Taylor R Campbell <campbell+netbsd-tech-k...@mumble.net> wrote: >> Date: Fri, 10 Nov 2017 18:54:58 +0900 >> From: Ryota Ozaki <ozak...@netbsd.org> >> >> 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