[igt-dev] [PATCH i-g-t] lib/igt_core: Reinitialize print mutex in child process
Petri Latvala
petri.latvala at intel.com
Tue Sep 29 09:24:59 UTC 2020
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.
--
Petri Latvala
>
> --
> Zbigniew
>
> From 998d15a647091c5db0ddee8ad52804cbbff2bd7f Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Zbigniew=20Kempczy=C5=84ski?=
> <zbigniew.kempczynski at intel.com>
> Date: Tue, 29 Sep 2020 09:43:14 +0200
> Subject: [PATCH i-g-t] lib/igt_core: Expose deadlock in igt logging
>
> Just for expose.
> ---
> lib/igt_core.c | 13 +++++++--
> tests/Makefile.sources | 1 +
> tests/core_logging.c | 66 ++++++++++++++++++++++++++++++++++++++++++
> tests/meson.build | 1 +
> 4 files changed, 79 insertions(+), 2 deletions(-)
> create mode 100644 tests/core_logging.c
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index cedd8168..c26927cd 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -2271,8 +2271,12 @@ static void children_exit_handler(int sig)
> ;
> }
>
> +static pthread_mutex_t print_mutex = PTHREAD_MUTEX_INITIALIZER;
> +
> bool __igt_fork(void)
> {
> + int ret;
> +
> internal_assert(!test_with_subtests || in_subtest,
> "forking is only allowed in subtests or igt_simple_main\n");
> internal_assert(!test_child,
> @@ -2300,6 +2304,11 @@ bool __igt_fork(void)
> igt_assert(0);
> case 0:
> test_child = true;
> + /* Below fixes issue */
> + //pthread_mutex_init(&print_mutex, NULL);
> + ret = pthread_mutex_trylock(&print_mutex);
> + internal_assert(ret == 0, "Mutex was locked during fork\n");
> + pthread_mutex_unlock(&print_mutex);
> exit_handler_count = 0;
> reset_helper_process_list();
> oom_adjust_for_doom();
> @@ -2737,8 +2746,6 @@ void igt_vlog(const char *domain, enum igt_log_level level, const char *format,
> "NONE"
> };
>
> - static pthread_mutex_t print_mutex = PTHREAD_MUTEX_INITIALIZER;
> -
> assert(format);
>
> #ifdef __GLIBC__
> @@ -2816,6 +2823,8 @@ void igt_vlog(const char *domain, enum igt_log_level level, const char *format,
> fwrite(line, sizeof(char), strlen(line), file);
> }
>
> + /* Extend critical section time */
> + sleep(1);
> pthread_mutex_unlock(&print_mutex);
>
> out:
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index f32ea9cf..e027cab1 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -19,6 +19,7 @@ TESTS_progs = \
> core_getstats \
> core_getversion \
> core_hotunplug \
> + core_logging \
> core_setmaster \
> core_setmaster_vs_auth \
> debugfs_test \
> diff --git a/tests/core_logging.c b/tests/core_logging.c
> new file mode 100644
> index 00000000..826ec85b
> --- /dev/null
> +++ b/tests/core_logging.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright © 2020 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include "igt.h"
> +#include <pthread.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +
> +IGT_TEST_DESCRIPTION("Tests is IGT logging prone to deadlock");
> +
> +#define TIMEOUT 10
> +
> +static void *thread_loop(void *data)
> +{
> + int cnt = 0;
> +
> + igt_until_timeout(TIMEOUT) {
> + igt_info("... %d\n", cnt++);
> + }
> +
> + return NULL;
> +}
> +
> +static void test_deadlock(void)
> +{
> + pthread_t thread;
> +
> + pthread_create(&thread, NULL, thread_loop, NULL);
> +
> + igt_until_timeout(TIMEOUT) {
> + igt_fork(child, 1) {
> + igt_info("child [%d]\n", getpid());
> + }
> + //igt_waitchildren_timeout(1, "deadlock found");
> + igt_waitchildren();
> + }
> +
> + pthread_join(thread, NULL);
> +}
> +
> +igt_main
> +{
> + igt_subtest("test-deadlock")
> + test_deadlock();
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index 515f7528..ebdc1aba 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -4,6 +4,7 @@ test_progs = [
> 'core_getstats',
> 'core_getversion',
> 'core_hotunplug',
> + 'core_logging',
> 'core_setmaster',
> 'core_setmaster_vs_auth',
> 'debugfs_test',
> --
> 2.26.0
>
More information about the igt-dev
mailing list