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

[Tvrtko Ursulin]

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 at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at 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