All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH i-g-t 1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs
@ 2021-01-26 13:05 ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-26 13:05 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev, Chris Wilson

An earlier client from an old test may still be lingering and disappear
as we scan the sysfs. Be graceful and let it go without tripping over it.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/sysfs_clients.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
index 6be52c04f..a3a1f81e1 100644
--- a/tests/i915/sysfs_clients.c
+++ b/tests/i915/sysfs_clients.c
@@ -62,11 +62,13 @@
 
 static void strterm(char *s, int len)
 {
-	igt_assert(len > 0);
-
-	s[len] = '\0';
-	if (s[len - 1] == '\n')
-		s[len - 1] = '\0';
+	if (len < 0) {
+		*s = '\0';
+	} else {
+		s[len] = '\0';
+		if (s[len - 1] == '\n')
+			s[len - 1] = '\0';
+	}
 }
 
 static void pidname(int i915, int clients)
@@ -78,7 +80,6 @@ static void pidname(int i915, int clients)
 	long count;
 	pid_t pid;
 	DIR *dir;
-	int len;
 
 	dir = fdopendir(dup(clients));
 	igt_assert(dir);
@@ -90,13 +91,11 @@ static void pidname(int i915, int clients)
 			continue;
 
 		snprintf(buf, sizeof(buf), "%s/name", de->d_name);
-		len = igt_sysfs_read(clients, buf, buf, sizeof(buf) - 1);
-		igt_assert_f(len > 0, "failed to open '%s/name'\n", de->d_name);
-		strterm(buf, len);
+		strterm(buf, igt_sysfs_read(clients, buf, buf, sizeof(buf) - 1));
 		igt_debug("%s: %s\n", de->d_name, buf);
 
 		/* Ignore closed clients created by drm_driver_open() */
-		if (*buf == '<')
+		if (*buf == '\0' || *buf == '<')
 			continue;
 
 		close(me);
-- 
2.30.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [igt-dev] [PATCH i-g-t 1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs
@ 2021-01-26 13:05 ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-26 13:05 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev, Chris Wilson

An earlier client from an old test may still be lingering and disappear
as we scan the sysfs. Be graceful and let it go without tripping over it.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/sysfs_clients.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
index 6be52c04f..a3a1f81e1 100644
--- a/tests/i915/sysfs_clients.c
+++ b/tests/i915/sysfs_clients.c
@@ -62,11 +62,13 @@
 
 static void strterm(char *s, int len)
 {
-	igt_assert(len > 0);
-
-	s[len] = '\0';
-	if (s[len - 1] == '\n')
-		s[len - 1] = '\0';
+	if (len < 0) {
+		*s = '\0';
+	} else {
+		s[len] = '\0';
+		if (s[len - 1] == '\n')
+			s[len - 1] = '\0';
+	}
 }
 
 static void pidname(int i915, int clients)
@@ -78,7 +80,6 @@ static void pidname(int i915, int clients)
 	long count;
 	pid_t pid;
 	DIR *dir;
-	int len;
 
 	dir = fdopendir(dup(clients));
 	igt_assert(dir);
@@ -90,13 +91,11 @@ static void pidname(int i915, int clients)
 			continue;
 
 		snprintf(buf, sizeof(buf), "%s/name", de->d_name);
-		len = igt_sysfs_read(clients, buf, buf, sizeof(buf) - 1);
-		igt_assert_f(len > 0, "failed to open '%s/name'\n", de->d_name);
-		strterm(buf, len);
+		strterm(buf, igt_sysfs_read(clients, buf, buf, sizeof(buf) - 1));
 		igt_debug("%s: %s\n", de->d_name, buf);
 
 		/* Ignore closed clients created by drm_driver_open() */
-		if (*buf == '<')
+		if (*buf == '\0' || *buf == '<')
 			continue;
 
 		close(me);
-- 
2.30.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
  2021-01-26 13:05 ` [igt-dev] " Chris Wilson
@ 2021-01-26 13:05   ` Chris Wilson
  -1 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-26 13:05 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev, Chris Wilson

The client id used is a cyclic allocator as that reduces the likelihood
of userspace seeing the same id used again (and so confusing the new
client as the old). Verify that each new client has an id greater than
the last.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
 1 file changed, 108 insertions(+), 21 deletions(-)

diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
index a3a1f81e1..d2c1ebc5f 100644
--- a/tests/i915/sysfs_clients.c
+++ b/tests/i915/sysfs_clients.c
@@ -8,6 +8,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <inttypes.h>
+#include <limits.h>
 #include <math.h>
 #include <sched.h>
 #include <sys/socket.h>
@@ -47,6 +48,8 @@
 #define assert_within_epsilon(x, ref, tolerance) \
 	__assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
 
+#define BUFSZ 280
+
 #define MI_BATCH_BUFFER_START (0x31 << 23)
 #define MI_BATCH_BUFFER_END (0xa << 23)
 #define MI_ARB_CHECK (0x5 << 23)
@@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
 {
 	struct dirent *de;
 	int sv[2], rv[2];
-	char buf[280];
+	char buf[BUFSZ];
 	int me = -1;
 	long count;
 	pid_t pid;
@@ -180,7 +183,7 @@ static long count_clients(int clients)
 {
 	struct dirent *de;
 	long count = 0;
-	char buf[280];
+	char buf[BUFSZ];
 	DIR *dir;
 
 	dir = fdopendir(dup(clients));
@@ -229,32 +232,113 @@ static void create(int i915, int clients)
 	igt_assert_eq(count_clients(clients), 1);
 }
 
+static const char *find_client(int clients, pid_t pid, char *buf)
+{
+	DIR *dir = fdopendir(dup(clients));
+
+	/* Reading a dir as it changes does not appear to be stable, SEP */
+	for (int pass = 0; pass < 2; pass++) {
+		struct dirent *de;
+
+		rewinddir(dir);
+		while ((de = readdir(dir))) {
+			if (!isdigit(de->d_name[0]))
+				continue;
+
+			snprintf(buf, BUFSZ, "%s/pid", de->d_name);
+			igt_sysfs_read(clients, buf, buf, sizeof(buf));
+			if (atoi(buf) != pid)
+				continue;
+
+			strncpy(buf, de->d_name, BUFSZ);
+			goto out;
+		}
+	}
+	*buf = '\0';
+out:
+	closedir(dir);
+	return buf;
+}
+
 static int find_me(int clients, pid_t pid)
 {
-	struct dirent *de;
-	char buf[280];
-	int me = -1;
-	DIR *dir;
+	char buf[BUFSZ];
 
-	dir = fdopendir(dup(clients));
-	igt_assert(dir);
-	rewinddir(dir);
+	return openat(clients,
+		      find_client(clients, pid, buf),
+		      O_DIRECTORY | O_RDONLY);
+}
 
-	while ((de = readdir(dir))) {
-		if (!isdigit(de->d_name[0]))
-			continue;
+static int reopen_directory(int fd)
+{
+	char buf[BUFSZ];
+	int dir;
 
-		snprintf(buf, sizeof(buf), "%s/pid", de->d_name);
-		igt_sysfs_read(clients, buf, buf, sizeof(buf));
-		if (atoi(buf) != pid)
-			continue;
+	snprintf(buf, sizeof(buf), "/proc/self/fd/%d", fd);
+	dir = open(buf, O_RDONLY);
+	igt_assert_fd(dir);
 
-		me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
-		break;
+	return dir;
+}
+
+static unsigned int my_id(int clients, pid_t pid)
+{
+	char buf[BUFSZ];
+
+	return atoi(find_client(clients, pid, buf));
+}
+
+static unsigned int recycle_client(int i915, int clients)
+{
+	int device, client;
+
+	device = gem_reopen_driver(i915);
+	client = my_id(clients, getpid());
+	close(device);
+
+	return client;
+}
+
+static void recycle(int i915, int clients)
+{
+	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
+
+	/*
+	 * As we open and close clients, we do not expect to reuse old ids,
+	 * i.e. we use a cyclic ida. This reduces the likelihood of userspace
+	 * watchers becoming confused and mistaking the new client as a
+	 * continuation of the old.
+	 */
+	igt_require(my_id(clients, getpid()) < INT_MAX / 2);
+	igt_assert(my_id(clients, getpid()));
+
+	igt_fork(child, 2 * ncpus) {
+		unsigned int client, last;
+
+		/* Reopen the directory fd for each client */
+		clients = reopen_directory(clients);
+
+		last = recycle_client(i915, clients);
+		igt_info("Child[%d] first client:%d\n", getpid(), last);
+		igt_until_timeout(5) {
+			client = recycle_client(i915, clients);
+
+			/* In 5 seconds we are not going to exhaust the ids */
+			igt_assert(client > last);
+			last = client;
+		}
+		igt_info("Child[%d] last client:%d\n", getpid(), last);
 	}
+	igt_waitchildren();
 
-	closedir(dir);
-	return me;
+	/* Cleanup delayed behind rcu */
+	igt_until_timeout(30) {
+		sched_yield();
+		if (count_clients(clients) == 1)
+			break;
+		usleep(10000);
+	}
+	igt_assert_eq(count_clients(clients), 1);
 }
 
 static int64_t read_runtime(int client, int class)
@@ -719,7 +803,7 @@ sema(int i915, int clients, const struct intel_execution_engine2 *e, int f)
 static int read_all(int clients, pid_t pid, int class, uint64_t *runtime)
 {
 	struct dirent *de;
-	char buf[280];
+	char buf[BUFSZ];
 	int count = 0;
 	DIR *dir;
 
@@ -958,6 +1042,9 @@ igt_main
 	igt_subtest("create")
 		create(i915, clients);
 
+	igt_subtest("recycle")
+		recycle(i915, clients);
+
 	igt_subtest_group
 		test_busy(i915, clients);
 
-- 
2.30.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [igt-dev] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
@ 2021-01-26 13:05   ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-26 13:05 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev, Chris Wilson

The client id used is a cyclic allocator as that reduces the likelihood
of userspace seeing the same id used again (and so confusing the new
client as the old). Verify that each new client has an id greater than
the last.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
 1 file changed, 108 insertions(+), 21 deletions(-)

diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
index a3a1f81e1..d2c1ebc5f 100644
--- a/tests/i915/sysfs_clients.c
+++ b/tests/i915/sysfs_clients.c
@@ -8,6 +8,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <inttypes.h>
+#include <limits.h>
 #include <math.h>
 #include <sched.h>
 #include <sys/socket.h>
@@ -47,6 +48,8 @@
 #define assert_within_epsilon(x, ref, tolerance) \
 	__assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
 
+#define BUFSZ 280
+
 #define MI_BATCH_BUFFER_START (0x31 << 23)
 #define MI_BATCH_BUFFER_END (0xa << 23)
 #define MI_ARB_CHECK (0x5 << 23)
@@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
 {
 	struct dirent *de;
 	int sv[2], rv[2];
-	char buf[280];
+	char buf[BUFSZ];
 	int me = -1;
 	long count;
 	pid_t pid;
@@ -180,7 +183,7 @@ static long count_clients(int clients)
 {
 	struct dirent *de;
 	long count = 0;
-	char buf[280];
+	char buf[BUFSZ];
 	DIR *dir;
 
 	dir = fdopendir(dup(clients));
@@ -229,32 +232,113 @@ static void create(int i915, int clients)
 	igt_assert_eq(count_clients(clients), 1);
 }
 
+static const char *find_client(int clients, pid_t pid, char *buf)
+{
+	DIR *dir = fdopendir(dup(clients));
+
+	/* Reading a dir as it changes does not appear to be stable, SEP */
+	for (int pass = 0; pass < 2; pass++) {
+		struct dirent *de;
+
+		rewinddir(dir);
+		while ((de = readdir(dir))) {
+			if (!isdigit(de->d_name[0]))
+				continue;
+
+			snprintf(buf, BUFSZ, "%s/pid", de->d_name);
+			igt_sysfs_read(clients, buf, buf, sizeof(buf));
+			if (atoi(buf) != pid)
+				continue;
+
+			strncpy(buf, de->d_name, BUFSZ);
+			goto out;
+		}
+	}
+	*buf = '\0';
+out:
+	closedir(dir);
+	return buf;
+}
+
 static int find_me(int clients, pid_t pid)
 {
-	struct dirent *de;
-	char buf[280];
-	int me = -1;
-	DIR *dir;
+	char buf[BUFSZ];
 
-	dir = fdopendir(dup(clients));
-	igt_assert(dir);
-	rewinddir(dir);
+	return openat(clients,
+		      find_client(clients, pid, buf),
+		      O_DIRECTORY | O_RDONLY);
+}
 
-	while ((de = readdir(dir))) {
-		if (!isdigit(de->d_name[0]))
-			continue;
+static int reopen_directory(int fd)
+{
+	char buf[BUFSZ];
+	int dir;
 
-		snprintf(buf, sizeof(buf), "%s/pid", de->d_name);
-		igt_sysfs_read(clients, buf, buf, sizeof(buf));
-		if (atoi(buf) != pid)
-			continue;
+	snprintf(buf, sizeof(buf), "/proc/self/fd/%d", fd);
+	dir = open(buf, O_RDONLY);
+	igt_assert_fd(dir);
 
-		me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
-		break;
+	return dir;
+}
+
+static unsigned int my_id(int clients, pid_t pid)
+{
+	char buf[BUFSZ];
+
+	return atoi(find_client(clients, pid, buf));
+}
+
+static unsigned int recycle_client(int i915, int clients)
+{
+	int device, client;
+
+	device = gem_reopen_driver(i915);
+	client = my_id(clients, getpid());
+	close(device);
+
+	return client;
+}
+
+static void recycle(int i915, int clients)
+{
+	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
+
+	/*
+	 * As we open and close clients, we do not expect to reuse old ids,
+	 * i.e. we use a cyclic ida. This reduces the likelihood of userspace
+	 * watchers becoming confused and mistaking the new client as a
+	 * continuation of the old.
+	 */
+	igt_require(my_id(clients, getpid()) < INT_MAX / 2);
+	igt_assert(my_id(clients, getpid()));
+
+	igt_fork(child, 2 * ncpus) {
+		unsigned int client, last;
+
+		/* Reopen the directory fd for each client */
+		clients = reopen_directory(clients);
+
+		last = recycle_client(i915, clients);
+		igt_info("Child[%d] first client:%d\n", getpid(), last);
+		igt_until_timeout(5) {
+			client = recycle_client(i915, clients);
+
+			/* In 5 seconds we are not going to exhaust the ids */
+			igt_assert(client > last);
+			last = client;
+		}
+		igt_info("Child[%d] last client:%d\n", getpid(), last);
 	}
+	igt_waitchildren();
 
-	closedir(dir);
-	return me;
+	/* Cleanup delayed behind rcu */
+	igt_until_timeout(30) {
+		sched_yield();
+		if (count_clients(clients) == 1)
+			break;
+		usleep(10000);
+	}
+	igt_assert_eq(count_clients(clients), 1);
 }
 
 static int64_t read_runtime(int client, int class)
@@ -719,7 +803,7 @@ sema(int i915, int clients, const struct intel_execution_engine2 *e, int f)
 static int read_all(int clients, pid_t pid, int class, uint64_t *runtime)
 {
 	struct dirent *de;
-	char buf[280];
+	char buf[BUFSZ];
 	int count = 0;
 	DIR *dir;
 
@@ -958,6 +1042,9 @@ igt_main
 	igt_subtest("create")
 		create(i915, clients);
 
+	igt_subtest("recycle")
+		recycle(i915, clients);
+
 	igt_subtest_group
 		test_busy(i915, clients);
 
-- 
2.30.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs
  2021-01-26 13:05 ` [igt-dev] " Chris Wilson
  (?)
  (?)
@ 2021-01-26 16:33 ` Tvrtko Ursulin
  -1 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2021-01-26 16:33 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 26/01/2021 13:05, Chris Wilson wrote:
> An earlier client from an old test may still be lingering and disappear
> as we scan the sysfs. Be graceful and let it go without tripping over it.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   tests/i915/sysfs_clients.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
> index 6be52c04f..a3a1f81e1 100644
> --- a/tests/i915/sysfs_clients.c
> +++ b/tests/i915/sysfs_clients.c
> @@ -62,11 +62,13 @@
>   
>   static void strterm(char *s, int len)
>   {
> -	igt_assert(len > 0);
> -
> -	s[len] = '\0';
> -	if (s[len - 1] == '\n')
> -		s[len - 1] = '\0';
> +	if (len < 0) {
> +		*s = '\0';
> +	} else {
> +		s[len] = '\0';
> +		if (s[len - 1] == '\n')
> +			s[len - 1] = '\0';
> +	}
>   }
>   
>   static void pidname(int i915, int clients)
> @@ -78,7 +80,6 @@ static void pidname(int i915, int clients)
>   	long count;
>   	pid_t pid;
>   	DIR *dir;
> -	int len;
>   
>   	dir = fdopendir(dup(clients));
>   	igt_assert(dir);
> @@ -90,13 +91,11 @@ static void pidname(int i915, int clients)
>   			continue;
>   
>   		snprintf(buf, sizeof(buf), "%s/name", de->d_name);
> -		len = igt_sysfs_read(clients, buf, buf, sizeof(buf) - 1);
> -		igt_assert_f(len > 0, "failed to open '%s/name'\n", de->d_name);
> -		strterm(buf, len);
> +		strterm(buf, igt_sysfs_read(clients, buf, buf, sizeof(buf) - 1));
>   		igt_debug("%s: %s\n", de->d_name, buf);
>   
>   		/* Ignore closed clients created by drm_driver_open() */
> -		if (*buf == '<')
> +		if (*buf == '\0' || *buf == '<')
>   			continue;
>   
>   		close(me);
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
  2021-01-26 13:05   ` [igt-dev] " Chris Wilson
@ 2021-01-26 16:47     ` Tvrtko Ursulin
  -1 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2021-01-26 16:47 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 26/01/2021 13:05, Chris Wilson wrote:
> The client id used is a cyclic allocator as that reduces the likelihood
> of userspace seeing the same id used again (and so confusing the new
> client as the old). Verify that each new client has an id greater than
> the last.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
>   1 file changed, 108 insertions(+), 21 deletions(-)
> 
> diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
> index a3a1f81e1..d2c1ebc5f 100644
> --- a/tests/i915/sysfs_clients.c
> +++ b/tests/i915/sysfs_clients.c
> @@ -8,6 +8,7 @@
>   #include <errno.h>
>   #include <fcntl.h>
>   #include <inttypes.h>
> +#include <limits.h>
>   #include <math.h>
>   #include <sched.h>
>   #include <sys/socket.h>
> @@ -47,6 +48,8 @@
>   #define assert_within_epsilon(x, ref, tolerance) \
>   	__assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
>   
> +#define BUFSZ 280
> +
>   #define MI_BATCH_BUFFER_START (0x31 << 23)
>   #define MI_BATCH_BUFFER_END (0xa << 23)
>   #define MI_ARB_CHECK (0x5 << 23)
> @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
>   {
>   	struct dirent *de;
>   	int sv[2], rv[2];
> -	char buf[280];
> +	char buf[BUFSZ];
>   	int me = -1;
>   	long count;
>   	pid_t pid;
> @@ -180,7 +183,7 @@ static long count_clients(int clients)
>   {
>   	struct dirent *de;
>   	long count = 0;
> -	char buf[280];
> +	char buf[BUFSZ];
>   	DIR *dir;
>   
>   	dir = fdopendir(dup(clients));
> @@ -229,32 +232,113 @@ static void create(int i915, int clients)
>   	igt_assert_eq(count_clients(clients), 1);
>   }
>   
> +static const char *find_client(int clients, pid_t pid, char *buf)
> +{
> +	DIR *dir = fdopendir(dup(clients));
> +
> +	/* Reading a dir as it changes does not appear to be stable, SEP */

Oh yes, it is POSIX undefined as far as I could figure out. You are 
creating/destroying clients while reading the dir from different 
threads? I think best might be to introduce explicit sync points between 
those two entities to make this reliable. In another review for the same 
problem I suggested pipes for implementing this handshake. Along the 
lines of "let child open/close some processes, make it wait for parent; 
parent scans sysfs, signals child to open/close some more; repeat for a 
while".

Regards,

Tvrtko

> +	for (int pass = 0; pass < 2; pass++) {
> +		struct dirent *de;
> +
> +		rewinddir(dir);
> +		while ((de = readdir(dir))) {
> +			if (!isdigit(de->d_name[0]))
> +				continue;
> +
> +			snprintf(buf, BUFSZ, "%s/pid", de->d_name);
> +			igt_sysfs_read(clients, buf, buf, sizeof(buf));
> +			if (atoi(buf) != pid)
> +				continue;
> +
> +			strncpy(buf, de->d_name, BUFSZ);
> +			goto out;
> +		}
> +	}
> +	*buf = '\0';
> +out:
> +	closedir(dir);
> +	return buf;
> +}
> +
>   static int find_me(int clients, pid_t pid)
>   {
> -	struct dirent *de;
> -	char buf[280];
> -	int me = -1;
> -	DIR *dir;
> +	char buf[BUFSZ];
>   
> -	dir = fdopendir(dup(clients));
> -	igt_assert(dir);
> -	rewinddir(dir);
> +	return openat(clients,
> +		      find_client(clients, pid, buf),
> +		      O_DIRECTORY | O_RDONLY);
> +}
>   
> -	while ((de = readdir(dir))) {
> -		if (!isdigit(de->d_name[0]))
> -			continue;
> +static int reopen_directory(int fd)
> +{
> +	char buf[BUFSZ];
> +	int dir;
>   
> -		snprintf(buf, sizeof(buf), "%s/pid", de->d_name);
> -		igt_sysfs_read(clients, buf, buf, sizeof(buf));
> -		if (atoi(buf) != pid)
> -			continue;
> +	snprintf(buf, sizeof(buf), "/proc/self/fd/%d", fd);
> +	dir = open(buf, O_RDONLY);
> +	igt_assert_fd(dir);
>   
> -		me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
> -		break;
> +	return dir;
> +}
> +
> +static unsigned int my_id(int clients, pid_t pid)
> +{
> +	char buf[BUFSZ];
> +
> +	return atoi(find_client(clients, pid, buf));
> +}
> +
> +static unsigned int recycle_client(int i915, int clients)
> +{
> +	int device, client;
> +
> +	device = gem_reopen_driver(i915);
> +	client = my_id(clients, getpid());
> +	close(device);
> +
> +	return client;
> +}
> +
> +static void recycle(int i915, int clients)
> +{
> +	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> +
> +	/*
> +	 * As we open and close clients, we do not expect to reuse old ids,
> +	 * i.e. we use a cyclic ida. This reduces the likelihood of userspace
> +	 * watchers becoming confused and mistaking the new client as a
> +	 * continuation of the old.
> +	 */
> +	igt_require(my_id(clients, getpid()) < INT_MAX / 2);
> +	igt_assert(my_id(clients, getpid()));
> +
> +	igt_fork(child, 2 * ncpus) {
> +		unsigned int client, last;
> +
> +		/* Reopen the directory fd for each client */
> +		clients = reopen_directory(clients);
> +
> +		last = recycle_client(i915, clients);
> +		igt_info("Child[%d] first client:%d\n", getpid(), last);
> +		igt_until_timeout(5) {
> +			client = recycle_client(i915, clients);
> +
> +			/* In 5 seconds we are not going to exhaust the ids */
> +			igt_assert(client > last);
> +			last = client;
> +		}
> +		igt_info("Child[%d] last client:%d\n", getpid(), last);
>   	}
> +	igt_waitchildren();
>   
> -	closedir(dir);
> -	return me;
> +	/* Cleanup delayed behind rcu */
> +	igt_until_timeout(30) {
> +		sched_yield();
> +		if (count_clients(clients) == 1)
> +			break;
> +		usleep(10000);
> +	}
> +	igt_assert_eq(count_clients(clients), 1);
>   }
>   
>   static int64_t read_runtime(int client, int class)
> @@ -719,7 +803,7 @@ sema(int i915, int clients, const struct intel_execution_engine2 *e, int f)
>   static int read_all(int clients, pid_t pid, int class, uint64_t *runtime)
>   {
>   	struct dirent *de;
> -	char buf[280];
> +	char buf[BUFSZ];
>   	int count = 0;
>   	DIR *dir;
>   
> @@ -958,6 +1042,9 @@ igt_main
>   	igt_subtest("create")
>   		create(i915, clients);
>   
> +	igt_subtest("recycle")
> +		recycle(i915, clients);
> +
>   	igt_subtest_group
>   		test_busy(i915, clients);
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
@ 2021-01-26 16:47     ` Tvrtko Ursulin
  0 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2021-01-26 16:47 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 26/01/2021 13:05, Chris Wilson wrote:
> The client id used is a cyclic allocator as that reduces the likelihood
> of userspace seeing the same id used again (and so confusing the new
> client as the old). Verify that each new client has an id greater than
> the last.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
>   1 file changed, 108 insertions(+), 21 deletions(-)
> 
> diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
> index a3a1f81e1..d2c1ebc5f 100644
> --- a/tests/i915/sysfs_clients.c
> +++ b/tests/i915/sysfs_clients.c
> @@ -8,6 +8,7 @@
>   #include <errno.h>
>   #include <fcntl.h>
>   #include <inttypes.h>
> +#include <limits.h>
>   #include <math.h>
>   #include <sched.h>
>   #include <sys/socket.h>
> @@ -47,6 +48,8 @@
>   #define assert_within_epsilon(x, ref, tolerance) \
>   	__assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
>   
> +#define BUFSZ 280
> +
>   #define MI_BATCH_BUFFER_START (0x31 << 23)
>   #define MI_BATCH_BUFFER_END (0xa << 23)
>   #define MI_ARB_CHECK (0x5 << 23)
> @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
>   {
>   	struct dirent *de;
>   	int sv[2], rv[2];
> -	char buf[280];
> +	char buf[BUFSZ];
>   	int me = -1;
>   	long count;
>   	pid_t pid;
> @@ -180,7 +183,7 @@ static long count_clients(int clients)
>   {
>   	struct dirent *de;
>   	long count = 0;
> -	char buf[280];
> +	char buf[BUFSZ];
>   	DIR *dir;
>   
>   	dir = fdopendir(dup(clients));
> @@ -229,32 +232,113 @@ static void create(int i915, int clients)
>   	igt_assert_eq(count_clients(clients), 1);
>   }
>   
> +static const char *find_client(int clients, pid_t pid, char *buf)
> +{
> +	DIR *dir = fdopendir(dup(clients));
> +
> +	/* Reading a dir as it changes does not appear to be stable, SEP */

Oh yes, it is POSIX undefined as far as I could figure out. You are 
creating/destroying clients while reading the dir from different 
threads? I think best might be to introduce explicit sync points between 
those two entities to make this reliable. In another review for the same 
problem I suggested pipes for implementing this handshake. Along the 
lines of "let child open/close some processes, make it wait for parent; 
parent scans sysfs, signals child to open/close some more; repeat for a 
while".

Regards,

Tvrtko

> +	for (int pass = 0; pass < 2; pass++) {
> +		struct dirent *de;
> +
> +		rewinddir(dir);
> +		while ((de = readdir(dir))) {
> +			if (!isdigit(de->d_name[0]))
> +				continue;
> +
> +			snprintf(buf, BUFSZ, "%s/pid", de->d_name);
> +			igt_sysfs_read(clients, buf, buf, sizeof(buf));
> +			if (atoi(buf) != pid)
> +				continue;
> +
> +			strncpy(buf, de->d_name, BUFSZ);
> +			goto out;
> +		}
> +	}
> +	*buf = '\0';
> +out:
> +	closedir(dir);
> +	return buf;
> +}
> +
>   static int find_me(int clients, pid_t pid)
>   {
> -	struct dirent *de;
> -	char buf[280];
> -	int me = -1;
> -	DIR *dir;
> +	char buf[BUFSZ];
>   
> -	dir = fdopendir(dup(clients));
> -	igt_assert(dir);
> -	rewinddir(dir);
> +	return openat(clients,
> +		      find_client(clients, pid, buf),
> +		      O_DIRECTORY | O_RDONLY);
> +}
>   
> -	while ((de = readdir(dir))) {
> -		if (!isdigit(de->d_name[0]))
> -			continue;
> +static int reopen_directory(int fd)
> +{
> +	char buf[BUFSZ];
> +	int dir;
>   
> -		snprintf(buf, sizeof(buf), "%s/pid", de->d_name);
> -		igt_sysfs_read(clients, buf, buf, sizeof(buf));
> -		if (atoi(buf) != pid)
> -			continue;
> +	snprintf(buf, sizeof(buf), "/proc/self/fd/%d", fd);
> +	dir = open(buf, O_RDONLY);
> +	igt_assert_fd(dir);
>   
> -		me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
> -		break;
> +	return dir;
> +}
> +
> +static unsigned int my_id(int clients, pid_t pid)
> +{
> +	char buf[BUFSZ];
> +
> +	return atoi(find_client(clients, pid, buf));
> +}
> +
> +static unsigned int recycle_client(int i915, int clients)
> +{
> +	int device, client;
> +
> +	device = gem_reopen_driver(i915);
> +	client = my_id(clients, getpid());
> +	close(device);
> +
> +	return client;
> +}
> +
> +static void recycle(int i915, int clients)
> +{
> +	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> +
> +	/*
> +	 * As we open and close clients, we do not expect to reuse old ids,
> +	 * i.e. we use a cyclic ida. This reduces the likelihood of userspace
> +	 * watchers becoming confused and mistaking the new client as a
> +	 * continuation of the old.
> +	 */
> +	igt_require(my_id(clients, getpid()) < INT_MAX / 2);
> +	igt_assert(my_id(clients, getpid()));
> +
> +	igt_fork(child, 2 * ncpus) {
> +		unsigned int client, last;
> +
> +		/* Reopen the directory fd for each client */
> +		clients = reopen_directory(clients);
> +
> +		last = recycle_client(i915, clients);
> +		igt_info("Child[%d] first client:%d\n", getpid(), last);
> +		igt_until_timeout(5) {
> +			client = recycle_client(i915, clients);
> +
> +			/* In 5 seconds we are not going to exhaust the ids */
> +			igt_assert(client > last);
> +			last = client;
> +		}
> +		igt_info("Child[%d] last client:%d\n", getpid(), last);
>   	}
> +	igt_waitchildren();
>   
> -	closedir(dir);
> -	return me;
> +	/* Cleanup delayed behind rcu */
> +	igt_until_timeout(30) {
> +		sched_yield();
> +		if (count_clients(clients) == 1)
> +			break;
> +		usleep(10000);
> +	}
> +	igt_assert_eq(count_clients(clients), 1);
>   }
>   
>   static int64_t read_runtime(int client, int class)
> @@ -719,7 +803,7 @@ sema(int i915, int clients, const struct intel_execution_engine2 *e, int f)
>   static int read_all(int clients, pid_t pid, int class, uint64_t *runtime)
>   {
>   	struct dirent *de;
> -	char buf[280];
> +	char buf[BUFSZ];
>   	int count = 0;
>   	DIR *dir;
>   
> @@ -958,6 +1042,9 @@ igt_main
>   	igt_subtest("create")
>   		create(i915, clients);
>   
> +	igt_subtest("recycle")
> +		recycle(i915, clients);
> +
>   	igt_subtest_group
>   		test_busy(i915, clients);
>   
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [igt-dev] ✗ GitLab.Pipeline: failure for series starting with [i-g-t,1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs
  2021-01-26 13:05 ` [igt-dev] " Chris Wilson
                   ` (2 preceding siblings ...)
  (?)
@ 2021-01-26 18:06 ` Patchwork
  -1 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2021-01-26 18:06 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev

== Series Details ==

Series: series starting with [i-g-t,1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs
URL   : https://patchwork.freedesktop.org/series/86298/
State : failure

== Summary ==

ERROR! This series introduces new undocumented tests:

sysfs_clients@recycle

Can you document them as per the requirement in the [CONTRIBUTING.md]?

[Documentation] has more details on how to do this.

Here are few examples:
https://gitlab.freedesktop.org/drm/igt-gpu-tools/commit/0316695d03aa46108296b27f3982ec93200c7a6e
https://gitlab.freedesktop.org/drm/igt-gpu-tools/commit/443cc658e1e6b492ee17bf4f4d891029eb7a205d

Thanks in advance!

[CONTRIBUTING.md]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/CONTRIBUTING.md#L19
[Documentation]: https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-Core.html#igt-describe

Other than that, pipeline status: SUCCESS.

see https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/261939 for the overview.

== Logs ==

For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/261939
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs
  2021-01-26 13:05 ` [igt-dev] " Chris Wilson
                   ` (3 preceding siblings ...)
  (?)
@ 2021-01-26 18:18 ` Patchwork
  -1 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2021-01-26 18:18 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev


[-- Attachment #1.1: Type: text/plain, Size: 3550 bytes --]

== Series Details ==

Series: series starting with [i-g-t,1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs
URL   : https://patchwork.freedesktop.org/series/86298/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_9684 -> IGTPW_5432
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_5432:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
    - {fi-rkl-11500t}:    [SKIP][1] ([i915#533]) -> [SKIP][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/fi-rkl-11500t/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/fi-rkl-11500t/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html

  
Known issues
------------

  Here are the changes found in IGTPW_5432 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@prime_self_import@basic-with_one_bo_two_files:
    - fi-tgl-y:           [PASS][3] -> [DMESG-WARN][4] ([i915#402]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/fi-tgl-y/igt@prime_self_import@basic-with_one_bo_two_files.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/fi-tgl-y/igt@prime_self_import@basic-with_one_bo_two_files.html

  
#### Possible fixes ####

  * igt@debugfs_test@read_all_entries:
    - fi-tgl-y:           [DMESG-WARN][5] ([i915#402]) -> [PASS][6] +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/fi-tgl-y/igt@debugfs_test@read_all_entries.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/fi-tgl-y/igt@debugfs_test@read_all_entries.html

  * igt@i915_module_load@reload:
    - fi-kbl-7500u:       [DMESG-WARN][7] ([i915#2605]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/fi-kbl-7500u/igt@i915_module_load@reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/fi-kbl-7500u/igt@i915_module_load@reload.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2605]: https://gitlab.freedesktop.org/drm/intel/issues/2605
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533


Participating hosts (44 -> 38)
------------------------------

  Missing    (6): fi-jsl-1 fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600 fi-dg1-1 


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_5972 -> IGTPW_5432

  CI-20190529: 20190529
  CI_DRM_9684: 53a183b40b798192f7211b05d550c8145d1397b5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_5432: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/index.html
  IGT_5972: 82fa6021821edb5d9609f4cce213920e0936d6f3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@sysfs_clients@recycle

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/index.html

[-- Attachment #1.2: Type: text/html, Size: 4179 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
  2021-01-26 16:47     ` [igt-dev] " Tvrtko Ursulin
  (?)
@ 2021-01-26 22:04     ` Chris Wilson
  2021-01-26 22:07         ` [igt-dev] " Chris Wilson
  2021-01-27  9:12         ` [igt-dev] " Tvrtko Ursulin
  -1 siblings, 2 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-26 22:04 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2021-01-26 16:47:14)
> 
> On 26/01/2021 13:05, Chris Wilson wrote:
> > The client id used is a cyclic allocator as that reduces the likelihood
> > of userspace seeing the same id used again (and so confusing the new
> > client as the old). Verify that each new client has an id greater than
> > the last.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > ---
> >   tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
> >   1 file changed, 108 insertions(+), 21 deletions(-)
> > 
> > diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
> > index a3a1f81e1..d2c1ebc5f 100644
> > --- a/tests/i915/sysfs_clients.c
> > +++ b/tests/i915/sysfs_clients.c
> > @@ -8,6 +8,7 @@
> >   #include <errno.h>
> >   #include <fcntl.h>
> >   #include <inttypes.h>
> > +#include <limits.h>
> >   #include <math.h>
> >   #include <sched.h>
> >   #include <sys/socket.h>
> > @@ -47,6 +48,8 @@
> >   #define assert_within_epsilon(x, ref, tolerance) \
> >       __assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
> >   
> > +#define BUFSZ 280
> > +
> >   #define MI_BATCH_BUFFER_START (0x31 << 23)
> >   #define MI_BATCH_BUFFER_END (0xa << 23)
> >   #define MI_ARB_CHECK (0x5 << 23)
> > @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
> >   {
> >       struct dirent *de;
> >       int sv[2], rv[2];
> > -     char buf[280];
> > +     char buf[BUFSZ];
> >       int me = -1;
> >       long count;
> >       pid_t pid;
> > @@ -180,7 +183,7 @@ static long count_clients(int clients)
> >   {
> >       struct dirent *de;
> >       long count = 0;
> > -     char buf[280];
> > +     char buf[BUFSZ];
> >       DIR *dir;
> >   
> >       dir = fdopendir(dup(clients));
> > @@ -229,32 +232,113 @@ static void create(int i915, int clients)
> >       igt_assert_eq(count_clients(clients), 1);
> >   }
> >   
> > +static const char *find_client(int clients, pid_t pid, char *buf)
> > +{
> > +     DIR *dir = fdopendir(dup(clients));
> > +
> > +     /* Reading a dir as it changes does not appear to be stable, SEP */
> 
> Oh yes, it is POSIX undefined as far as I could figure out. You are 
> creating/destroying clients while reading the dir from different 
> threads?

Different processes on different directory fd. man readdir(3) does say
that glibc is threadsafe, but that is not a requirement of POSIX. The
problem we are seeing is that the directory contents are not stable
around the readdir loop as clients are being created/destroyed, causing
dirents to be missed.

stracing the problem shows that glibc used a 32KiB buffer for getdents
and only a single syscall was required to grab the entire directory. No
errors were seen before the missed dirent. It just seemed to be missing.

Afaict there is no delay in creating the sysfs entry, and I think there
should also be no delay in creating the kernfs node and inodes, so my
initial assumption is that it's something not quite happy in the
kernfs directory that shows up under stress. A second loop ran for a
long time without incident locally, but CI seems much more adept at
finding it.

> I think best might be to introduce explicit sync points between 
> those two entities to make this reliable. In another review for the same 
> problem I suggested pipes for implementing this handshake. Along the 
> lines of "let child open/close some processes, make it wait for parent; 
> parent scans sysfs, signals child to open/close some more; repeat for a 
> while".

Sadly, I don't expect userspace to do that :)

I do expect userspace to search for its own client/ upon creation, and I
expect there to be many clients being opened at once. So I think so long
as the use of the library readdir is correct (distinct processes with
distinct directory fd, so I think there's no accidental sharing) this
represents. Hmm. fsync(dirfd).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
  2021-01-26 22:04     ` [Intel-gfx] " Chris Wilson
@ 2021-01-26 22:07         ` Chris Wilson
  2021-01-27  9:12         ` [igt-dev] " Tvrtko Ursulin
  1 sibling, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-26 22:07 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Chris Wilson (2021-01-26 22:04:35)
> I do expect userspace to search for its own client/ upon creation, and I
> expect there to be many clients being opened at once. So I think so long
> as the use of the library readdir is correct (distinct processes with
> distinct directory fd, so I think there's no accidental sharing) this
> represents. Hmm. fsync(dirfd).

fsync(dirfd) makes no difference.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
@ 2021-01-26 22:07         ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-26 22:07 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Chris Wilson (2021-01-26 22:04:35)
> I do expect userspace to search for its own client/ upon creation, and I
> expect there to be many clients being opened at once. So I think so long
> as the use of the library readdir is correct (distinct processes with
> distinct directory fd, so I think there's no accidental sharing) this
> represents. Hmm. fsync(dirfd).

fsync(dirfd) makes no difference.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs
  2021-01-26 13:05 ` [igt-dev] " Chris Wilson
                   ` (4 preceding siblings ...)
  (?)
@ 2021-01-26 23:20 ` Patchwork
  -1 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2021-01-26 23:20 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev


[-- Attachment #1.1: Type: text/plain, Size: 30320 bytes --]

== Series Details ==

Series: series starting with [i-g-t,1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs
URL   : https://patchwork.freedesktop.org/series/86298/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_9684_full -> IGTPW_5432_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_5432_full:

### IGT changes ###

#### Possible regressions ####

  * {igt@sysfs_clients@recycle} (NEW):
    - shard-snb:          NOTRUN -> [FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-snb5/igt@sysfs_clients@recycle.html
    - shard-iclb:         NOTRUN -> [FAIL][2]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb5/igt@sysfs_clients@recycle.html
    - shard-tglb:         NOTRUN -> [FAIL][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb6/igt@sysfs_clients@recycle.html

  
New tests
---------

  New tests have been introduced between CI_DRM_9684_full and IGTPW_5432_full:

### New IGT tests (1) ###

  * igt@sysfs_clients@recycle:
    - Statuses : 3 fail(s) 4 pass(s)
    - Exec time: [0.73, 5.54] s

  

Known issues
------------

  Here are the changes found in IGTPW_5432_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@core_hotunplug@unbind-rebind:
    - shard-tglb:         [PASS][4] -> [DMESG-WARN][5] ([i915#1602])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-tglb6/igt@core_hotunplug@unbind-rebind.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb8/igt@core_hotunplug@unbind-rebind.html
    - shard-apl:          [PASS][6] -> [DMESG-WARN][7] ([i915#2283])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-apl3/igt@core_hotunplug@unbind-rebind.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl3/igt@core_hotunplug@unbind-rebind.html
    - shard-glk:          [PASS][8] -> [DMESG-WARN][9] ([i915#2283])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-glk9/igt@core_hotunplug@unbind-rebind.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk7/igt@core_hotunplug@unbind-rebind.html
    - shard-kbl:          [PASS][10] -> [DMESG-WARN][11] ([i915#2283])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-kbl4/igt@core_hotunplug@unbind-rebind.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl2/igt@core_hotunplug@unbind-rebind.html
    - shard-iclb:         [PASS][12] -> [DMESG-WARN][13] ([i915#1602] / [i915#2283])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-iclb5/igt@core_hotunplug@unbind-rebind.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb4/igt@core_hotunplug@unbind-rebind.html

  * igt@feature_discovery@display-4x:
    - shard-tglb:         NOTRUN -> [SKIP][14] ([i915#1839])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb6/igt@feature_discovery@display-4x.html

  * igt@gem_ctx_persistence@hang:
    - shard-snb:          NOTRUN -> [SKIP][15] ([fdo#109271] / [i915#1099])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-snb7/igt@gem_ctx_persistence@hang.html

  * igt@gem_ctx_persistence@legacy-engines-mixed-process:
    - shard-hsw:          NOTRUN -> [SKIP][16] ([fdo#109271] / [i915#1099]) +5 similar issues
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-hsw2/igt@gem_ctx_persistence@legacy-engines-mixed-process.html

  * igt@gem_exec_fair@basic-deadline:
    - shard-kbl:          [PASS][17] -> [FAIL][18] ([i915#2846])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-kbl2/igt@gem_exec_fair@basic-deadline.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl1/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-flow@rcs0:
    - shard-tglb:         NOTRUN -> [FAIL][19] ([i915#2842]) +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb1/igt@gem_exec_fair@basic-flow@rcs0.html

  * igt@gem_exec_fair@basic-none-share@rcs0:
    - shard-iclb:         [PASS][20] -> [FAIL][21] ([i915#2842]) +1 similar issue
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-iclb6/igt@gem_exec_fair@basic-none-share@rcs0.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb3/igt@gem_exec_fair@basic-none-share@rcs0.html

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-iclb:         NOTRUN -> [FAIL][22] ([i915#2842])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb5/igt@gem_exec_fair@basic-pace-solo@rcs0.html
    - shard-glk:          NOTRUN -> [FAIL][23] ([i915#2842])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk5/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@gem_exec_fair@basic-throttle@rcs0:
    - shard-glk:          [PASS][24] -> [FAIL][25] ([i915#2842]) +3 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-glk9/igt@gem_exec_fair@basic-throttle@rcs0.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk8/igt@gem_exec_fair@basic-throttle@rcs0.html

  * igt@gem_exec_reloc@basic-parallel:
    - shard-apl:          NOTRUN -> [TIMEOUT][26] ([i915#1729])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl1/igt@gem_exec_reloc@basic-parallel.html

  * igt@gem_exec_reloc@basic-wide-active@vcs1:
    - shard-iclb:         NOTRUN -> [FAIL][27] ([i915#2389]) +1 similar issue
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb2/igt@gem_exec_reloc@basic-wide-active@vcs1.html

  * igt@gem_exec_schedule@u-fairslice@vcs1:
    - shard-tglb:         [PASS][28] -> [DMESG-WARN][29] ([i915#2803])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-tglb6/igt@gem_exec_schedule@u-fairslice@vcs1.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb1/igt@gem_exec_schedule@u-fairslice@vcs1.html

  * igt@gem_exec_whisper@basic-queues-forked-all:
    - shard-glk:          [PASS][30] -> [DMESG-WARN][31] ([i915#118] / [i915#95]) +2 similar issues
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-glk5/igt@gem_exec_whisper@basic-queues-forked-all.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk4/igt@gem_exec_whisper@basic-queues-forked-all.html

  * igt@gem_fenced_exec_thrash@too-many-fences:
    - shard-snb:          [PASS][32] -> [INCOMPLETE][33] ([i915#2055])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-snb2/igt@gem_fenced_exec_thrash@too-many-fences.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-snb1/igt@gem_fenced_exec_thrash@too-many-fences.html

  * igt@gem_huc_copy@huc-copy:
    - shard-apl:          NOTRUN -> [SKIP][34] ([fdo#109271] / [i915#2190])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl1/igt@gem_huc_copy@huc-copy.html
    - shard-glk:          NOTRUN -> [SKIP][35] ([fdo#109271] / [i915#2190])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk6/igt@gem_huc_copy@huc-copy.html
    - shard-iclb:         NOTRUN -> [SKIP][36] ([i915#2190])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb3/igt@gem_huc_copy@huc-copy.html
    - shard-kbl:          NOTRUN -> [SKIP][37] ([fdo#109271] / [i915#2190])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl7/igt@gem_huc_copy@huc-copy.html

  * igt@gem_userptr_blits@mmap-offset-invalidate-active@gtt:
    - shard-iclb:         NOTRUN -> [SKIP][38] ([i915#1317]) +3 similar issues
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb3/igt@gem_userptr_blits@mmap-offset-invalidate-active@gtt.html

  * igt@gem_userptr_blits@mmap-offset-invalidate-active@wb:
    - shard-snb:          NOTRUN -> [SKIP][39] ([fdo#109271]) +60 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-snb5/igt@gem_userptr_blits@mmap-offset-invalidate-active@wb.html
    - shard-kbl:          NOTRUN -> [SKIP][40] ([fdo#109271]) +65 similar issues
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl7/igt@gem_userptr_blits@mmap-offset-invalidate-active@wb.html

  * igt@gem_userptr_blits@mmap-offset-invalidate-active@wc:
    - shard-tglb:         NOTRUN -> [SKIP][41] ([i915#1317]) +3 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb1/igt@gem_userptr_blits@mmap-offset-invalidate-active@wc.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-glk:          [PASS][42] -> [DMESG-WARN][43] ([i915#1436] / [i915#716])
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-glk5/igt@gen9_exec_parse@allowed-all.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk7/igt@gen9_exec_parse@allowed-all.html

  * igt@gen9_exec_parse@bb-start-out:
    - shard-iclb:         NOTRUN -> [SKIP][44] ([fdo#112306])
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb5/igt@gen9_exec_parse@bb-start-out.html

  * igt@i915_pm_rc6_residency@media-rc6-accuracy:
    - shard-iclb:         NOTRUN -> [SKIP][45] ([fdo#109289])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb5/igt@i915_pm_rc6_residency@media-rc6-accuracy.html

  * igt@i915_pm_rc6_residency@rc6-idle:
    - shard-tglb:         [PASS][46] -> [WARN][47] ([i915#2681] / [i915#2684])
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-tglb8/igt@i915_pm_rc6_residency@rc6-idle.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb2/igt@i915_pm_rc6_residency@rc6-idle.html
    - shard-hsw:          [PASS][48] -> [FAIL][49] ([i915#1860])
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-hsw1/igt@i915_pm_rc6_residency@rc6-idle.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-hsw1/igt@i915_pm_rc6_residency@rc6-idle.html

  * igt@kms_async_flips@test-time-stamp:
    - shard-tglb:         [PASS][50] -> [FAIL][51] ([i915#2597])
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-tglb6/igt@kms_async_flips@test-time-stamp.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb3/igt@kms_async_flips@test-time-stamp.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - shard-apl:          NOTRUN -> [SKIP][52] ([fdo#109271] / [fdo#111827]) +6 similar issues
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl2/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@hdmi-hpd-with-enabled-mode:
    - shard-hsw:          NOTRUN -> [SKIP][53] ([fdo#109271] / [fdo#111827]) +12 similar issues
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-hsw2/igt@kms_chamelium@hdmi-hpd-with-enabled-mode.html
    - shard-tglb:         NOTRUN -> [SKIP][54] ([fdo#109284] / [fdo#111827]) +2 similar issues
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb6/igt@kms_chamelium@hdmi-hpd-with-enabled-mode.html

  * igt@kms_chamelium@hdmi-mode-timings:
    - shard-snb:          NOTRUN -> [SKIP][55] ([fdo#109271] / [fdo#111827]) +2 similar issues
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-snb5/igt@kms_chamelium@hdmi-mode-timings.html
    - shard-kbl:          NOTRUN -> [SKIP][56] ([fdo#109271] / [fdo#111827]) +5 similar issues
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl2/igt@kms_chamelium@hdmi-mode-timings.html
    - shard-iclb:         NOTRUN -> [SKIP][57] ([fdo#109284] / [fdo#111827]) +3 similar issues
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb4/igt@kms_chamelium@hdmi-mode-timings.html
    - shard-glk:          NOTRUN -> [SKIP][58] ([fdo#109271] / [fdo#111827]) +4 similar issues
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk7/igt@kms_chamelium@hdmi-mode-timings.html

  * igt@kms_color@pipe-a-ctm-0-75:
    - shard-iclb:         NOTRUN -> [FAIL][59] ([i915#1149] / [i915#315])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb5/igt@kms_color@pipe-a-ctm-0-75.html

  * igt@kms_color@pipe-d-ctm-0-75:
    - shard-tglb:         NOTRUN -> [FAIL][60] ([i915#1149] / [i915#315]) +1 similar issue
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb5/igt@kms_color@pipe-d-ctm-0-75.html
    - shard-iclb:         NOTRUN -> [SKIP][61] ([fdo#109278] / [i915#1149])
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb2/igt@kms_color@pipe-d-ctm-0-75.html

  * igt@kms_color_chamelium@pipe-d-ctm-red-to-blue:
    - shard-iclb:         NOTRUN -> [SKIP][62] ([fdo#109278] / [fdo#109284] / [fdo#111827])
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb4/igt@kms_color_chamelium@pipe-d-ctm-red-to-blue.html

  * igt@kms_content_protection@mei_interface:
    - shard-iclb:         NOTRUN -> [SKIP][63] ([fdo#109300] / [fdo#111066])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb7/igt@kms_content_protection@mei_interface.html
    - shard-tglb:         NOTRUN -> [SKIP][64] ([fdo#111828])
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb5/igt@kms_content_protection@mei_interface.html

  * igt@kms_content_protection@srm:
    - shard-apl:          NOTRUN -> [TIMEOUT][65] ([i915#1319])
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl8/igt@kms_content_protection@srm.html

  * igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen:
    - shard-kbl:          [PASS][66] -> [FAIL][67] ([i915#54])
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-kbl6/igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen.html
    - shard-apl:          [PASS][68] -> [FAIL][69] ([i915#54])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-apl7/igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen.html
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl8/igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen.html

  * igt@kms_cursor_crc@pipe-c-cursor-512x512-rapid-movement:
    - shard-iclb:         NOTRUN -> [SKIP][70] ([fdo#109278] / [fdo#109279])
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb8/igt@kms_cursor_crc@pipe-c-cursor-512x512-rapid-movement.html

  * igt@kms_cursor_crc@pipe-d-cursor-512x512-onscreen:
    - shard-tglb:         NOTRUN -> [SKIP][71] ([fdo#109279])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb2/igt@kms_cursor_crc@pipe-d-cursor-512x512-onscreen.html

  * igt@kms_cursor_legacy@cursorb-vs-flipa-atomic:
    - shard-iclb:         NOTRUN -> [SKIP][72] ([fdo#109274] / [fdo#109278])
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb6/igt@kms_cursor_legacy@cursorb-vs-flipa-atomic.html

  * igt@kms_dp_tiled_display@basic-test-pattern:
    - shard-iclb:         NOTRUN -> [SKIP][73] ([i915#426])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb7/igt@kms_dp_tiled_display@basic-test-pattern.html

  * igt@kms_flip@2x-flip-vs-modeset-vs-hang:
    - shard-iclb:         NOTRUN -> [SKIP][74] ([fdo#109274]) +1 similar issue
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb3/igt@kms_flip@2x-flip-vs-modeset-vs-hang.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1:
    - shard-tglb:         [PASS][75] -> [FAIL][76] ([i915#2598])
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-tglb3/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb1/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html

  * igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile:
    - shard-apl:          NOTRUN -> [SKIP][77] ([fdo#109271] / [i915#2642])
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl7/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile.html
    - shard-glk:          NOTRUN -> [SKIP][78] ([fdo#109271] / [i915#2642])
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk7/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile.html
    - shard-kbl:          NOTRUN -> [SKIP][79] ([fdo#109271] / [i915#2642])
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl1/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile.html

  * igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilercccs:
    - shard-kbl:          NOTRUN -> [SKIP][80] ([fdo#109271] / [i915#2672])
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl1/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilercccs.html
    - shard-glk:          NOTRUN -> [SKIP][81] ([fdo#109271] / [i915#2672])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk9/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilercccs.html
    - shard-iclb:         NOTRUN -> [SKIP][82] ([i915#2587])
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb7/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilercccs.html
    - shard-apl:          NOTRUN -> [SKIP][83] ([fdo#109271] / [i915#2672])
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl8/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilercccs.html

  * igt@kms_flip_tiling@flip-yf-tiled:
    - shard-tglb:         NOTRUN -> [SKIP][84] ([fdo#111615])
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb1/igt@kms_flip_tiling@flip-yf-tiled.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-mmap-wc:
    - shard-apl:          NOTRUN -> [SKIP][85] ([fdo#109271]) +71 similar issues
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl1/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-mmap-wc.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-indfb-fliptrack:
    - shard-tglb:         NOTRUN -> [SKIP][86] ([fdo#111825]) +12 similar issues
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb2/igt@kms_frontbuffer_tracking@fbcpsr-2p-indfb-fliptrack.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-shrfb-fliptrack:
    - shard-iclb:         NOTRUN -> [SKIP][87] ([fdo#109280]) +16 similar issues
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb4/igt@kms_frontbuffer_tracking@fbcpsr-2p-shrfb-fliptrack.html

  * igt@kms_frontbuffer_tracking@psr-rgb565-draw-mmap-cpu:
    - shard-glk:          NOTRUN -> [SKIP][88] ([fdo#109271]) +50 similar issues
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk2/igt@kms_frontbuffer_tracking@psr-rgb565-draw-mmap-cpu.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-apl:          NOTRUN -> [FAIL][89] ([fdo#108145] / [i915#265])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl8/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html
    - shard-kbl:          NOTRUN -> [FAIL][90] ([fdo#108145] / [i915#265]) +1 similar issue
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl1/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html

  * igt@kms_plane_alpha_blend@pipe-d-constant-alpha-max:
    - shard-iclb:         NOTRUN -> [SKIP][91] ([fdo#109278]) +7 similar issues
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb5/igt@kms_plane_alpha_blend@pipe-d-constant-alpha-max.html

  * igt@kms_psr2_sf@plane-move-sf-dmg-area-3:
    - shard-kbl:          NOTRUN -> [SKIP][92] ([fdo#109271] / [i915#658]) +2 similar issues
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl1/igt@kms_psr2_sf@plane-move-sf-dmg-area-3.html

  * igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-4:
    - shard-iclb:         NOTRUN -> [SKIP][93] ([i915#658]) +1 similar issue
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb1/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-4.html
    - shard-glk:          NOTRUN -> [SKIP][94] ([fdo#109271] / [i915#658]) +1 similar issue
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk8/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-4.html
    - shard-apl:          NOTRUN -> [SKIP][95] ([fdo#109271] / [i915#658]) +1 similar issue
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl1/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-4.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [PASS][96] -> [SKIP][97] ([fdo#109642] / [fdo#111068] / [i915#658])
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb3/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@cursor_plane_onoff:
    - shard-hsw:          NOTRUN -> [SKIP][98] ([fdo#109271] / [i915#1072]) +6 similar issues
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-hsw1/igt@kms_psr@cursor_plane_onoff.html

  * igt@kms_psr@psr2_cursor_plane_move:
    - shard-iclb:         NOTRUN -> [SKIP][99] ([fdo#109441]) +2 similar issues
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb4/igt@kms_psr@psr2_cursor_plane_move.html

  * igt@kms_psr@psr2_sprite_render:
    - shard-iclb:         [PASS][100] -> [SKIP][101] ([fdo#109441])
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-iclb2/igt@kms_psr@psr2_sprite_render.html
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb4/igt@kms_psr@psr2_sprite_render.html

  * igt@kms_sysfs_edid_timing:
    - shard-apl:          NOTRUN -> [FAIL][102] ([IGT#2])
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl3/igt@kms_sysfs_edid_timing.html
    - shard-kbl:          NOTRUN -> [FAIL][103] ([IGT#2])
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl2/igt@kms_sysfs_edid_timing.html

  * igt@nouveau_crc@pipe-b-source-outp-inactive:
    - shard-iclb:         NOTRUN -> [SKIP][104] ([i915#2530])
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb4/igt@nouveau_crc@pipe-b-source-outp-inactive.html
    - shard-tglb:         NOTRUN -> [SKIP][105] ([i915#2530])
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb8/igt@nouveau_crc@pipe-b-source-outp-inactive.html

  * igt@perf@gen8-unprivileged-single-ctx-counters:
    - shard-tglb:         NOTRUN -> [SKIP][106] ([fdo#109289])
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb7/igt@perf@gen8-unprivileged-single-ctx-counters.html

  * igt@prime_nv_api@i915_nv_import_vs_close:
    - shard-tglb:         NOTRUN -> [SKIP][107] ([fdo#109291])
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb1/igt@prime_nv_api@i915_nv_import_vs_close.html

  * igt@prime_nv_api@nv_i915_reimport_twice_check_flink_name:
    - shard-iclb:         NOTRUN -> [SKIP][108] ([fdo#109291]) +1 similar issue
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb5/igt@prime_nv_api@nv_i915_reimport_twice_check_flink_name.html

  * igt@sysfs_heartbeat_interval@precise@vcs0:
    - shard-hsw:          NOTRUN -> [SKIP][109] ([fdo#109271]) +149 similar issues
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-hsw4/igt@sysfs_heartbeat_interval@precise@vcs0.html

  
#### Possible fixes ####

  * igt@api_intel_bb@render-ccs:
    - shard-tglb:         [INCOMPLETE][110] ([i915#2588]) -> [PASS][111]
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-tglb5/igt@api_intel_bb@render-ccs.html
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb6/igt@api_intel_bb@render-ccs.html

  * igt@gem_exec_fair@basic-deadline:
    - shard-glk:          [FAIL][112] ([i915#2846]) -> [PASS][113]
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-glk7/igt@gem_exec_fair@basic-deadline.html
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk5/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none@vcs0:
    - shard-kbl:          [FAIL][114] ([i915#2842]) -> [PASS][115]
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-kbl1/igt@gem_exec_fair@basic-none@vcs0.html
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl7/igt@gem_exec_fair@basic-none@vcs0.html

  * igt@gem_exec_fair@basic-none@vecs0:
    - shard-apl:          [FAIL][116] ([i915#2842]) -> [PASS][117]
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-apl7/igt@gem_exec_fair@basic-none@vecs0.html
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl4/igt@gem_exec_fair@basic-none@vecs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-tglb:         [FAIL][118] ([i915#2842]) -> [PASS][119] +1 similar issue
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-tglb7/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb5/igt@gem_exec_fair@basic-pace-share@rcs0.html
    - shard-glk:          [FAIL][120] ([i915#2842]) -> [PASS][121]
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-glk4/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk2/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_fair@basic-throttle@rcs0:
    - shard-iclb:         [FAIL][122] ([i915#2849]) -> [PASS][123]
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-iclb5/igt@gem_exec_fair@basic-throttle@rcs0.html
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb6/igt@gem_exec_fair@basic-throttle@rcs0.html

  * igt@gem_exec_schedule@u-fairslice@bcs0:
    - shard-iclb:         [DMESG-WARN][124] ([i915#2803]) -> [PASS][125]
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-iclb4/igt@gem_exec_schedule@u-fairslice@bcs0.html
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb3/igt@gem_exec_schedule@u-fairslice@bcs0.html

  * igt@gem_exec_schedule@u-fairslice@vcs0:
    - shard-apl:          [DMESG-WARN][126] ([i915#1610]) -> [PASS][127]
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-apl1/igt@gem_exec_schedule@u-fairslice@vcs0.html
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl6/igt@gem_exec_schedule@u-fairslice@vcs0.html
    - shard-kbl:          [DMESG-WARN][128] ([i915#1610] / [i915#2803]) -> [PASS][129]
   [128]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-kbl3/igt@gem_exec_schedule@u-fairslice@vcs0.html
   [129]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl4/igt@gem_exec_schedule@u-fairslice@vcs0.html

  * igt@gem_huc_copy@huc-copy:
    - shard-tglb:         [SKIP][130] ([i915#2190]) -> [PASS][131]
   [130]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-tglb6/igt@gem_huc_copy@huc-copy.html
   [131]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb1/igt@gem_huc_copy@huc-copy.html

  * igt@gem_softpin@noreloc-s3:
    - shard-kbl:          [INCOMPLETE][132] ([i915#2405]) -> [PASS][133]
   [132]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-kbl2/igt@gem_softpin@noreloc-s3.html
   [133]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl2/igt@gem_softpin@noreloc-s3.html

  * igt@gem_userptr_blits@huge-split:
    - shard-apl:          [INCOMPLETE][134] ([i915#2502]) -> [PASS][135]
   [134]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-apl3/igt@gem_userptr_blits@huge-split.html
   [135]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-apl8/igt@gem_userptr_blits@huge-split.html

  * igt@gem_workarounds@reset-context:
    - shard-snb:          [TIMEOUT][136] -> [PASS][137]
   [136]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-snb1/igt@gem_workarounds@reset-context.html
   [137]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-snb7/igt@gem_workarounds@reset-context.html

  * igt@i915_selftest@mock@requests:
    - shard-glk:          [INCOMPLETE][138] -> [PASS][139]
   [138]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-glk9/igt@i915_selftest@mock@requests.html
   [139]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-glk7/igt@i915_selftest@mock@requests.html

  * igt@kms_cursor_legacy@cursor-vs-flip-toggle:
    - shard-hsw:          [FAIL][140] ([i915#2370]) -> [PASS][141]
   [140]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-hsw1/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html
   [141]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-hsw1/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html

  * igt@kms_flip@flip-vs-expired-vblank@a-edp1:
    - shard-tglb:         [FAIL][142] ([i915#2598]) -> [PASS][143]
   [142]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-tglb1/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html
   [143]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-tglb3/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - shard-kbl:          [INCOMPLETE][144] ([i915#155] / [i915#2828]) -> [PASS][145]
   [144]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-kbl6/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html
   [145]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-kbl4/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [SKIP][146] ([fdo#109642] / [fdo#111068] / [i915#658]) -> [PASS][147]
   [146]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-iclb6/igt@kms_psr2_su@page_flip.html
   [147]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/shard-iclb2/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_suspend:
    - shard-iclb:         [SKIP][148] ([fdo#109441]) -> [PASS][149] +1 similar issue
   [148]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9684/shard-iclb6/igt@kms_psr@psr2_suspend.html
   [

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5432/index.html

[-- Attachment #1.2: Type: text/html, Size: 33650 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
  2021-01-26 22:04     ` [Intel-gfx] " Chris Wilson
@ 2021-01-27  9:12         ` Tvrtko Ursulin
  2021-01-27  9:12         ` [igt-dev] " Tvrtko Ursulin
  1 sibling, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2021-01-27  9:12 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 26/01/2021 22:04, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2021-01-26 16:47:14)
>> On 26/01/2021 13:05, Chris Wilson wrote:
>>> The client id used is a cyclic allocator as that reduces the likelihood
>>> of userspace seeing the same id used again (and so confusing the new
>>> client as the old). Verify that each new client has an id greater than
>>> the last.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> ---
>>>    tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
>>>    1 file changed, 108 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
>>> index a3a1f81e1..d2c1ebc5f 100644
>>> --- a/tests/i915/sysfs_clients.c
>>> +++ b/tests/i915/sysfs_clients.c
>>> @@ -8,6 +8,7 @@
>>>    #include <errno.h>
>>>    #include <fcntl.h>
>>>    #include <inttypes.h>
>>> +#include <limits.h>
>>>    #include <math.h>
>>>    #include <sched.h>
>>>    #include <sys/socket.h>
>>> @@ -47,6 +48,8 @@
>>>    #define assert_within_epsilon(x, ref, tolerance) \
>>>        __assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
>>>    
>>> +#define BUFSZ 280
>>> +
>>>    #define MI_BATCH_BUFFER_START (0x31 << 23)
>>>    #define MI_BATCH_BUFFER_END (0xa << 23)
>>>    #define MI_ARB_CHECK (0x5 << 23)
>>> @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
>>>    {
>>>        struct dirent *de;
>>>        int sv[2], rv[2];
>>> -     char buf[280];
>>> +     char buf[BUFSZ];
>>>        int me = -1;
>>>        long count;
>>>        pid_t pid;
>>> @@ -180,7 +183,7 @@ static long count_clients(int clients)
>>>    {
>>>        struct dirent *de;
>>>        long count = 0;
>>> -     char buf[280];
>>> +     char buf[BUFSZ];
>>>        DIR *dir;
>>>    
>>>        dir = fdopendir(dup(clients));
>>> @@ -229,32 +232,113 @@ static void create(int i915, int clients)
>>>        igt_assert_eq(count_clients(clients), 1);
>>>    }
>>>    
>>> +static const char *find_client(int clients, pid_t pid, char *buf)
>>> +{
>>> +     DIR *dir = fdopendir(dup(clients));
>>> +
>>> +     /* Reading a dir as it changes does not appear to be stable, SEP */
>>
>> Oh yes, it is POSIX undefined as far as I could figure out. You are
>> creating/destroying clients while reading the dir from different
>> threads?
> 
> Different processes on different directory fd. man readdir(3) does say
> that glibc is threadsafe, but that is not a requirement of POSIX. The
> problem we are seeing is that the directory contents are not stable
> around the readdir loop as clients are being created/destroyed, causing
> dirents to be missed.
> 
> stracing the problem shows that glibc used a 32KiB buffer for getdents
> and only a single syscall was required to grab the entire directory. No
> errors were seen before the missed dirent. It just seemed to be missing.
> 
> Afaict there is no delay in creating the sysfs entry, and I think there
> should also be no delay in creating the kernfs node and inodes, so my
> initial assumption is that it's something not quite happy in the
> kernfs directory that shows up under stress. A second loop ran for a
> long time without incident locally, but CI seems much more adept at
> finding it.

I think we are talking about the same thing..

>> I think best might be to introduce explicit sync points between
>> those two entities to make this reliable. In another review for the same
>> problem I suggested pipes for implementing this handshake. Along the
>> lines of "let child open/close some processes, make it wait for parent;
>> parent scans sysfs, signals child to open/close some more; repeat for a
>> while".
> 
> Sadly, I don't expect userspace to do that :)
> 
> I do expect userspace to search for its own client/ upon creation, and I
> expect there to be many clients being opened at once. So I think so long
> as the use of the library readdir is correct (distinct processes with
> distinct directory fd, so I think there's no accidental sharing) this
> represents. Hmm. fsync(dirfd).

...and I think it is just not guaranteed that reading the dir in 
parallel to dentries being created/deleted is guaranteed to work (no 
guarantee for discovering newly addded or deleted clients since the 
iteration started). Or you are saying that a client opens itself starts 
readdir loop and then fails to find itself?

Otherwise, I was thinking, if we wanted to allow clients to inspect 
themselves, we should really add a getrusage(2) like ioctl.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
@ 2021-01-27  9:12         ` Tvrtko Ursulin
  0 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2021-01-27  9:12 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 26/01/2021 22:04, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2021-01-26 16:47:14)
>> On 26/01/2021 13:05, Chris Wilson wrote:
>>> The client id used is a cyclic allocator as that reduces the likelihood
>>> of userspace seeing the same id used again (and so confusing the new
>>> client as the old). Verify that each new client has an id greater than
>>> the last.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> ---
>>>    tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
>>>    1 file changed, 108 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
>>> index a3a1f81e1..d2c1ebc5f 100644
>>> --- a/tests/i915/sysfs_clients.c
>>> +++ b/tests/i915/sysfs_clients.c
>>> @@ -8,6 +8,7 @@
>>>    #include <errno.h>
>>>    #include <fcntl.h>
>>>    #include <inttypes.h>
>>> +#include <limits.h>
>>>    #include <math.h>
>>>    #include <sched.h>
>>>    #include <sys/socket.h>
>>> @@ -47,6 +48,8 @@
>>>    #define assert_within_epsilon(x, ref, tolerance) \
>>>        __assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
>>>    
>>> +#define BUFSZ 280
>>> +
>>>    #define MI_BATCH_BUFFER_START (0x31 << 23)
>>>    #define MI_BATCH_BUFFER_END (0xa << 23)
>>>    #define MI_ARB_CHECK (0x5 << 23)
>>> @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
>>>    {
>>>        struct dirent *de;
>>>        int sv[2], rv[2];
>>> -     char buf[280];
>>> +     char buf[BUFSZ];
>>>        int me = -1;
>>>        long count;
>>>        pid_t pid;
>>> @@ -180,7 +183,7 @@ static long count_clients(int clients)
>>>    {
>>>        struct dirent *de;
>>>        long count = 0;
>>> -     char buf[280];
>>> +     char buf[BUFSZ];
>>>        DIR *dir;
>>>    
>>>        dir = fdopendir(dup(clients));
>>> @@ -229,32 +232,113 @@ static void create(int i915, int clients)
>>>        igt_assert_eq(count_clients(clients), 1);
>>>    }
>>>    
>>> +static const char *find_client(int clients, pid_t pid, char *buf)
>>> +{
>>> +     DIR *dir = fdopendir(dup(clients));
>>> +
>>> +     /* Reading a dir as it changes does not appear to be stable, SEP */
>>
>> Oh yes, it is POSIX undefined as far as I could figure out. You are
>> creating/destroying clients while reading the dir from different
>> threads?
> 
> Different processes on different directory fd. man readdir(3) does say
> that glibc is threadsafe, but that is not a requirement of POSIX. The
> problem we are seeing is that the directory contents are not stable
> around the readdir loop as clients are being created/destroyed, causing
> dirents to be missed.
> 
> stracing the problem shows that glibc used a 32KiB buffer for getdents
> and only a single syscall was required to grab the entire directory. No
> errors were seen before the missed dirent. It just seemed to be missing.
> 
> Afaict there is no delay in creating the sysfs entry, and I think there
> should also be no delay in creating the kernfs node and inodes, so my
> initial assumption is that it's something not quite happy in the
> kernfs directory that shows up under stress. A second loop ran for a
> long time without incident locally, but CI seems much more adept at
> finding it.

I think we are talking about the same thing..

>> I think best might be to introduce explicit sync points between
>> those two entities to make this reliable. In another review for the same
>> problem I suggested pipes for implementing this handshake. Along the
>> lines of "let child open/close some processes, make it wait for parent;
>> parent scans sysfs, signals child to open/close some more; repeat for a
>> while".
> 
> Sadly, I don't expect userspace to do that :)
> 
> I do expect userspace to search for its own client/ upon creation, and I
> expect there to be many clients being opened at once. So I think so long
> as the use of the library readdir is correct (distinct processes with
> distinct directory fd, so I think there's no accidental sharing) this
> represents. Hmm. fsync(dirfd).

...and I think it is just not guaranteed that reading the dir in 
parallel to dentries being created/deleted is guaranteed to work (no 
guarantee for discovering newly addded or deleted clients since the 
iteration started). Or you are saying that a client opens itself starts 
readdir loop and then fails to find itself?

Otherwise, I was thinking, if we wanted to allow clients to inspect 
themselves, we should really add a getrusage(2) like ioctl.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
  2021-01-27  9:12         ` [igt-dev] " Tvrtko Ursulin
@ 2021-01-27  9:51           ` Chris Wilson
  -1 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-27  9:51 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2021-01-27 09:12:00)
> 
> On 26/01/2021 22:04, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2021-01-26 16:47:14)
> >> On 26/01/2021 13:05, Chris Wilson wrote:
> >>> The client id used is a cyclic allocator as that reduces the likelihood
> >>> of userspace seeing the same id used again (and so confusing the new
> >>> client as the old). Verify that each new client has an id greater than
> >>> the last.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >>> ---
> >>>    tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
> >>>    1 file changed, 108 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
> >>> index a3a1f81e1..d2c1ebc5f 100644
> >>> --- a/tests/i915/sysfs_clients.c
> >>> +++ b/tests/i915/sysfs_clients.c
> >>> @@ -8,6 +8,7 @@
> >>>    #include <errno.h>
> >>>    #include <fcntl.h>
> >>>    #include <inttypes.h>
> >>> +#include <limits.h>
> >>>    #include <math.h>
> >>>    #include <sched.h>
> >>>    #include <sys/socket.h>
> >>> @@ -47,6 +48,8 @@
> >>>    #define assert_within_epsilon(x, ref, tolerance) \
> >>>        __assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
> >>>    
> >>> +#define BUFSZ 280
> >>> +
> >>>    #define MI_BATCH_BUFFER_START (0x31 << 23)
> >>>    #define MI_BATCH_BUFFER_END (0xa << 23)
> >>>    #define MI_ARB_CHECK (0x5 << 23)
> >>> @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
> >>>    {
> >>>        struct dirent *de;
> >>>        int sv[2], rv[2];
> >>> -     char buf[280];
> >>> +     char buf[BUFSZ];
> >>>        int me = -1;
> >>>        long count;
> >>>        pid_t pid;
> >>> @@ -180,7 +183,7 @@ static long count_clients(int clients)
> >>>    {
> >>>        struct dirent *de;
> >>>        long count = 0;
> >>> -     char buf[280];
> >>> +     char buf[BUFSZ];
> >>>        DIR *dir;
> >>>    
> >>>        dir = fdopendir(dup(clients));
> >>> @@ -229,32 +232,113 @@ static void create(int i915, int clients)
> >>>        igt_assert_eq(count_clients(clients), 1);
> >>>    }
> >>>    
> >>> +static const char *find_client(int clients, pid_t pid, char *buf)
> >>> +{
> >>> +     DIR *dir = fdopendir(dup(clients));
> >>> +
> >>> +     /* Reading a dir as it changes does not appear to be stable, SEP */
> >>
> >> Oh yes, it is POSIX undefined as far as I could figure out. You are
> >> creating/destroying clients while reading the dir from different
> >> threads?
> > 
> > Different processes on different directory fd. man readdir(3) does say
> > that glibc is threadsafe, but that is not a requirement of POSIX. The
> > problem we are seeing is that the directory contents are not stable
> > around the readdir loop as clients are being created/destroyed, causing
> > dirents to be missed.
> > 
> > stracing the problem shows that glibc used a 32KiB buffer for getdents
> > and only a single syscall was required to grab the entire directory. No
> > errors were seen before the missed dirent. It just seemed to be missing.
> > 
> > Afaict there is no delay in creating the sysfs entry, and I think there
> > should also be no delay in creating the kernfs node and inodes, so my
> > initial assumption is that it's something not quite happy in the
> > kernfs directory that shows up under stress. A second loop ran for a
> > long time without incident locally, but CI seems much more adept at
> > finding it.
> 
> I think we are talking about the same thing..

The manpages only talked about the thread-safety of the library readdir,
and since you mentioned threads not processes, I was hopeful.

> >> I think best might be to introduce explicit sync points between
> >> those two entities to make this reliable. In another review for the same
> >> problem I suggested pipes for implementing this handshake. Along the
> >> lines of "let child open/close some processes, make it wait for parent;
> >> parent scans sysfs, signals child to open/close some more; repeat for a
> >> while".
> > 
> > Sadly, I don't expect userspace to do that :)
> > 
> > I do expect userspace to search for its own client/ upon creation, and I
> > expect there to be many clients being opened at once. So I think so long
> > as the use of the library readdir is correct (distinct processes with
> > distinct directory fd, so I think there's no accidental sharing) this
> > represents. Hmm. fsync(dirfd).
> 
> ...and I think it is just not guaranteed that reading the dir in 
> parallel to dentries being created/deleted is guaranteed to work (no 
> guarantee for discovering newly addded or deleted clients since the 
> iteration started). Or you are saying that a client opens itself starts 
> readdir loop and then fails to find itself?

That's exactly what happens -- but with multiple concurrent processes.
(Needs to run with one process to double check).

device = gem_reopen_driver(i915);
DIR fdopendir(open("/sysfs/clients", O_DIRECTORY))
rewinddir();
while (readdir) ...
=> this pid not found
	
> Otherwise, I was thinking, if we wanted to allow clients to inspect 
> themselves, we should really add a getrusage(2) like ioctl.

Maybe, but I've yet to see something that says a directory operation
prior to a opendir/rewinddir will not be visible in readdir. POSIX says
that directory operations in parallel to the readdir, whether or not
those are visible in the next readdir is undefined, but it seems to be
clear on operations prior to opening the directory.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
@ 2021-01-27  9:51           ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-27  9:51 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2021-01-27 09:12:00)
> 
> On 26/01/2021 22:04, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2021-01-26 16:47:14)
> >> On 26/01/2021 13:05, Chris Wilson wrote:
> >>> The client id used is a cyclic allocator as that reduces the likelihood
> >>> of userspace seeing the same id used again (and so confusing the new
> >>> client as the old). Verify that each new client has an id greater than
> >>> the last.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >>> ---
> >>>    tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
> >>>    1 file changed, 108 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
> >>> index a3a1f81e1..d2c1ebc5f 100644
> >>> --- a/tests/i915/sysfs_clients.c
> >>> +++ b/tests/i915/sysfs_clients.c
> >>> @@ -8,6 +8,7 @@
> >>>    #include <errno.h>
> >>>    #include <fcntl.h>
> >>>    #include <inttypes.h>
> >>> +#include <limits.h>
> >>>    #include <math.h>
> >>>    #include <sched.h>
> >>>    #include <sys/socket.h>
> >>> @@ -47,6 +48,8 @@
> >>>    #define assert_within_epsilon(x, ref, tolerance) \
> >>>        __assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
> >>>    
> >>> +#define BUFSZ 280
> >>> +
> >>>    #define MI_BATCH_BUFFER_START (0x31 << 23)
> >>>    #define MI_BATCH_BUFFER_END (0xa << 23)
> >>>    #define MI_ARB_CHECK (0x5 << 23)
> >>> @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
> >>>    {
> >>>        struct dirent *de;
> >>>        int sv[2], rv[2];
> >>> -     char buf[280];
> >>> +     char buf[BUFSZ];
> >>>        int me = -1;
> >>>        long count;
> >>>        pid_t pid;
> >>> @@ -180,7 +183,7 @@ static long count_clients(int clients)
> >>>    {
> >>>        struct dirent *de;
> >>>        long count = 0;
> >>> -     char buf[280];
> >>> +     char buf[BUFSZ];
> >>>        DIR *dir;
> >>>    
> >>>        dir = fdopendir(dup(clients));
> >>> @@ -229,32 +232,113 @@ static void create(int i915, int clients)
> >>>        igt_assert_eq(count_clients(clients), 1);
> >>>    }
> >>>    
> >>> +static const char *find_client(int clients, pid_t pid, char *buf)
> >>> +{
> >>> +     DIR *dir = fdopendir(dup(clients));
> >>> +
> >>> +     /* Reading a dir as it changes does not appear to be stable, SEP */
> >>
> >> Oh yes, it is POSIX undefined as far as I could figure out. You are
> >> creating/destroying clients while reading the dir from different
> >> threads?
> > 
> > Different processes on different directory fd. man readdir(3) does say
> > that glibc is threadsafe, but that is not a requirement of POSIX. The
> > problem we are seeing is that the directory contents are not stable
> > around the readdir loop as clients are being created/destroyed, causing
> > dirents to be missed.
> > 
> > stracing the problem shows that glibc used a 32KiB buffer for getdents
> > and only a single syscall was required to grab the entire directory. No
> > errors were seen before the missed dirent. It just seemed to be missing.
> > 
> > Afaict there is no delay in creating the sysfs entry, and I think there
> > should also be no delay in creating the kernfs node and inodes, so my
> > initial assumption is that it's something not quite happy in the
> > kernfs directory that shows up under stress. A second loop ran for a
> > long time without incident locally, but CI seems much more adept at
> > finding it.
> 
> I think we are talking about the same thing..

The manpages only talked about the thread-safety of the library readdir,
and since you mentioned threads not processes, I was hopeful.

> >> I think best might be to introduce explicit sync points between
> >> those two entities to make this reliable. In another review for the same
> >> problem I suggested pipes for implementing this handshake. Along the
> >> lines of "let child open/close some processes, make it wait for parent;
> >> parent scans sysfs, signals child to open/close some more; repeat for a
> >> while".
> > 
> > Sadly, I don't expect userspace to do that :)
> > 
> > I do expect userspace to search for its own client/ upon creation, and I
> > expect there to be many clients being opened at once. So I think so long
> > as the use of the library readdir is correct (distinct processes with
> > distinct directory fd, so I think there's no accidental sharing) this
> > represents. Hmm. fsync(dirfd).
> 
> ...and I think it is just not guaranteed that reading the dir in 
> parallel to dentries being created/deleted is guaranteed to work (no 
> guarantee for discovering newly addded or deleted clients since the 
> iteration started). Or you are saying that a client opens itself starts 
> readdir loop and then fails to find itself?

That's exactly what happens -- but with multiple concurrent processes.
(Needs to run with one process to double check).

device = gem_reopen_driver(i915);
DIR fdopendir(open("/sysfs/clients", O_DIRECTORY))
rewinddir();
while (readdir) ...
=> this pid not found
	
> Otherwise, I was thinking, if we wanted to allow clients to inspect 
> themselves, we should really add a getrusage(2) like ioctl.

Maybe, but I've yet to see something that says a directory operation
prior to a opendir/rewinddir will not be visible in readdir. POSIX says
that directory operations in parallel to the readdir, whether or not
those are visible in the next readdir is undefined, but it seems to be
clear on operations prior to opening the directory.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
  2021-01-26 13:05   ` [igt-dev] " Chris Wilson
  (?)
  (?)
@ 2021-01-29  9:18   ` Tvrtko Ursulin
  2021-01-29  9:52       ` [igt-dev] " Chris Wilson
  -1 siblings, 1 reply; 22+ messages in thread
From: Tvrtko Ursulin @ 2021-01-29  9:18 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev



On 26/01/2021 13:05, Chris Wilson wrote:
> The client id used is a cyclic allocator as that reduces the likelihood
> of userspace seeing the same id used again (and so confusing the new
> client as the old). Verify that each new client has an id greater than
> the last.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
>   1 file changed, 108 insertions(+), 21 deletions(-)
> 
> diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
> index a3a1f81e1..d2c1ebc5f 100644
> --- a/tests/i915/sysfs_clients.c
> +++ b/tests/i915/sysfs_clients.c
> @@ -8,6 +8,7 @@
>   #include <errno.h>
>   #include <fcntl.h>
>   #include <inttypes.h>
> +#include <limits.h>
>   #include <math.h>
>   #include <sched.h>
>   #include <sys/socket.h>
> @@ -47,6 +48,8 @@
>   #define assert_within_epsilon(x, ref, tolerance) \
>   	__assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
>   
> +#define BUFSZ 280
> +
>   #define MI_BATCH_BUFFER_START (0x31 << 23)
>   #define MI_BATCH_BUFFER_END (0xa << 23)
>   #define MI_ARB_CHECK (0x5 << 23)
> @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
>   {
>   	struct dirent *de;
>   	int sv[2], rv[2];
> -	char buf[280];
> +	char buf[BUFSZ];
>   	int me = -1;
>   	long count;
>   	pid_t pid;
> @@ -180,7 +183,7 @@ static long count_clients(int clients)
>   {
>   	struct dirent *de;
>   	long count = 0;
> -	char buf[280];
> +	char buf[BUFSZ];
>   	DIR *dir;
>   
>   	dir = fdopendir(dup(clients));
> @@ -229,32 +232,113 @@ static void create(int i915, int clients)
>   	igt_assert_eq(count_clients(clients), 1);
>   }
>   
> +static const char *find_client(int clients, pid_t pid, char *buf)
> +{
> +	DIR *dir = fdopendir(dup(clients));
> +
> +	/* Reading a dir as it changes does not appear to be stable, SEP */
> +	for (int pass = 0; pass < 2; pass++) {
> +		struct dirent *de;
> +
> +		rewinddir(dir);
> +		while ((de = readdir(dir))) {
> +			if (!isdigit(de->d_name[0]))
> +				continue;
> +
> +			snprintf(buf, BUFSZ, "%s/pid", de->d_name);
> +			igt_sysfs_read(clients, buf, buf, sizeof(buf));
> +			if (atoi(buf) != pid)
> +				continue;
> +
> +			strncpy(buf, de->d_name, BUFSZ);
> +			goto out;
> +		}
> +	}
> +	*buf = '\0';
> +out:
> +	closedir(dir);
> +	return buf;
> +}
> +
>   static int find_me(int clients, pid_t pid)
>   {
> -	struct dirent *de;
> -	char buf[280];
> -	int me = -1;
> -	DIR *dir;
> +	char buf[BUFSZ];
>   
> -	dir = fdopendir(dup(clients));
> -	igt_assert(dir);
> -	rewinddir(dir);
> +	return openat(clients,
> +		      find_client(clients, pid, buf),
> +		      O_DIRECTORY | O_RDONLY);
> +}
>   
> -	while ((de = readdir(dir))) {
> -		if (!isdigit(de->d_name[0]))
> -			continue;
> +static int reopen_directory(int fd)
> +{
> +	char buf[BUFSZ];
> +	int dir;
>   
> -		snprintf(buf, sizeof(buf), "%s/pid", de->d_name);
> -		igt_sysfs_read(clients, buf, buf, sizeof(buf));
> -		if (atoi(buf) != pid)
> -			continue;
> +	snprintf(buf, sizeof(buf), "/proc/self/fd/%d", fd);
> +	dir = open(buf, O_RDONLY);

Maybe O_DIRECTORY if it is open_directory.

> +	igt_assert_fd(dir);
>   
> -		me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
> -		break;
> +	return dir;
> +}
> +
> +static unsigned int my_id(int clients, pid_t pid)
> +{
> +	char buf[BUFSZ];
> +
> +	return atoi(find_client(clients, pid, buf));
> +}
> +
> +static unsigned int recycle_client(int i915, int clients)
> +{
> +	int device, client;
> +
> +	device = gem_reopen_driver(i915);
> +	client = my_id(clients, getpid());
> +	close(device);
> +
> +	return client;
> +}
> +
> +static void recycle(int i915, int clients)
> +{
> +	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> +
> +	/*
> +	 * As we open and close clients, we do not expect to reuse old ids,
> +	 * i.e. we use a cyclic ida. This reduces the likelihood of userspace
> +	 * watchers becoming confused and mistaking the new client as a
> +	 * continuation of the old.
> +	 */
> +	igt_require(my_id(clients, getpid()) < INT_MAX / 2);

Hm this is a bit dodgy - it will cause "permanent" skips if running the 
test in a loop. Just for the client > last assert below? I guess it is 
hard to handle wrap with forked clients.

> +	igt_assert(my_id(clients, getpid()));
> +
> +	igt_fork(child, 2 * ncpus) {
> +		unsigned int client, last;
> +
> +		/* Reopen the directory fd for each client */
> +		clients = reopen_directory(clients);
> +
> +		last = recycle_client(i915, clients);
> +		igt_info("Child[%d] first client:%d\n", getpid(), last);
> +		igt_until_timeout(5) {
> +			client = recycle_client(i915, clients);
> +
> +			/* In 5 seconds we are not going to exhaust the ids */
> +			igt_assert(client > last);
> +			last = client;
> +		}
> +		igt_info("Child[%d] last client:%d\n", getpid(), last);
>   	}
> +	igt_waitchildren();
>   
> -	closedir(dir);
> -	return me;
> +	/* Cleanup delayed behind rcu */
> +	igt_until_timeout(30) {
> +		sched_yield();
> +		if (count_clients(clients) == 1)
> +			break;
> +		usleep(10000);
> +	}
> +	igt_assert_eq(count_clients(clients), 1);
>   }
>   
>   static int64_t read_runtime(int client, int class)
> @@ -719,7 +803,7 @@ sema(int i915, int clients, const struct intel_execution_engine2 *e, int f)
>   static int read_all(int clients, pid_t pid, int class, uint64_t *runtime)
>   {
>   	struct dirent *de;
> -	char buf[280];
> +	char buf[BUFSZ];
>   	int count = 0;
>   	DIR *dir;
>   
> @@ -958,6 +1042,9 @@ igt_main
>   	igt_subtest("create")
>   		create(i915, clients);
>   
> +	igt_subtest("recycle")
> +		recycle(i915, clients);
> +
>   	igt_subtest_group
>   		test_busy(i915, clients);
>   
> 

Okay better than nothing.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
  2021-01-29  9:18   ` [Intel-gfx] " Tvrtko Ursulin
@ 2021-01-29  9:52       ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-29  9:52 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2021-01-29 09:18:50)
> 
> 
> On 26/01/2021 13:05, Chris Wilson wrote:
> > The client id used is a cyclic allocator as that reduces the likelihood
> > of userspace seeing the same id used again (and so confusing the new
> > client as the old). Verify that each new client has an id greater than
> > the last.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > ---
> >   tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
> >   1 file changed, 108 insertions(+), 21 deletions(-)
> > 
> > diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
> > index a3a1f81e1..d2c1ebc5f 100644
> > --- a/tests/i915/sysfs_clients.c
> > +++ b/tests/i915/sysfs_clients.c
> > @@ -8,6 +8,7 @@
> >   #include <errno.h>
> >   #include <fcntl.h>
> >   #include <inttypes.h>
> > +#include <limits.h>
> >   #include <math.h>
> >   #include <sched.h>
> >   #include <sys/socket.h>
> > @@ -47,6 +48,8 @@
> >   #define assert_within_epsilon(x, ref, tolerance) \
> >       __assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
> >   
> > +#define BUFSZ 280
> > +
> >   #define MI_BATCH_BUFFER_START (0x31 << 23)
> >   #define MI_BATCH_BUFFER_END (0xa << 23)
> >   #define MI_ARB_CHECK (0x5 << 23)
> > @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
> >   {
> >       struct dirent *de;
> >       int sv[2], rv[2];
> > -     char buf[280];
> > +     char buf[BUFSZ];
> >       int me = -1;
> >       long count;
> >       pid_t pid;
> > @@ -180,7 +183,7 @@ static long count_clients(int clients)
> >   {
> >       struct dirent *de;
> >       long count = 0;
> > -     char buf[280];
> > +     char buf[BUFSZ];
> >       DIR *dir;
> >   
> >       dir = fdopendir(dup(clients));
> > @@ -229,32 +232,113 @@ static void create(int i915, int clients)
> >       igt_assert_eq(count_clients(clients), 1);
> >   }
> >   
> > +static const char *find_client(int clients, pid_t pid, char *buf)
> > +{
> > +     DIR *dir = fdopendir(dup(clients));
> > +
> > +     /* Reading a dir as it changes does not appear to be stable, SEP */
> > +     for (int pass = 0; pass < 2; pass++) {
> > +             struct dirent *de;
> > +
> > +             rewinddir(dir);
> > +             while ((de = readdir(dir))) {
> > +                     if (!isdigit(de->d_name[0]))
> > +                             continue;
> > +
> > +                     snprintf(buf, BUFSZ, "%s/pid", de->d_name);
> > +                     igt_sysfs_read(clients, buf, buf, sizeof(buf));
> > +                     if (atoi(buf) != pid)
> > +                             continue;
> > +
> > +                     strncpy(buf, de->d_name, BUFSZ);
> > +                     goto out;
> > +             }
> > +     }
> > +     *buf = '\0';
> > +out:
> > +     closedir(dir);
> > +     return buf;
> > +}
> > +
> >   static int find_me(int clients, pid_t pid)
> >   {
> > -     struct dirent *de;
> > -     char buf[280];
> > -     int me = -1;
> > -     DIR *dir;
> > +     char buf[BUFSZ];
> >   
> > -     dir = fdopendir(dup(clients));
> > -     igt_assert(dir);
> > -     rewinddir(dir);
> > +     return openat(clients,
> > +                   find_client(clients, pid, buf),
> > +                   O_DIRECTORY | O_RDONLY);
> > +}
> >   
> > -     while ((de = readdir(dir))) {
> > -             if (!isdigit(de->d_name[0]))
> > -                     continue;
> > +static int reopen_directory(int fd)
> > +{
> > +     char buf[BUFSZ];
> > +     int dir;
> >   
> > -             snprintf(buf, sizeof(buf), "%s/pid", de->d_name);
> > -             igt_sysfs_read(clients, buf, buf, sizeof(buf));
> > -             if (atoi(buf) != pid)
> > -                     continue;
> > +     snprintf(buf, sizeof(buf), "/proc/self/fd/%d", fd);
> > +     dir = open(buf, O_RDONLY);
> 
> Maybe O_DIRECTORY if it is open_directory.
> 
> > +     igt_assert_fd(dir);
> >   
> > -             me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
> > -             break;
> > +     return dir;
> > +}
> > +
> > +static unsigned int my_id(int clients, pid_t pid)
> > +{
> > +     char buf[BUFSZ];
> > +
> > +     return atoi(find_client(clients, pid, buf));
> > +}
> > +
> > +static unsigned int recycle_client(int i915, int clients)
> > +{
> > +     int device, client;
> > +
> > +     device = gem_reopen_driver(i915);
> > +     client = my_id(clients, getpid());
> > +     close(device);
> > +
> > +     return client;
> > +}
> > +
> > +static void recycle(int i915, int clients)
> > +{
> > +     const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> > +
> > +     /*
> > +      * As we open and close clients, we do not expect to reuse old ids,
> > +      * i.e. we use a cyclic ida. This reduces the likelihood of userspace
> > +      * watchers becoming confused and mistaking the new client as a
> > +      * continuation of the old.
> > +      */
> > +     igt_require(my_id(clients, getpid()) < INT_MAX / 2);
> 
> Hm this is a bit dodgy - it will cause "permanent" skips if running the 
> test in a loop. Just for the client > last assert below? I guess it is 
> hard to handle wrap with forked clients.

It takes about a day to reach 2 billion ids on a fast machine. For CI, I
think we are safe.

We could do the (int)(A - B) > 0 to handle the wrap, that would be more
sensible.

[...]

> Okay better than nothing.

But first we need to resolve the failure to find itself. :(
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
@ 2021-01-29  9:52       ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-29  9:52 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2021-01-29 09:18:50)
> 
> 
> On 26/01/2021 13:05, Chris Wilson wrote:
> > The client id used is a cyclic allocator as that reduces the likelihood
> > of userspace seeing the same id used again (and so confusing the new
> > client as the old). Verify that each new client has an id greater than
> > the last.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > ---
> >   tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
> >   1 file changed, 108 insertions(+), 21 deletions(-)
> > 
> > diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
> > index a3a1f81e1..d2c1ebc5f 100644
> > --- a/tests/i915/sysfs_clients.c
> > +++ b/tests/i915/sysfs_clients.c
> > @@ -8,6 +8,7 @@
> >   #include <errno.h>
> >   #include <fcntl.h>
> >   #include <inttypes.h>
> > +#include <limits.h>
> >   #include <math.h>
> >   #include <sched.h>
> >   #include <sys/socket.h>
> > @@ -47,6 +48,8 @@
> >   #define assert_within_epsilon(x, ref, tolerance) \
> >       __assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
> >   
> > +#define BUFSZ 280
> > +
> >   #define MI_BATCH_BUFFER_START (0x31 << 23)
> >   #define MI_BATCH_BUFFER_END (0xa << 23)
> >   #define MI_ARB_CHECK (0x5 << 23)
> > @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
> >   {
> >       struct dirent *de;
> >       int sv[2], rv[2];
> > -     char buf[280];
> > +     char buf[BUFSZ];
> >       int me = -1;
> >       long count;
> >       pid_t pid;
> > @@ -180,7 +183,7 @@ static long count_clients(int clients)
> >   {
> >       struct dirent *de;
> >       long count = 0;
> > -     char buf[280];
> > +     char buf[BUFSZ];
> >       DIR *dir;
> >   
> >       dir = fdopendir(dup(clients));
> > @@ -229,32 +232,113 @@ static void create(int i915, int clients)
> >       igt_assert_eq(count_clients(clients), 1);
> >   }
> >   
> > +static const char *find_client(int clients, pid_t pid, char *buf)
> > +{
> > +     DIR *dir = fdopendir(dup(clients));
> > +
> > +     /* Reading a dir as it changes does not appear to be stable, SEP */
> > +     for (int pass = 0; pass < 2; pass++) {
> > +             struct dirent *de;
> > +
> > +             rewinddir(dir);
> > +             while ((de = readdir(dir))) {
> > +                     if (!isdigit(de->d_name[0]))
> > +                             continue;
> > +
> > +                     snprintf(buf, BUFSZ, "%s/pid", de->d_name);
> > +                     igt_sysfs_read(clients, buf, buf, sizeof(buf));
> > +                     if (atoi(buf) != pid)
> > +                             continue;
> > +
> > +                     strncpy(buf, de->d_name, BUFSZ);
> > +                     goto out;
> > +             }
> > +     }
> > +     *buf = '\0';
> > +out:
> > +     closedir(dir);
> > +     return buf;
> > +}
> > +
> >   static int find_me(int clients, pid_t pid)
> >   {
> > -     struct dirent *de;
> > -     char buf[280];
> > -     int me = -1;
> > -     DIR *dir;
> > +     char buf[BUFSZ];
> >   
> > -     dir = fdopendir(dup(clients));
> > -     igt_assert(dir);
> > -     rewinddir(dir);
> > +     return openat(clients,
> > +                   find_client(clients, pid, buf),
> > +                   O_DIRECTORY | O_RDONLY);
> > +}
> >   
> > -     while ((de = readdir(dir))) {
> > -             if (!isdigit(de->d_name[0]))
> > -                     continue;
> > +static int reopen_directory(int fd)
> > +{
> > +     char buf[BUFSZ];
> > +     int dir;
> >   
> > -             snprintf(buf, sizeof(buf), "%s/pid", de->d_name);
> > -             igt_sysfs_read(clients, buf, buf, sizeof(buf));
> > -             if (atoi(buf) != pid)
> > -                     continue;
> > +     snprintf(buf, sizeof(buf), "/proc/self/fd/%d", fd);
> > +     dir = open(buf, O_RDONLY);
> 
> Maybe O_DIRECTORY if it is open_directory.
> 
> > +     igt_assert_fd(dir);
> >   
> > -             me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
> > -             break;
> > +     return dir;
> > +}
> > +
> > +static unsigned int my_id(int clients, pid_t pid)
> > +{
> > +     char buf[BUFSZ];
> > +
> > +     return atoi(find_client(clients, pid, buf));
> > +}
> > +
> > +static unsigned int recycle_client(int i915, int clients)
> > +{
> > +     int device, client;
> > +
> > +     device = gem_reopen_driver(i915);
> > +     client = my_id(clients, getpid());
> > +     close(device);
> > +
> > +     return client;
> > +}
> > +
> > +static void recycle(int i915, int clients)
> > +{
> > +     const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> > +
> > +     /*
> > +      * As we open and close clients, we do not expect to reuse old ids,
> > +      * i.e. we use a cyclic ida. This reduces the likelihood of userspace
> > +      * watchers becoming confused and mistaking the new client as a
> > +      * continuation of the old.
> > +      */
> > +     igt_require(my_id(clients, getpid()) < INT_MAX / 2);
> 
> Hm this is a bit dodgy - it will cause "permanent" skips if running the 
> test in a loop. Just for the client > last assert below? I guess it is 
> hard to handle wrap with forked clients.

It takes about a day to reach 2 billion ids on a fast machine. For CI, I
think we are safe.

We could do the (int)(A - B) > 0 to handle the wrap, that would be more
sensible.

[...]

> Okay better than nothing.

But first we need to resolve the failure to find itself. :(
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
  2021-01-29  9:52       ` [igt-dev] " Chris Wilson
  (?)
@ 2021-01-29 18:30       ` Tvrtko Ursulin
  -1 siblings, 0 replies; 22+ messages in thread
From: Tvrtko Ursulin @ 2021-01-29 18:30 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 29/01/2021 09:52, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2021-01-29 09:18:50)
>> On 26/01/2021 13:05, Chris Wilson wrote:
>>> The client id used is a cyclic allocator as that reduces the likelihood
>>> of userspace seeing the same id used again (and so confusing the new
>>> client as the old). Verify that each new client has an id greater than
>>> the last.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> ---
>>>    tests/i915/sysfs_clients.c | 129 +++++++++++++++++++++++++++++++------
>>>    1 file changed, 108 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
>>> index a3a1f81e1..d2c1ebc5f 100644
>>> --- a/tests/i915/sysfs_clients.c
>>> +++ b/tests/i915/sysfs_clients.c
>>> @@ -8,6 +8,7 @@
>>>    #include <errno.h>
>>>    #include <fcntl.h>
>>>    #include <inttypes.h>
>>> +#include <limits.h>
>>>    #include <math.h>
>>>    #include <sched.h>
>>>    #include <sys/socket.h>
>>> @@ -47,6 +48,8 @@
>>>    #define assert_within_epsilon(x, ref, tolerance) \
>>>        __assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
>>>    
>>> +#define BUFSZ 280
>>> +
>>>    #define MI_BATCH_BUFFER_START (0x31 << 23)
>>>    #define MI_BATCH_BUFFER_END (0xa << 23)
>>>    #define MI_ARB_CHECK (0x5 << 23)
>>> @@ -75,7 +78,7 @@ static void pidname(int i915, int clients)
>>>    {
>>>        struct dirent *de;
>>>        int sv[2], rv[2];
>>> -     char buf[280];
>>> +     char buf[BUFSZ];
>>>        int me = -1;
>>>        long count;
>>>        pid_t pid;
>>> @@ -180,7 +183,7 @@ static long count_clients(int clients)
>>>    {
>>>        struct dirent *de;
>>>        long count = 0;
>>> -     char buf[280];
>>> +     char buf[BUFSZ];
>>>        DIR *dir;
>>>    
>>>        dir = fdopendir(dup(clients));
>>> @@ -229,32 +232,113 @@ static void create(int i915, int clients)
>>>        igt_assert_eq(count_clients(clients), 1);
>>>    }
>>>    
>>> +static const char *find_client(int clients, pid_t pid, char *buf)
>>> +{
>>> +     DIR *dir = fdopendir(dup(clients));
>>> +
>>> +     /* Reading a dir as it changes does not appear to be stable, SEP */
>>> +     for (int pass = 0; pass < 2; pass++) {
>>> +             struct dirent *de;
>>> +
>>> +             rewinddir(dir);
>>> +             while ((de = readdir(dir))) {
>>> +                     if (!isdigit(de->d_name[0]))
>>> +                             continue;
>>> +
>>> +                     snprintf(buf, BUFSZ, "%s/pid", de->d_name);
>>> +                     igt_sysfs_read(clients, buf, buf, sizeof(buf));
>>> +                     if (atoi(buf) != pid)
>>> +                             continue;
>>> +
>>> +                     strncpy(buf, de->d_name, BUFSZ);
>>> +                     goto out;
>>> +             }
>>> +     }
>>> +     *buf = '\0';
>>> +out:
>>> +     closedir(dir);
>>> +     return buf;
>>> +}
>>> +
>>>    static int find_me(int clients, pid_t pid)
>>>    {
>>> -     struct dirent *de;
>>> -     char buf[280];
>>> -     int me = -1;
>>> -     DIR *dir;
>>> +     char buf[BUFSZ];
>>>    
>>> -     dir = fdopendir(dup(clients));
>>> -     igt_assert(dir);
>>> -     rewinddir(dir);
>>> +     return openat(clients,
>>> +                   find_client(clients, pid, buf),
>>> +                   O_DIRECTORY | O_RDONLY);
>>> +}
>>>    
>>> -     while ((de = readdir(dir))) {
>>> -             if (!isdigit(de->d_name[0]))
>>> -                     continue;
>>> +static int reopen_directory(int fd)
>>> +{
>>> +     char buf[BUFSZ];
>>> +     int dir;
>>>    
>>> -             snprintf(buf, sizeof(buf), "%s/pid", de->d_name);
>>> -             igt_sysfs_read(clients, buf, buf, sizeof(buf));
>>> -             if (atoi(buf) != pid)
>>> -                     continue;
>>> +     snprintf(buf, sizeof(buf), "/proc/self/fd/%d", fd);
>>> +     dir = open(buf, O_RDONLY);
>>
>> Maybe O_DIRECTORY if it is open_directory.
>>
>>> +     igt_assert_fd(dir);
>>>    
>>> -             me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
>>> -             break;
>>> +     return dir;
>>> +}
>>> +
>>> +static unsigned int my_id(int clients, pid_t pid)
>>> +{
>>> +     char buf[BUFSZ];
>>> +
>>> +     return atoi(find_client(clients, pid, buf));
>>> +}
>>> +
>>> +static unsigned int recycle_client(int i915, int clients)
>>> +{
>>> +     int device, client;
>>> +
>>> +     device = gem_reopen_driver(i915);
>>> +     client = my_id(clients, getpid());
>>> +     close(device);
>>> +
>>> +     return client;
>>> +}
>>> +
>>> +static void recycle(int i915, int clients)
>>> +{
>>> +     const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
>>> +
>>> +     /*
>>> +      * As we open and close clients, we do not expect to reuse old ids,
>>> +      * i.e. we use a cyclic ida. This reduces the likelihood of userspace
>>> +      * watchers becoming confused and mistaking the new client as a
>>> +      * continuation of the old.
>>> +      */
>>> +     igt_require(my_id(clients, getpid()) < INT_MAX / 2);
>>
>> Hm this is a bit dodgy - it will cause "permanent" skips if running the
>> test in a loop. Just for the client > last assert below? I guess it is
>> hard to handle wrap with forked clients.
> 
> It takes about a day to reach 2 billion ids on a fast machine. For CI, I
> think we are safe.
> 
> We could do the (int)(A - B) > 0 to handle the wrap, that would be more
> sensible.
> 
> [...]
> 
>> Okay better than nothing.
> 
> But first we need to resolve the failure to find itself. :(

Forgot about that.. Start with one child for now?

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic
  2021-01-26 10:30 [Intel-gfx] [PATCH i-g-t 1/2] " Chris Wilson
@ 2021-01-26 10:30 ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2021-01-26 10:30 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev, Chris Wilson

The client id used is a cyclic allocator as that reduces the likelihood
of userspace seeing the same id used again (and so confusing the new
client as the old). Verify that each new client has an id greater than
the last.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 tests/i915/sysfs_clients.c | 126 +++++++++++++++++++++++++++++++------
 1 file changed, 108 insertions(+), 18 deletions(-)

diff --git a/tests/i915/sysfs_clients.c b/tests/i915/sysfs_clients.c
index a3a1f81e1..b7048938e 100644
--- a/tests/i915/sysfs_clients.c
+++ b/tests/i915/sysfs_clients.c
@@ -47,6 +47,8 @@
 #define assert_within_epsilon(x, ref, tolerance) \
 	__assert_within_epsilon(x, ref, tolerance / 100., tolerance / 100.)
 
+#define BUFSZ 280
+
 #define MI_BATCH_BUFFER_START (0x31 << 23)
 #define MI_BATCH_BUFFER_END (0xa << 23)
 #define MI_ARB_CHECK (0x5 << 23)
@@ -75,7 +77,7 @@ static void pidname(int i915, int clients)
 {
 	struct dirent *de;
 	int sv[2], rv[2];
-	char buf[280];
+	char buf[BUFSZ];
 	int me = -1;
 	long count;
 	pid_t pid;
@@ -180,7 +182,7 @@ static long count_clients(int clients)
 {
 	struct dirent *de;
 	long count = 0;
-	char buf[280];
+	char buf[BUFSZ];
 	DIR *dir;
 
 	dir = fdopendir(dup(clients));
@@ -229,32 +231,117 @@ static void create(int i915, int clients)
 	igt_assert_eq(count_clients(clients), 1);
 }
 
-static int find_me(int clients, pid_t pid)
+static const char *find_client(int clients, pid_t pid, char *buf)
 {
 	struct dirent *de;
-	char buf[280];
-	int me = -1;
 	DIR *dir;
 
 	dir = fdopendir(dup(clients));
 	igt_assert(dir);
-	rewinddir(dir);
 
-	while ((de = readdir(dir))) {
-		if (!isdigit(de->d_name[0]))
-			continue;
+	for (int pass = 0; pass < 2; pass++) { /* What if the dir changes? */
+		rewinddir(dir);
+		while ((de = readdir(dir))) {
+			if (!isdigit(de->d_name[0]))
+				continue;
 
-		snprintf(buf, sizeof(buf), "%s/pid", de->d_name);
-		igt_sysfs_read(clients, buf, buf, sizeof(buf));
-		if (atoi(buf) != pid)
-			continue;
+			snprintf(buf, BUFSZ, "%s/pid", de->d_name);
+			igt_sysfs_read(clients, buf, buf, sizeof(buf));
+			if (atoi(buf) != pid)
+				continue;
 
-		me = openat(clients, de->d_name, O_DIRECTORY | O_RDONLY);
-		break;
+			strncpy(buf, de->d_name, BUFSZ);
+			goto out;
+		}
 	}
-
+	*buf = '\0';
+out:
 	closedir(dir);
-	return me;
+	return buf;
+}
+
+static int find_me(int clients, pid_t pid)
+{
+	char buf[BUFSZ];
+
+	return openat(clients,
+		      find_client(clients, pid, buf),
+		      O_DIRECTORY | O_RDONLY);
+}
+
+static int reopen_directory(int fd)
+{
+	char buf[BUFSZ];
+	int dir;
+
+	snprintf(buf, sizeof(buf), "/proc/self/fd/%d", fd);
+	dir = open(buf, O_RDONLY);
+	igt_assert_fd(dir);
+
+	return dir;
+}
+
+static unsigned int my_id(int clients, pid_t pid)
+{
+	char buf[BUFSZ];
+
+	return atoi(find_client(clients, pid, buf));
+}
+
+static unsigned int recycle_client(int i915, int clients)
+{
+	int device, client;
+
+	device = gem_reopen_driver(i915);
+	client = my_id(clients, getpid());
+	close(device);
+
+	/* Client is now closed so no longer reported */
+	igt_assert_eq(my_id(clients, getpid()), 0);
+
+	return client;
+}
+
+static void recycle(int i915, int clients)
+{
+	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
+
+	/*
+	 * As we open and close clients, we do not expect to reuse old ids,
+	 * i.e. we use a cyclic ida. This reduces the likelihood of userspace
+	 * watchers becoming confused and mistaking the new client as a
+	 * continuation of the old.
+	 */
+	igt_require(my_id(clients, getpid()) < 10000000);
+	igt_assert(my_id(clients, getpid()));
+
+	igt_fork(child, 2 * ncpus) {
+		unsigned int client, last;
+
+		/* Reopen the directory fd for each client */
+		clients = reopen_directory(clients);
+
+		last = recycle_client(i915, clients);
+		igt_info("Child[%d] first client:%d\n", getpid(), last);
+		igt_until_timeout(5) {
+			client = recycle_client(i915, clients);
+
+			/* In 5 seconds we are not going to exhaust the ids */
+			igt_assert(client > last);
+			last = client;
+		}
+		igt_info("Child[%d] last client:%d\n", getpid(), last);
+	}
+	igt_waitchildren();
+
+	/* Cleanup delayed behind rcu */
+	igt_until_timeout(30) {
+		sched_yield();
+		if (count_clients(clients) == 1)
+			break;
+		usleep(10000);
+	}
+	igt_assert_eq(count_clients(clients), 1);
 }
 
 static int64_t read_runtime(int client, int class)
@@ -719,7 +806,7 @@ sema(int i915, int clients, const struct intel_execution_engine2 *e, int f)
 static int read_all(int clients, pid_t pid, int class, uint64_t *runtime)
 {
 	struct dirent *de;
-	char buf[280];
+	char buf[BUFSZ];
 	int count = 0;
 	DIR *dir;
 
@@ -958,6 +1045,9 @@ igt_main
 	igt_subtest("create")
 		create(i915, clients);
 
+	igt_subtest("recycle")
+		recycle(i915, clients);
+
 	igt_subtest_group
 		test_busy(i915, clients);
 
-- 
2.30.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-01-29 18:30 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-26 13:05 [Intel-gfx] [PATCH i-g-t 1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs Chris Wilson
2021-01-26 13:05 ` [igt-dev] " Chris Wilson
2021-01-26 13:05 ` [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic Chris Wilson
2021-01-26 13:05   ` [igt-dev] " Chris Wilson
2021-01-26 16:47   ` [Intel-gfx] " Tvrtko Ursulin
2021-01-26 16:47     ` [igt-dev] " Tvrtko Ursulin
2021-01-26 22:04     ` [Intel-gfx] " Chris Wilson
2021-01-26 22:07       ` Chris Wilson
2021-01-26 22:07         ` [igt-dev] " Chris Wilson
2021-01-27  9:12       ` [Intel-gfx] " Tvrtko Ursulin
2021-01-27  9:12         ` [igt-dev] " Tvrtko Ursulin
2021-01-27  9:51         ` [Intel-gfx] " Chris Wilson
2021-01-27  9:51           ` [igt-dev] " Chris Wilson
2021-01-29  9:18   ` [Intel-gfx] " Tvrtko Ursulin
2021-01-29  9:52     ` Chris Wilson
2021-01-29  9:52       ` [igt-dev] " Chris Wilson
2021-01-29 18:30       ` [Intel-gfx] " Tvrtko Ursulin
2021-01-26 16:33 ` [Intel-gfx] [igt-dev] [PATCH i-g-t 1/2] i915/sysfs_client: Ignore clients being closed as we read their sysfs Tvrtko Ursulin
2021-01-26 18:06 ` [igt-dev] ✗ GitLab.Pipeline: failure for series starting with [i-g-t,1/2] " Patchwork
2021-01-26 18:18 ` [igt-dev] ✓ Fi.CI.BAT: success " Patchwork
2021-01-26 23:20 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2021-01-26 10:30 [Intel-gfx] [PATCH i-g-t 1/2] " Chris Wilson
2021-01-26 10:30 ` [Intel-gfx] [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.