Re: [Intel-gfx] [PATCH i-g-t v2 04/11] lib/kunit: Parse KTAP report from the main process thread
Hi Janusz,
On 2023-10-10 at 19:49:35 +0200, Janusz Krzysztofik wrote:
> Hi Kamil,
>
> Thanks for review.
>
> On Tuesday, 10 October 2023 17:59:56 CEST Kamil Konieczny wrote:
> > Hi Janusz,
> > On 2023-10-09 at 14:27:55 +0200, Janusz Krzysztofik wrote:
> > > There was an attempt to parse KTAP reports in the background while a kunit
> > > test module is loading. However, since dynamic sub-subtests can be
> > > executed only from the main thread, that attempt was not quite successful,
> > > as IGT results from all executed kunit test cases were generated only
> > > after loading of kunit test module completed.
> > >
> > > Now that the parser maintains its state and we can call it separately for
> > > each input line of a KTAP report, it is perfectly possible to call the
> > > parser from the main thread while the module is loading in the background,
> > > and convert results from kunit test cases immediately to results of IGT
> > > dynamic sub-subtests by running an igt_dynamic() section for each result
> > > as soon as returned by the parser.
> > >
> > > Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> > > result obtained from igt_ktap_parse() called from the main thread.
> > >
> > > Also, drop no longer needed functions from igt_ktap soruces.
> > >
> > > v3: Fix ktap structure not freed on lseek error,
> > > - fix initial SIGCHLD handler not restored,
> > > - fix missing handling of potential errors returned by sigaction,
> > > - fix potential race of read() vs. ptherad_kill(), use robust mutex for
> > > synchronization with modprobe thread,
> > > - fix potentially illegal use of igt_assert() called outside of
> > > dynamic sub-subtest section,
> > > - fix unsupported exit code potentially passed to igt_fail(),
> > > - no need to fail a dynamic sub-subtest on potential KTAP parser error
> > > after a valid result from the parser has been processed,
> > > - fix trailing newlines missing from error messages,
> > > - add more debug statements,
> > > - integrate common code around kunit_result_free() into it.
> > > v2: Interrupt blocking read() on modprobe failure.
> > >
> > > Signed-off-by: Janusz Krzysztofik
> > > Acked-by: Mauro Carvalho Chehab # v2
> > > ---
> > > lib/igt_kmod.c | 261 +++
> > > lib/igt_ktap.c | 568 -
> > > lib/igt_ktap.h | 22 --
> > > 3 files changed, 222 insertions(+), 629 deletions(-)
> > >
> > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > > index 426ae5b26f..7bca4cdaab 100644
> > > --- a/lib/igt_kmod.c
> > > +++ b/lib/igt_kmod.c
> > > @@ -1,5 +1,5 @@
> > > /*
> > > - * Copyright © 2016 Intel Corporation
> > > + * Copyright © 2016-2023 Intel Corporation
> > > *
> > > * Permission is hereby granted, free of charge, to any person obtaining
> > > a
> > > * copy of this software and associated documentation files (the
> > > "Software"),
> > > @@ -26,7 +26,12 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > +#include
> > > #include
> > > +#include
> > > +
> > > +#include "assembler/brw_compat.h"/* [un]likely() */
> > ^
> > Do we really need this?
>
> I think the correct question is if wee really need [un]likely(). I'm using
> it
> to document unlikely cases, which is a widely accepted method of documenting
> cases like that, I believe. Having that clarified, I hope you just tell me
> if
> you think we need those cases documented, and how, if not that way.
>
Now I see, it is for unlikely() - ok it can stay.
> >
> > >
> > > #include "igt_aux.h"
> > > #include "igt_core.h"
> > > @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module
> > > *kmod,
> > > }
> > >
> > > struct modprobe_data {
> > > + pthread_t parent;
> > ---
> > Please move it below to other related thread data.
> > Also consider a comment why(or for what purpose)
> > did you put this here.
>
> No problem to move it to the bottom, if that's important to you, but
> regarding
> the comment, do you think that the purpose of this field of the structure,
> compared to other fields, is so unclear form review of the code, despite its
> name, that it requires a comment, unlike the other fields?
>
I am ok with only move.
> >
> > > struct kmod_module *kmod;
> > > const char *opts;
> > > int err;
> > > + pthread_mutex_t lock;
> > > + pthread_t thread;
> > > };
> > >
> > > static void *modprobe_task(void *arg)
> > > @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
> > >
> > > data->err = modprobe(data->kmod, data->opts);
> > >
> > > + if (igt_debug_on(data->err)) {
> > > + int err;
> > > +
> > > + while (err = pthread_mutex_trylock(>lock),
> > > +err && !igt_debug_on(err != EBUSY))
> > > + igt_debug_on(pthread_kill(data->parent, SIGCHLD));
> > > + } else {
> > > + /* let main thread use mutex
Re: [Intel-gfx] [PATCH i-g-t v2 04/11] lib/kunit: Parse KTAP report from the main process thread
Hi Mauro,
Thanks for review.
On Tuesday, 10 October 2023 15:33:57 CEST Mauro Carvalho Chehab wrote:
> On Mon, 9 Oct 2023 14:27:55 +0200
> Janusz Krzysztofik wrote:
>
> > There was an attempt to parse KTAP reports in the background while a kunit
> > test module is loading. However, since dynamic sub-subtests can be
> > executed only from the main thread, that attempt was not quite successful,
> > as IGT results from all executed kunit test cases were generated only
> > after loading of kunit test module completed.
> >
> > Now that the parser maintains its state and we can call it separately for
> > each input line of a KTAP report, it is perfectly possible to call the
> > parser from the main thread while the module is loading in the background,
> > and convert results from kunit test cases immediately to results of IGT
> > dynamic sub-subtests by running an igt_dynamic() section for each result
> > as soon as returned by the parser.
> >
> > Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> > result obtained from igt_ktap_parse() called from the main thread.
> >
> > Also, drop no longer needed functions from igt_ktap soruces.
> >
> > v3: Fix ktap structure not freed on lseek error,
> > - fix initial SIGCHLD handler not restored,
> > - fix missing handling of potential errors returned by sigaction,
> > - fix potential race of read() vs. ptherad_kill(), use robust mutex for
> > synchronization with modprobe thread,
> > - fix potentially illegal use of igt_assert() called outside of
> > dynamic sub-subtest section,
> > - fix unsupported exit code potentially passed to igt_fail(),
> > - no need to fail a dynamic sub-subtest on potential KTAP parser error
> > after a valid result from the parser has been processed,
> > - fix trailing newlines missing from error messages,
> > - add more debug statements,
> > - integrate common code around kunit_result_free() into it.
> > v2: Interrupt blocking read() on modprobe failure.
> >
> > Signed-off-by: Janusz Krzysztofik
> > Acked-by: Mauro Carvalho Chehab # v2
> > ---
> > lib/igt_kmod.c | 261 +++
> > lib/igt_ktap.c | 568 -
> > lib/igt_ktap.h | 22 --
> > 3 files changed, 222 insertions(+), 629 deletions(-)
> >
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index 426ae5b26f..7bca4cdaab 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright © 2016 Intel Corporation
> > + * Copyright © 2016-2023 Intel Corporation
> > *
> > * Permission is hereby granted, free of charge, to any person obtaining a
> > * copy of this software and associated documentation files (the
> > "Software"),
> > @@ -26,7 +26,12 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > #include
> > +#include
> > +
> > +#include "assembler/brw_compat.h" /* [un]likely() */
> >
> > #include "igt_aux.h"
> > #include "igt_core.h"
> > @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> > }
> >
> > struct modprobe_data {
> > + pthread_t parent;
> > struct kmod_module *kmod;
> > const char *opts;
> > int err;
> > + pthread_mutex_t lock;
> > + pthread_t thread;
> > };
> >
> > static void *modprobe_task(void *arg)
> > @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
> >
> > data->err = modprobe(data->kmod, data->opts);
> >
> > + if (igt_debug_on(data->err)) {
> > + int err;
> > +
> > + while (err = pthread_mutex_trylock(>lock),
> > + err && !igt_debug_on(err != EBUSY))
> > + igt_debug_on(pthread_kill(data->parent, SIGCHLD));
>
> I guess you need here an "igt_debug_on_once", as it doesn't make
> sense to have a (potentially) endless loop here printing error
> messages.
Right. Since we don't have igt_debug_on_once() implemented, I'll open code
that improvement.
Thanks,
Janusz
>
> the other changes LGTM.
>
> > + } else {
> > + /* let main thread use mutex to detect modprobe completion */
> > + igt_debug_on(pthread_mutex_lock(>lock));
> > + }
> > +
> > return NULL;
> > }
> >
> > +static void kunit_sigchld_handler(int signal)
> > +{
> > + return;
> > +}
> > +
> > +static int kunit_kmsg_result_get(struct igt_list_head *results,
> > +struct modprobe_data *modprobe,
> > +int fd, struct igt_ktap_results *ktap)
> > +{
> > + struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
> > +*saved;
> > + char record[BUF_LEN + 1], *buf;
> > + unsigned long taints;
> > + int ret;
> > +
> > + do {
> > + int err;
> > +
> > + if (igt_debug_on(igt_kernel_tainted()))
> > + return -ENOTRECOVERABLE;
> > +
> > + err = igt_debug_on(sigaction(SIGCHLD, , saved));
> > + if (err == -1)
> > +
Re: [Intel-gfx] [PATCH i-g-t v2 04/11] lib/kunit: Parse KTAP report from the main process thread
Hi Kamil,
Thanks for review.
On Tuesday, 10 October 2023 17:59:56 CEST Kamil Konieczny wrote:
> Hi Janusz,
> On 2023-10-09 at 14:27:55 +0200, Janusz Krzysztofik wrote:
> > There was an attempt to parse KTAP reports in the background while a kunit
> > test module is loading. However, since dynamic sub-subtests can be
> > executed only from the main thread, that attempt was not quite successful,
> > as IGT results from all executed kunit test cases were generated only
> > after loading of kunit test module completed.
> >
> > Now that the parser maintains its state and we can call it separately for
> > each input line of a KTAP report, it is perfectly possible to call the
> > parser from the main thread while the module is loading in the background,
> > and convert results from kunit test cases immediately to results of IGT
> > dynamic sub-subtests by running an igt_dynamic() section for each result
> > as soon as returned by the parser.
> >
> > Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> > result obtained from igt_ktap_parse() called from the main thread.
> >
> > Also, drop no longer needed functions from igt_ktap soruces.
> >
> > v3: Fix ktap structure not freed on lseek error,
> > - fix initial SIGCHLD handler not restored,
> > - fix missing handling of potential errors returned by sigaction,
> > - fix potential race of read() vs. ptherad_kill(), use robust mutex for
> > synchronization with modprobe thread,
> > - fix potentially illegal use of igt_assert() called outside of
> > dynamic sub-subtest section,
> > - fix unsupported exit code potentially passed to igt_fail(),
> > - no need to fail a dynamic sub-subtest on potential KTAP parser error
> > after a valid result from the parser has been processed,
> > - fix trailing newlines missing from error messages,
> > - add more debug statements,
> > - integrate common code around kunit_result_free() into it.
> > v2: Interrupt blocking read() on modprobe failure.
> >
> > Signed-off-by: Janusz Krzysztofik
> > Acked-by: Mauro Carvalho Chehab # v2
> > ---
> > lib/igt_kmod.c | 261 +++
> > lib/igt_ktap.c | 568 -
> > lib/igt_ktap.h | 22 --
> > 3 files changed, 222 insertions(+), 629 deletions(-)
> >
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index 426ae5b26f..7bca4cdaab 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright © 2016 Intel Corporation
> > + * Copyright © 2016-2023 Intel Corporation
> > *
> > * Permission is hereby granted, free of charge, to any person obtaining a
> > * copy of this software and associated documentation files (the
> > "Software"),
> > @@ -26,7 +26,12 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > #include
> > +#include
> > +
> > +#include "assembler/brw_compat.h" /* [un]likely() */
> ^
> Do we really need this?
I think the correct question is if wee really need [un]likely(). I'm using it
to document unlikely cases, which is a widely accepted method of documenting
cases like that, I believe. Having that clarified, I hope you just tell me if
you think we need those cases documented, and how, if not that way.
>
> >
> > #include "igt_aux.h"
> > #include "igt_core.h"
> > @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> > }
> >
> > struct modprobe_data {
> > + pthread_t parent;
> ---
> Please move it below to other related thread data.
> Also consider a comment why(or for what purpose)
> did you put this here.
No problem to move it to the bottom, if that's important to you, but regarding
the comment, do you think that the purpose of this field of the structure,
compared to other fields, is so unclear form review of the code, despite its
name, that it requires a comment, unlike the other fields?
>
> > struct kmod_module *kmod;
> > const char *opts;
> > int err;
> > + pthread_mutex_t lock;
> > + pthread_t thread;
> > };
> >
> > static void *modprobe_task(void *arg)
> > @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
> >
> > data->err = modprobe(data->kmod, data->opts);
> >
> > + if (igt_debug_on(data->err)) {
> > + int err;
> > +
> > + while (err = pthread_mutex_trylock(>lock),
> > + err && !igt_debug_on(err != EBUSY))
> > + igt_debug_on(pthread_kill(data->parent, SIGCHLD));
> > + } else {
> > + /* let main thread use mutex to detect modprobe completion */
> > + igt_debug_on(pthread_mutex_lock(>lock));
> > + }
> > +
> > return NULL;
> > }
> >
> > +static void kunit_sigchld_handler(int signal)
> > +{
> > + return;
> --- ^^^
> Why not removing this? checkpatch complains about return from void.
OK.
>
> > +}
> > +
> > +static int kunit_kmsg_result_get(struct igt_list_head
Re: [Intel-gfx] [PATCH i-g-t v2 04/11] lib/kunit: Parse KTAP report from the main process thread
Hi Janusz,
On 2023-10-09 at 14:27:55 +0200, Janusz Krzysztofik wrote:
> There was an attempt to parse KTAP reports in the background while a kunit
> test module is loading. However, since dynamic sub-subtests can be
> executed only from the main thread, that attempt was not quite successful,
> as IGT results from all executed kunit test cases were generated only
> after loading of kunit test module completed.
>
> Now that the parser maintains its state and we can call it separately for
> each input line of a KTAP report, it is perfectly possible to call the
> parser from the main thread while the module is loading in the background,
> and convert results from kunit test cases immediately to results of IGT
> dynamic sub-subtests by running an igt_dynamic() section for each result
> as soon as returned by the parser.
>
> Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> result obtained from igt_ktap_parse() called from the main thread.
>
> Also, drop no longer needed functions from igt_ktap soruces.
>
> v3: Fix ktap structure not freed on lseek error,
> - fix initial SIGCHLD handler not restored,
> - fix missing handling of potential errors returned by sigaction,
> - fix potential race of read() vs. ptherad_kill(), use robust mutex for
> synchronization with modprobe thread,
> - fix potentially illegal use of igt_assert() called outside of
> dynamic sub-subtest section,
> - fix unsupported exit code potentially passed to igt_fail(),
> - no need to fail a dynamic sub-subtest on potential KTAP parser error
> after a valid result from the parser has been processed,
> - fix trailing newlines missing from error messages,
> - add more debug statements,
> - integrate common code around kunit_result_free() into it.
> v2: Interrupt blocking read() on modprobe failure.
>
> Signed-off-by: Janusz Krzysztofik
> Acked-by: Mauro Carvalho Chehab # v2
> ---
> lib/igt_kmod.c | 261 +++
> lib/igt_ktap.c | 568 -
> lib/igt_ktap.h | 22 --
> 3 files changed, 222 insertions(+), 629 deletions(-)
>
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 426ae5b26f..7bca4cdaab 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright © 2016 Intel Corporation
> + * Copyright © 2016-2023 Intel Corporation
> *
> * Permission is hereby granted, free of charge, to any person obtaining a
> * copy of this software and associated documentation files (the "Software"),
> @@ -26,7 +26,12 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> +#include
> +
> +#include "assembler/brw_compat.h"/* [un]likely() */
^
Do we really need this?
>
> #include "igt_aux.h"
> #include "igt_core.h"
> @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> }
>
> struct modprobe_data {
> + pthread_t parent;
---
Please move it below to other related thread data.
Also consider a comment why(or for what purpose)
did you put this here.
> struct kmod_module *kmod;
> const char *opts;
> int err;
> + pthread_mutex_t lock;
> + pthread_t thread;
> };
>
> static void *modprobe_task(void *arg)
> @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
>
> data->err = modprobe(data->kmod, data->opts);
>
> + if (igt_debug_on(data->err)) {
> + int err;
> +
> + while (err = pthread_mutex_trylock(>lock),
> +err && !igt_debug_on(err != EBUSY))
> + igt_debug_on(pthread_kill(data->parent, SIGCHLD));
> + } else {
> + /* let main thread use mutex to detect modprobe completion */
> + igt_debug_on(pthread_mutex_lock(>lock));
> + }
> +
> return NULL;
> }
>
> +static void kunit_sigchld_handler(int signal)
> +{
> + return;
--- ^^^
Why not removing this? checkpatch complains about return from void.
> +}
> +
> +static int kunit_kmsg_result_get(struct igt_list_head *results,
> + struct modprobe_data *modprobe,
> + int fd, struct igt_ktap_results *ktap)
> +{
> + struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
> + *saved;
> + char record[BUF_LEN + 1], *buf;
> + unsigned long taints;
> + int ret;
> +
> + do {
> + int err;
> +
> + if (igt_debug_on(igt_kernel_tainted()))
> + return -ENOTRECOVERABLE;
> +
> + err = igt_debug_on(sigaction(SIGCHLD, , saved));
> + if (err == -1)
> + return -errno;
> + else if (unlikely(err))
> + return err;
> +
> + err = pthread_mutex_lock(>lock);
> + switch (err) {
> + case EOWNERDEAD:
> + /* leave the mutex unrecoverable */
> +
Re: [Intel-gfx] [PATCH i-g-t v2 04/11] lib/kunit: Parse KTAP report from the main process thread
On Mon, 9 Oct 2023 14:27:55 +0200
Janusz Krzysztofik wrote:
> There was an attempt to parse KTAP reports in the background while a kunit
> test module is loading. However, since dynamic sub-subtests can be
> executed only from the main thread, that attempt was not quite successful,
> as IGT results from all executed kunit test cases were generated only
> after loading of kunit test module completed.
>
> Now that the parser maintains its state and we can call it separately for
> each input line of a KTAP report, it is perfectly possible to call the
> parser from the main thread while the module is loading in the background,
> and convert results from kunit test cases immediately to results of IGT
> dynamic sub-subtests by running an igt_dynamic() section for each result
> as soon as returned by the parser.
>
> Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
> result obtained from igt_ktap_parse() called from the main thread.
>
> Also, drop no longer needed functions from igt_ktap soruces.
>
> v3: Fix ktap structure not freed on lseek error,
> - fix initial SIGCHLD handler not restored,
> - fix missing handling of potential errors returned by sigaction,
> - fix potential race of read() vs. ptherad_kill(), use robust mutex for
> synchronization with modprobe thread,
> - fix potentially illegal use of igt_assert() called outside of
> dynamic sub-subtest section,
> - fix unsupported exit code potentially passed to igt_fail(),
> - no need to fail a dynamic sub-subtest on potential KTAP parser error
> after a valid result from the parser has been processed,
> - fix trailing newlines missing from error messages,
> - add more debug statements,
> - integrate common code around kunit_result_free() into it.
> v2: Interrupt blocking read() on modprobe failure.
>
> Signed-off-by: Janusz Krzysztofik
> Acked-by: Mauro Carvalho Chehab # v2
> ---
> lib/igt_kmod.c | 261 +++
> lib/igt_ktap.c | 568 -
> lib/igt_ktap.h | 22 --
> 3 files changed, 222 insertions(+), 629 deletions(-)
>
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 426ae5b26f..7bca4cdaab 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright © 2016 Intel Corporation
> + * Copyright © 2016-2023 Intel Corporation
> *
> * Permission is hereby granted, free of charge, to any person obtaining a
> * copy of this software and associated documentation files (the "Software"),
> @@ -26,7 +26,12 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> +#include
> +
> +#include "assembler/brw_compat.h"/* [un]likely() */
>
> #include "igt_aux.h"
> #include "igt_core.h"
> @@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> }
>
> struct modprobe_data {
> + pthread_t parent;
> struct kmod_module *kmod;
> const char *opts;
> int err;
> + pthread_mutex_t lock;
> + pthread_t thread;
> };
>
> static void *modprobe_task(void *arg)
> @@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
>
> data->err = modprobe(data->kmod, data->opts);
>
> + if (igt_debug_on(data->err)) {
> + int err;
> +
> + while (err = pthread_mutex_trylock(>lock),
> +err && !igt_debug_on(err != EBUSY))
> + igt_debug_on(pthread_kill(data->parent, SIGCHLD));
I guess you need here an "igt_debug_on_once", as it doesn't make
sense to have a (potentially) endless loop here printing error
messages.
the other changes LGTM.
> + } else {
> + /* let main thread use mutex to detect modprobe completion */
> + igt_debug_on(pthread_mutex_lock(>lock));
> + }
> +
> return NULL;
> }
>
> +static void kunit_sigchld_handler(int signal)
> +{
> + return;
> +}
> +
> +static int kunit_kmsg_result_get(struct igt_list_head *results,
> + struct modprobe_data *modprobe,
> + int fd, struct igt_ktap_results *ktap)
> +{
> + struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
> + *saved;
> + char record[BUF_LEN + 1], *buf;
> + unsigned long taints;
> + int ret;
> +
> + do {
> + int err;
> +
> + if (igt_debug_on(igt_kernel_tainted()))
> + return -ENOTRECOVERABLE;
> +
> + err = igt_debug_on(sigaction(SIGCHLD, , saved));
> + if (err == -1)
> + return -errno;
> + else if (unlikely(err))
> + return err;
> +
> + err = pthread_mutex_lock(>lock);
> + switch (err) {
> + case EOWNERDEAD:
> + /* leave the mutex unrecoverable */
> + igt_debug_on(pthread_mutex_unlock(>lock));
> + __attribute__ ((fallthrough));
> +
[Intel-gfx] [PATCH i-g-t v2 04/11] lib/kunit: Parse KTAP report from the main process thread
There was an attempt to parse KTAP reports in the background while a kunit
test module is loading. However, since dynamic sub-subtests can be
executed only from the main thread, that attempt was not quite successful,
as IGT results from all executed kunit test cases were generated only
after loading of kunit test module completed.
Now that the parser maintains its state and we can call it separately for
each input line of a KTAP report, it is perfectly possible to call the
parser from the main thread while the module is loading in the background,
and convert results from kunit test cases immediately to results of IGT
dynamic sub-subtests by running an igt_dynamic() section for each result
as soon as returned by the parser.
Drop igt_ktap_parser() thread and execute igt_dynamic() for each kunit
result obtained from igt_ktap_parse() called from the main thread.
Also, drop no longer needed functions from igt_ktap soruces.
v3: Fix ktap structure not freed on lseek error,
- fix initial SIGCHLD handler not restored,
- fix missing handling of potential errors returned by sigaction,
- fix potential race of read() vs. ptherad_kill(), use robust mutex for
synchronization with modprobe thread,
- fix potentially illegal use of igt_assert() called outside of
dynamic sub-subtest section,
- fix unsupported exit code potentially passed to igt_fail(),
- no need to fail a dynamic sub-subtest on potential KTAP parser error
after a valid result from the parser has been processed,
- fix trailing newlines missing from error messages,
- add more debug statements,
- integrate common code around kunit_result_free() into it.
v2: Interrupt blocking read() on modprobe failure.
Signed-off-by: Janusz Krzysztofik
Acked-by: Mauro Carvalho Chehab # v2
---
lib/igt_kmod.c | 261 +++
lib/igt_ktap.c | 568 -
lib/igt_ktap.h | 22 --
3 files changed, 222 insertions(+), 629 deletions(-)
diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 426ae5b26f..7bca4cdaab 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -1,5 +1,5 @@
/*
- * Copyright © 2016 Intel Corporation
+ * Copyright © 2016-2023 Intel Corporation
*
* Permission is hereby granted, free of charge, to any person obtaining a
* copy of this software and associated documentation files (the "Software"),
@@ -26,7 +26,12 @@
#include
#include
#include
+#include
+#include
#include
+#include
+
+#include "assembler/brw_compat.h" /* [un]likely() */
#include "igt_aux.h"
#include "igt_core.h"
@@ -748,9 +753,12 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
}
struct modprobe_data {
+ pthread_t parent;
struct kmod_module *kmod;
const char *opts;
int err;
+ pthread_mutex_t lock;
+ pthread_t thread;
};
static void *modprobe_task(void *arg)
@@ -759,16 +767,132 @@ static void *modprobe_task(void *arg)
data->err = modprobe(data->kmod, data->opts);
+ if (igt_debug_on(data->err)) {
+ int err;
+
+ while (err = pthread_mutex_trylock(>lock),
+ err && !igt_debug_on(err != EBUSY))
+ igt_debug_on(pthread_kill(data->parent, SIGCHLD));
+ } else {
+ /* let main thread use mutex to detect modprobe completion */
+ igt_debug_on(pthread_mutex_lock(>lock));
+ }
+
return NULL;
}
+static void kunit_sigchld_handler(int signal)
+{
+ return;
+}
+
+static int kunit_kmsg_result_get(struct igt_list_head *results,
+struct modprobe_data *modprobe,
+int fd, struct igt_ktap_results *ktap)
+{
+ struct sigaction sigchld = { .sa_handler = kunit_sigchld_handler, },
+*saved;
+ char record[BUF_LEN + 1], *buf;
+ unsigned long taints;
+ int ret;
+
+ do {
+ int err;
+
+ if (igt_debug_on(igt_kernel_tainted()))
+ return -ENOTRECOVERABLE;
+
+ err = igt_debug_on(sigaction(SIGCHLD, , saved));
+ if (err == -1)
+ return -errno;
+ else if (unlikely(err))
+ return err;
+
+ err = pthread_mutex_lock(>lock);
+ switch (err) {
+ case EOWNERDEAD:
+ /* leave the mutex unrecoverable */
+ igt_debug_on(pthread_mutex_unlock(>lock));
+ __attribute__ ((fallthrough));
+ case ENOTRECOVERABLE:
+ igt_debug_on(sigaction(SIGCHLD, saved, NULL));
+ if (igt_debug_on(modprobe->err))
+ return modprobe->err;
+ break;
+ case 0:
+ break;
+ default:
+ igt_debug("pthread_mutex_lock() error: %d\n", err);
+
