[igt-dev] [PATCH i-g-t] lib/igt_core: Reinitialize print mutex in child process

[Zbigniew Kempczyński]

On Tue, Sep 29, 2020 at 12:24:59PM +0300, Petri Latvala wrote:
> On Tue, Sep 29, 2020 at 09:53:05AM +0200, Zbigniew Kempczyński wrote:
> > On Mon, Sep 28, 2020 at 06:29:20PM +0300, Petri Latvala wrote:
> > > On Mon, Sep 28, 2020 at 03:15:02PM +0200, Zbigniew Kempczyński wrote:
> > > > IGT are prone to deadlock in igt_log() in following scenario:
> > > > 
> > > > 1. Parent process creates additional thread which for example
> > > >    is doing endless loop
> > > > 2. Thread sometimes is logging to console using igt_info().
> > > >    This locks and unlocks print_mutex.
> > > > 3. If in the meantime parent process will spawn it can be
> > > >    spawned with print_mutex locked. If in the child process
> > > >    it will do igt_info() it will deadlock.
> > > > 
> > > > We should reinitialize print_mutex in child process to avoid
> > > > use inherited value.
> > > > 
> > > > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > > > Cc: Petri Latvala <petri.latvala at intel.com>
> > > > ---
> > > 
> > > Are you able to type up a unit test in lib/tests/ that reproduces this
> > > deadlock?
> > 
> > I've attached patch which exposes the deadlock. Unfortunately I don't
> > think the test which produces thousands of log lines is much usable
> > in the future. In the patch I intentionally increased critical
> > section time to 1 second to spot the problem. In my opinion patch I've 
> > sent yesterday should work and I don't know should we add additional
> > test to verify this scenario in the future.
> 
> No no, I meant _lib_/tests/. tests/ contains tests that check whether
> kernel works correctly, lib/tests/ contains tests that check whether
> IGT infrastructure works correctly.
> 
> The structure of the tests in lib/tests can be a bit awkward since you
> need to do a lot of the test framework stuff manually, but there
> should be plenty of examples.
> 

I don't have currently much time to work on test version you likely 
expect. I think we know what's the root cause of not proper IGT logging
behavior in multithreading/multiprocessing environmant and fix is quite
obvious. I don't think we need test which will test that lock verification.
Especially to reproduce I have to do atomic bomb of igt_info() which 
completely dominate in output and produces have thousaunds of lines before
deadlock occurs.

Anyway, I needed such logging during my development so in final version
of patches they will disappear. So decision of incorporation of my fix
or not I left to you.

--
Zbigniew