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

[Petri Latvala]

On Tue, Sep 29, 2020 at 12:19:14PM +0200, Zbigniew Kempczyński wrote:
> 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.

Yeah, fair enough.

Do you have a version without the spurious whitespace changes? Slap a

Reviewed-by: Petri Latvala <petri.latvala at intel.com>

on it.