Re: [PATCH i-g-t 2/2] i915/sysfs_clients: Check that client ids are cyclic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
---
   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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux