[igt-dev] [PATCH i-g-t 5/5] Change logic of ktap parser to run on a thread
Piatkowski, Dominik Karol
dominik.karol.piatkowski at intel.com
Fri Mar 24 08:11:09 UTC 2023
> -----Original Message-----
> From: Mauro Carvalho Chehab <mauro.chehab at linux.intel.com>
> Sent: Wednesday, March 15, 2023 13:49
> To: Piatkowski, Dominik Karol <dominik.karol.piatkowski at intel.com>
> Cc: igt-dev at lists.freedesktop.org; Janusz Krzysztofik
> <janusz.krzysztofik at linux.intel.com>
> Subject: Re: [PATCH i-g-t 5/5] Change logic of ktap parser to run on a thread
>
> On Wed, 15 Mar 2023 10:40:42 +0100
> Dominik Karol Piatkowski <dominik.karol.piatkowski at intel.com> wrote:
>
> > The ktap parser should be listening and parsing messages as the tests
> > are executed, and not after the end of module load.
> >
> > 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 | 12 +++--
> > lib/igt_ktap.c | 137 +++++++++++++++++++++++++++++++------------------
> > lib/igt_ktap.h | 3 +-
> > 3 files changed, 97 insertions(+), 55 deletions(-)
> >
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c index 3b92cc94..23b92742
> > 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;
> > @@ -804,19 +803,24 @@ 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);
> > }
> >
> > - ret = igt_ktap_parser(f, record, is_builtin);
> > - if (ret != 0)
> > - ret = IGT_EXIT_ABORT;
> > unload:
> > 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..acbb76ba
> > 100644
> > --- a/lib/igt_ktap.c
> > +++ b/lib/igt_ktap.c
> > @@ -10,6 +10,10 @@
> > #include "igt_core.h"
> > #include "igt_ktap.h"
> >
> > +#include <libkmod.h>
> > +
>
>
> > +#include <pthread.h>
>
> Hmm... IGT has some pthread handlers. You should use them.
I could not find them, but I found out that there are some tests in this repository that use pthread.h directly. If there is a particular handler that you suggest to use, I will happily look into that.
>
>
> > +
> > static int log_to_end(enum igt_log_level level, FILE *f,
> > char *record, const char *format, ...)
> > __attribute__((format(printf, 4, 5)));
> >
> > @@ -253,6 +257,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 +275,97 @@ 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)
>
> Nitpick: as we use Kernel coding style, it should be:
>
> 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';
> > +
> > + while (fgets(record, BUF_LEN, fp) != NULL) {
> > + if (!ktap_args.is_running)
> > + return NULL;
> > + usleep(1000);
> > }
>
> Please don't add sleep, as it might lose messages on crashes.
>
> You should probably check ferror() and/or feof() if it returns NULL, as it can
> indicate either:
>
> - that modprobe ended - so, feof() would be nonzero;
> - some error happened - so, ferror() would be nonzero.
> On such case, it should likely print the error.
>
> Also, don't call usleep(). If fp was opened on sync mode (usually the default),
> usleep is not needed at all, as fgets() will block until an error or read.
>
> Otherwise, if fp is in async mode, you can switch to sync mode with:
>
> s = fcntl(*fd, F_GETFL);
> s |= O_SYNC;
> result = fcntl(*fd, F_SETFL, s);
>
> and avoid the usleep().
>
> Btw, you probably need to add error check logic too. Maybe, you could do
> something like (untested):
>
> while (!feof(record)) {
> if (fgets(record, BUF_LEN, fp) == NULL) {
> if (ferror(record))
> igt_warn("klog read failed: %s\n",
> strerror(errno));
>
> if (!ktap_args.is_running)
> return NULL;
> }
>
> here and the calls to fgets() below.
>
> > - 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:
> > - if (fgets(record, BUF_LEN, fp) == NULL) {
> > - igt_warn("kmsg truncated: unknown error
> (%m)\n");
> > - return -2;
> > + 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) {
> > + igt_warn("kmsg truncated: unknown
> error (%m)\n");
> > + goto igt_ktap_parser_start;
> > + }
> > + found_tests = true;
> > + sublevel++;
> > + break;
> > }
> > - 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;
> > + 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)
> > + 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;
> > + }
> > }
> > }
> >
> > 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;
> >
> > - return IGT_EXIT_SUCCESS;
> > +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); }
> > +
> > +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..461df6f5
> > 100644
> > --- a/lib/igt_ktap.h
> > +++ b/lib/igt_ktap.h
> > @@ -26,6 +26,7 @@
> >
> > #define BUF_LEN 4096
> >
> > -int igt_ktap_parser(FILE *fp, char *record, bool is_builtin);
> > +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