[PATCH i-g-t v2 2/3] lib/igt_device_scan: change device list variable visibility

[Zbigniew Kempczyński]

On Mon, Jan 27, 2025 at 09:31:46AM -0600, Lucas De Marchi wrote:
> On Mon, Jan 27, 2025 at 05:49:15AM +0100, Zbigniew Kempczyński wrote:
> > On Fri, Jan 24, 2025 at 05:26:25PM +0100, Kamil Konieczny wrote:
> > > Hi Zbigniew,
> > > On 2025-01-23 at 10:52:07 +0100, Zbigniew Kempczyński wrote:
> > > 
> > > could you improve subject? something like:
> > > 
> > > lib/igt_device_scan: Fix multithreading for Xe
> > 
> > Yes, I can change it to sth like
> > 
> > 'lib/igt_device_scan: fix multithreading support in device scanning'
> > 
> > because it also affects i915 even if there's not used there.
> > 
> > > 
> > > > In commit: 2e8f3e06fb (tests/xe_evict|exec_threads: Use fd reopen
> > > > to avoid corrupting global data) I simply changed tests which
> > > > corrupted device list global variable. i915 didn't use opening
> > > > devices in multithreaded tests but this changed with xe.
> > > >
> > > > Device scan is now unconditional so we may change device list
> > > > variable visibility to TLS. With this reverting of above commit
> > > > should be safe (but I'm not sure is it worth to do this).
> > > >
> > > 
> > > What about adding Fixes: tag? It is not a kernel so no strict
> > > rules here, up to you.
> > 
> > I don't if any freedesktop issue was created for this, maybe Lucas
> > knows about it.
> 
> fdo issue would use a Closes:, not Fixes:
> 
> We do have some issues in which the device "disappears" after certain
> tests. They are usually created and closed as "cannot reproduce" after
> some time.
> 
> I don't think we can point to any bug with 100% confidence it actually
> closes it - there may be other bugs lurking around on either kernel or
> igt.
> 
> For this specific commit (not the one before) maybe we can point to the
> commit first using multiple threads, but I don't think it's really
> accurate.

So if you don't mind I would merge it as it is (patches 1-2) with reprase
of commit title as Kamil suggested for second patch. If you won't oppose
I'm going to merge this tomorrow (no negative answer means agreement).

--
Zbigniew

> 
> Lucas De Marchi
> 
> > 
> > --
> > Zbigniew
> > 
> > > 
> > > > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > > 
> > > You forgot about:
> > > Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > > 
> > > LGTM so also:
> > > Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > > 
> > > > ---
> > > >  lib/igt_device_scan.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
> > > > index 44b632008b..c36b0efa90 100644
> > > > --- a/lib/igt_device_scan.c
> > > > +++ b/lib/igt_device_scan.c
> > > > @@ -247,7 +247,7 @@ struct igt_device {
> > > >  };
> > > >
> > > >  /* Scanned devices */
> > > > -static struct {
> > > > +static __thread struct {
> > > >  	struct igt_list_head all;
> > > >  	struct igt_list_head filtered;
> > > >  	bool devs_scanned;
> > > > --
> > > > 2.34.1
> > > >