[igt-dev] [PATCH i-g-t v14 04/33] lib/igt_core: Track child process pid and tid

[Zbigniew Kempczyński]

On Thu, Jan 14, 2021 at 03:56:13PM +0000, Chris Wilson wrote:
> Quoting Zbigniew Kempczyński (2021-01-14 12:22:04)
> > Introduce variables which can decrease number of getpid()/gettid()
> > calls, especially for allocator which must be aware of method
> > acquiring addresses.
> > 
> > When child is spawned using igt_fork() we can control its initialization
> > and prepare child_pid implicitly. Tracking child_tid requires our
> > intervention in the code and do something like this:
> > 
> > if (child_tid == -1)
> >         child_tid = gettid()
> > 
> > Variable was created for using in TLS so each thread is created
> > with variable set to -1. This will give each thread it's own "copy"
> > and there's no risk to use other thread tid. For each forked child
> > we reassign -1 to child_tid to avoid using already set variable.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Dominik Grzegorzek <dominik.grzegorzek at intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Petri Latvala <petri.latvala at intel.com>
> > Acked-by: Arjun Melkaveri <arjun.melkaveri at intel.com>
> > ---
> >  lib/igt_core.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 739e3efab..ce7e27b62 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -58,6 +58,7 @@
> >  #include <glib.h>
> >  
> >  #include "drmtest.h"
> > +#include "intel_allocator.h"
> >  #include "intel_chipset.h"
> >  #include "intel_io.h"
> >  #include "igt_debugfs.h"
> > @@ -306,6 +307,10 @@ int num_test_children;
> >  int test_children_sz;
> >  bool test_child;
> >  
> > +/* For allocator purposes */
> > +pid_t child_pid  = -1;
> > +__thread pid_t child_tid  = -1;
> > +
> >  enum {
> >         /*
> >          * Let the first values be used by individual tests so options don't
> > @@ -2303,6 +2308,8 @@ bool __igt_fork(void)
> >         case 0:
> >                 test_child = true;
> >                 pthread_mutex_init(&print_mutex, NULL);
> > +               child_pid = getpid();
> > +               child_tid = -1;
> >                 exit_handler_count = 0;
> >                 reset_helper_process_list();
> >                 oom_adjust_for_doom();
> 
> Where's the user? Why is it only partially exported? What's the include
> for?

Agree, include is not valid here. 

> 
> Should we also be pulling in gettid from glibc-2.30?

IGT's already have this missing syscall defined in igt_aux.h. What you
mean we have pull in gettid from glibc?

> 
> I presume you have some microbenchmarks that reflect the importance of
> such caching? Although I can be convinced by an argument that it makes
> strace sane.
> -Chris

I haven't done benchmarks here. I don'tt want to ask kernel each time 
about process id because as you've mentioned strace looks sliced with 
getpid() calls. We're using IGT framework for doing fork (igt_fork())
so there's good place to get that global information once. Allocator
code was designed to support threads / processes so these variables
plays main role in the design. 

--
Zbigniew