[igt-dev] [PATCH i-g-t 5/5] Change logic of ktap parser to run on a thread
Janusz Krzysztofik
janusz.krzysztofik at linux.intel.com
Fri Apr 7 12:53:15 UTC 2023
Hi Dominik,
I can see some progress, but still quite a few issues.
If you and/or other reviewers think my concerns are not important from the PoV
of Xe validation requirements then please feel free to ignore my comments.
On Friday, 24 March 2023 09:11:20 CEST Dominik Karol Piatkowski wrote:
> The ktap parser should be listening and parsing messages as the tests
> are executed, and not after the end of module load.
>
> v1 -> v2:
> - fix coding style
> - remove usleep
> - add error check logic
> - follow the structure of igt_kselftests more closely
>
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski at intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab at linux.intel.com>
> ---
> lib/igt_kmod.c | 26 +++++----
> lib/igt_ktap.c | 148 ++++++++++++++++++++++++++++++++-----------------
> lib/igt_ktap.h | 5 +-
> 3 files changed, 118 insertions(+), 61 deletions(-)
>
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 21e801bd..45a0c6ed 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -758,7 +758,6 @@ int igt_kunit(const char *module_name, const char *opts)
> {
> struct igt_ktest tst;
> struct kmod_module *kunit_kmod;
> - char record[BUF_LEN + 1];
> FILE *f;
> bool is_builtin;
> int ret;
> @@ -771,12 +770,13 @@ int igt_kunit(const char *module_name, const char *opts)
> return ret;
> }
>
> - if (igt_ktest_begin(&tst) != 0) {
> - igt_warn("Unable to begin ktest for %s\n", module_name);
> + igt_fixture
> + if (igt_ktest_begin(&tst) != 0) {
> + igt_warn("Unable to begin ktest for %s\n", module_name);
>
> - igt_ktest_fini(&tst);
> - return ret;
> - }
> + igt_ktest_fini(&tst);
> + return ret;
> + }
Please note that since igt_require() is still called inside igt_ktest_begin()
and can potentially long jump out of your igt_fixture section body, your
attempt to return an error code from igt_kunit() if some requirements are not
met can still fail. If that happens, igt_kunit() will proceed, though all
following igt_fixture and igt_subtest section bodies will be omitted.
Moreover, omitted not only inside igt_kunit(), but also outside of it, after
it returns to its caller, until the program exits. I believe that's not what
we intend.
>
> if (tst.kmsg < 0) {
> igt_warn("Could not open /dev/kmsg\n");
> @@ -804,19 +804,25 @@ int igt_kunit(const char *module_name, const char *opts)
>
> is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
>
> + ktap_parser_start(f, is_builtin);
> +
> if (igt_kmod_load(module_name, opts) != 0) {
> igt_warn("Unable to load %s module\n", module_name);
> + ret = ktap_parser_stop();
> igt_fail(IGT_EXIT_FAILURE);
This igt_fail() (and not only this one) still seems to be called from outside
of any igt_fixture / igt_subtest sections.
> }
>
> - ret = igt_ktap_parser(f, record, is_builtin);
> - if (ret != 0)
> - ret = IGT_EXIT_ABORT;
> unload:
> - igt_ktest_end(&tst);
> + igt_fixture
> + igt_ktest_end(&tst);
>
> igt_ktest_fini(&tst);
>
> + ret = ktap_parser_stop();
> +
> + if (ret != 0)
> + ret = IGT_EXIT_ABORT;
> +
> if (ret == 0)
> igt_success();
>
> diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> index 117598fa..090f98b7 100644
> --- a/lib/igt_ktap.c
> +++ b/lib/igt_ktap.c
> @@ -5,11 +5,16 @@
>
> #include <ctype.h>
> #include <limits.h>
> +#include <libkmod.h>
> +#include <pthread.h>
> +#include <errno.h>
>
> #include "igt_aux.h"
> #include "igt_core.h"
> #include "igt_ktap.h"
>
> +
> +
> static int log_to_end(enum igt_log_level level, FILE *f,
> char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
>
> @@ -118,7 +123,10 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
> igt_info("%s", version_rptr);
>
> if (fgets(record, BUF_LEN, fp) == NULL) {
As Petri suggested in his comment to one of previous versions of the series,
using line buffered input is not needed for reading from /dev/kmesg (which I
didn't know when I suggested to switch to it before, still when Isabella was
working on this -- my fault, sorry). Before you decide whether to keep using
line buffered input or switch to raw read(), please have a look at
lib/igt_kmod.c:kmsg_dump() for an example of how we've been reading log
records from /dev/kmsg so far, and at commit 98783313b8b3 ("lib/kmod:
reimplement kmsg_dump()") for some justification.
Some of below comments apply to current line buffered reading, if you decide
to keep it.
> - igt_warn("kmsg truncated: unknown error (%m)\n");
> + if (ferror(fp))
> + igt_warn("kmsg read failed: %s\n", strerror(errno));
> + else
> + igt_warn("kmsg truncated: unknown error (%m)\n");
> return -2;
> }
> }
> @@ -253,6 +261,13 @@ static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
> return 1;
> }
>
> +struct ktap_parser_args {
> + FILE *fp;
> + bool is_builtin;
> + volatile bool is_running;
> + int ret;
> +} ktap_args;
> +
> /**
> * igt_ktap_parser:
> * @fp: FILE pointer
> @@ -264,71 +279,104 @@ static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
> *
> * Returns: IGT default codes
> */
> -int igt_ktap_parser(FILE *fp, char *record, bool is_builtin)
> +void *igt_ktap_parser(void *unused)
> {
> + FILE *fp = ktap_args.fp;
> + char record[BUF_LEN + 1];
> + bool is_builtin = ktap_args.is_builtin;
> +
> char test_name[BUF_LEN + 1];
> bool failed_tests, found_tests;
> int sublevel = 0;
>
> - test_name[0] = '\0';
> - test_name[BUF_LEN] = '\0';
> -
> failed_tests = false;
> found_tests = false;
>
> - while (sublevel >= 0) {
> - if (fgets(record, BUF_LEN, fp) == NULL) {
> - if (!found_tests)
> - igt_warn("kmsg truncated: unknown error (%m)\n");
> - break;
> - }
> +igt_ktap_parser_start:
> + while (ktap_args.is_running) {
> + test_name[0] = '\0';
> + test_name[BUF_LEN] = '\0';
>
> - switch (find_next_tap_subtest(fp, record, is_builtin)) {
> - case -2:
> - /* no more data to read */
> - return IGT_EXIT_FAILURE;
> - case -1:
> - /* no test found, so we keep parsing */
> - break;
> - case 0:
> - /*
> - * tests found, but they're missing info, so we might
> - * have read into test output
> - */
> - found_tests = true;
> - sublevel++;
> - break;
> - default:
> + while(!feof(fp)) {
Since we are reading from /dev/kmsg, which is a special character device file,
I think we never get EOF.
> if (fgets(record, BUF_LEN, fp) == NULL) {
> - igt_warn("kmsg truncated: unknown error (%m)\n");
> - return -2;
> + if (ferror(fp))
> + igt_warn("kmsg read failed: %s\n", strerror(errno));
Moreover, I think we always get an error if no data is ready for reading.
Then we will get a lot of noise from here since we are using non-blocking
read. I think we should try to use blocking mode, or switch to poll() or an
equivalent, whichever works best for reading from a character device special
file.
> +
> + if (!ktap_args.is_running)
> + return NULL;
> }
> - found_tests = true;
> - sublevel++;
> - break;
> }
>
> - switch (parse_kmsg_for_tap(fp, record, test_name)) {
> - case -2:
> - return IGT_EXIT_FAILURE;
> - case -1:
> - sublevel--;
> - failed_tests = true;
> - igt_subtest(test_name)
> - igt_fail(IGT_EXIT_FAILURE);
> - test_name[0] = '\0';
> - break;
> - case 0: /* fallthrough */
> - igt_subtest(test_name)
> - igt_success();
> - test_name[0] = '\0';
> - default:
> - break;
> + while (sublevel >= 0) {
> + switch (find_next_tap_subtest(fp, record, is_builtin)) {
> + case -2:
> + /* no more data to read */
> + goto igt_ktap_parser_start;
> + case -1:
> + /* no test found, so we keep parsing */
> + break;
> + case 0:
> + /*
> + * tests found, but they're missing info, so we might
> + * have read into test output
> + */
> + found_tests = true;
> + sublevel++;
> + break;
> + default:
> + if (fgets(record, BUF_LEN, fp) == NULL) {
> + if (ferror(fp))
> + igt_warn("kmsg read failed: %s\n", strerror(errno));
> + else
> + igt_warn("kmsg truncated: unknown error (%m)\n");
> + goto igt_ktap_parser_start;
> + }
> + found_tests = true;
> + sublevel++;
> + break;
> + }
> +
> + switch (parse_kmsg_for_tap(fp, record, test_name)) {
> + case -2:
> + goto igt_ktap_parser_start;
> + case -1:
> + sublevel--;
> + failed_tests = true;
> + igt_subtest(test_name)
AFAICT, igt_subtest sections can be used only in the main thread. I think
that if we are going to parse the kernel log for ktap messages from a sub-
thread then we need to establish a communication channel for passing progress
to the main thread. The main thread in turn should enter an igt_subtest
section when start of a new subtest is reported bu the ktap parser, then wait
for the subtest result to be reported and either leave the section on success
or igt_fail() on failure.
> + igt_fail(IGT_EXIT_FAILURE);
> + test_name[0] = '\0';
> + break;
> + case 0: /* fallthrough */
> + igt_subtest(test_name)
> + igt_success();
> + test_name[0] = '\0';
> + default:
> + break;
> + }
> }
While ktap_args.is_running can help to force the parser to exit if something
goes wrong, I think we should be able to leave it when we detect end of ktap
data (sublevel < 0 ?).
Thanks,
Janusz
> }
>
> if (failed_tests || !found_tests)
> - return IGT_EXIT_FAILURE;
> + ktap_args.ret = IGT_EXIT_FAILURE;
> + else
> + ktap_args.ret = IGT_EXIT_SUCCESS;
> +
> + return NULL;
> +}
> +
> +static pthread_t ktap_parser_thread;
> +
> +void ktap_parser_start(FILE *fp, bool is_builtin)
> +{
> + ktap_args.fp = fp;
> + ktap_args.is_builtin = is_builtin;
> + ktap_args.is_running = true;
> + pthread_create(&ktap_parser_thread, NULL, igt_ktap_parser, NULL);
> +}
>
> - return IGT_EXIT_SUCCESS;
> +int ktap_parser_stop(void)
> +{
> + ktap_args.is_running = false;
> + pthread_join(ktap_parser_thread, NULL);
> + return ktap_args.ret;
> }
> diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> index b2f69df2..ee6689c1 100644
> --- a/lib/igt_ktap.h
> +++ b/lib/igt_ktap.h
> @@ -26,6 +26,9 @@
>
> #define BUF_LEN 4096
>
> -int igt_ktap_parser(FILE *fp, char *record, bool is_builtin);
> +void *igt_ktap_parser(void *unused);
> +
> +void ktap_parser_start(FILE *fp, bool is_builtin);
> +int ktap_parser_stop(void);
>
> #endif /* IGT_KTAP_H */
>
More information about the igt-dev
mailing list