Re: [Intel-gfx] [PATCH i-g-t v2 12/17] lib/ktap: Use IGT linked lists for storing KTAP results
On Fri, 8 Sep 2023 14:32:46 +0200
Janusz Krzysztofik wrote:
> For code simplicity and clarity, use existing IGT linked lists library
> instead of open coding a custom implementation of a list of KTAP results.
>
> While being at it, flatten the code by inverting a check for pending
> results.
LGTM.
Acked-by: Mauro Carvalho Chehab
>
> Signed-off-by: Janusz Krzysztofik
> ---
> lib/igt_kmod.c | 28
> lib/igt_ktap.c | 25 +
> lib/igt_ktap.h | 6 --
> 3 files changed, 25 insertions(+), 34 deletions(-)
>
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 988ac164cb..c692954911 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -760,7 +760,6 @@ static void __igt_kunit(struct igt_ktest *tst, const char
> *opts)
> struct kmod_module *kunit_kmod;
> bool is_builtin;
> struct ktap_test_results *results;
> - struct ktap_test_results_element *temp;
> unsigned long taints;
> int flags, ret;
>
> @@ -784,28 +783,33 @@ static void __igt_kunit(struct igt_ktest *tst, const
> char *opts)
> igt_skip("Unable to load %s module\n", tst->module_name);
> }
>
> - while (READ_ONCE(results->still_running) || READ_ONCE(results->head) !=
> NULL)
> + while (READ_ONCE(results->still_running) ||
> !igt_list_empty(&results->list))
> {
> + struct ktap_test_results_element *result;
> +
> if (igt_kernel_tainted(&taints)) {
> ktap_parser_cancel();
> break;
> }
>
> - if (READ_ONCE(results->head) != NULL) {
> - pthread_mutex_lock(&results->mutex);
> + pthread_mutex_lock(&results->mutex);
> + if (igt_list_empty(&results->list)) {
> + pthread_mutex_unlock(&results->mutex);
> + continue;
> + }
>
> - igt_dynamic(results->head->test_name) {
> - igt_assert(READ_ONCE(results->head->passed));
> + result = igt_list_first_entry(&results->list, result, link);
>
> - igt_fail_on(igt_kernel_tainted(&taints));
> - }
> + igt_list_del(&result->link);
> + pthread_mutex_unlock(&results->mutex);
>
> - temp = results->head;
> - results->head = results->head->next;
> - free(temp);
> + igt_dynamic(result->test_name) {
> + igt_assert(READ_ONCE(result->passed));
>
> - pthread_mutex_unlock(&results->mutex);
> + igt_fail_on(igt_kernel_tainted(&taints));
> }
> +
> + free(result);
> }
>
> ret = ktap_parser_stop();
> diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> index 165f6b2cce..5e9967f980 100644
> --- a/lib/igt_ktap.c
> +++ b/lib/igt_ktap.c
> @@ -12,6 +12,7 @@
> #include "igt_aux.h"
> #include "igt_core.h"
> #include "igt_ktap.h"
> +#include "igt_list.h"
>
> #define DELIMITER "-"
>
> @@ -335,7 +336,7 @@ static int parse_tap_level(int fd, char *base_test_name,
> int test_count, bool *f
> bool *found_tests, bool is_builtin)
> {
> char record[BUF_LEN + 1];
> - struct ktap_test_results_element *r, *temp;
> + struct ktap_test_results_element *r;
> int internal_test_count;
> char test_name[BUF_LEN + 1];
> char base_test_name_for_next_level[BUF_LEN + 1];
> @@ -403,17 +404,9 @@ static int parse_tap_level(int fd, char *base_test_name,
> int test_count, bool *f
> r->test_name[BUF_LEN] = '\0';
>
> r->passed = false;
> - r->next = NULL;
>
> pthread_mutex_lock(&results.mutex);
> - if (results.head == NULL) {
> - results.head = r;
> - } else {
> - temp = results.head;
> - while (temp->next != NULL)
> - temp = temp->next;
> - temp->next = r;
> - }
> + igt_list_add_tail(&r->link, &results.list);
> pthread_mutex_unlock(&results.mutex);
>
> test_name[0] = '\0';
> @@ -431,17 +424,9 @@ static int parse_tap_level(int fd, char *base_test_name,
> int test_count, bool *f
> r->test_name[BUF_LEN] = '\0';
>
> r->passed = true;
> - r->next = NULL;
>
> pthread_mutex_lock(&results.mutex);
> - if (results.head == NULL) {
> - results.head = r;
> - } else {
> - temp = results.head;
> - while (temp->next != NULL)
> -
[Intel-gfx] [PATCH i-g-t v2 12/17] lib/ktap: Use IGT linked lists for storing KTAP results
For code simplicity and clarity, use existing IGT linked lists library
instead of open coding a custom implementation of a list of KTAP results.
While being at it, flatten the code by inverting a check for pending
results.
Signed-off-by: Janusz Krzysztofik
---
lib/igt_kmod.c | 28
lib/igt_ktap.c | 25 +
lib/igt_ktap.h | 6 --
3 files changed, 25 insertions(+), 34 deletions(-)
diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 988ac164cb..c692954911 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -760,7 +760,6 @@ static void __igt_kunit(struct igt_ktest *tst, const char
*opts)
struct kmod_module *kunit_kmod;
bool is_builtin;
struct ktap_test_results *results;
- struct ktap_test_results_element *temp;
unsigned long taints;
int flags, ret;
@@ -784,28 +783,33 @@ static void __igt_kunit(struct igt_ktest *tst, const char
*opts)
igt_skip("Unable to load %s module\n", tst->module_name);
}
- while (READ_ONCE(results->still_running) || READ_ONCE(results->head) !=
NULL)
+ while (READ_ONCE(results->still_running) ||
!igt_list_empty(&results->list))
{
+ struct ktap_test_results_element *result;
+
if (igt_kernel_tainted(&taints)) {
ktap_parser_cancel();
break;
}
- if (READ_ONCE(results->head) != NULL) {
- pthread_mutex_lock(&results->mutex);
+ pthread_mutex_lock(&results->mutex);
+ if (igt_list_empty(&results->list)) {
+ pthread_mutex_unlock(&results->mutex);
+ continue;
+ }
- igt_dynamic(results->head->test_name) {
- igt_assert(READ_ONCE(results->head->passed));
+ result = igt_list_first_entry(&results->list, result, link);
- igt_fail_on(igt_kernel_tainted(&taints));
- }
+ igt_list_del(&result->link);
+ pthread_mutex_unlock(&results->mutex);
- temp = results->head;
- results->head = results->head->next;
- free(temp);
+ igt_dynamic(result->test_name) {
+ igt_assert(READ_ONCE(result->passed));
- pthread_mutex_unlock(&results->mutex);
+ igt_fail_on(igt_kernel_tainted(&taints));
}
+
+ free(result);
}
ret = ktap_parser_stop();
diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
index 165f6b2cce..5e9967f980 100644
--- a/lib/igt_ktap.c
+++ b/lib/igt_ktap.c
@@ -12,6 +12,7 @@
#include "igt_aux.h"
#include "igt_core.h"
#include "igt_ktap.h"
+#include "igt_list.h"
#define DELIMITER "-"
@@ -335,7 +336,7 @@ static int parse_tap_level(int fd, char *base_test_name,
int test_count, bool *f
bool *found_tests, bool is_builtin)
{
char record[BUF_LEN + 1];
- struct ktap_test_results_element *r, *temp;
+ struct ktap_test_results_element *r;
int internal_test_count;
char test_name[BUF_LEN + 1];
char base_test_name_for_next_level[BUF_LEN + 1];
@@ -403,17 +404,9 @@ static int parse_tap_level(int fd, char *base_test_name,
int test_count, bool *f
r->test_name[BUF_LEN] = '\0';
r->passed = false;
- r->next = NULL;
pthread_mutex_lock(&results.mutex);
- if (results.head == NULL) {
- results.head = r;
- } else {
- temp = results.head;
- while (temp->next != NULL)
- temp = temp->next;
- temp->next = r;
- }
+ igt_list_add_tail(&r->link, &results.list);
pthread_mutex_unlock(&results.mutex);
test_name[0] = '\0';
@@ -431,17 +424,9 @@ static int parse_tap_level(int fd, char *base_test_name,
int test_count, bool *f
r->test_name[BUF_LEN] = '\0';
r->passed = true;
- r->next = NULL;
pthread_mutex_lock(&results.mutex);
- if (results.head == NULL) {
- results.head = r;
- } else {
- temp = results.head;
- while (temp->next != NULL)
- temp = temp->next;
- temp->next = r;
- }
+ igt_list_add_tail(&r->link, &results.list);
pthr
