Re: [PATCH i-g-t 2/2] tests/i915/perf: Exercise barrier race

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

 



Hi Janusz,

On 2023-02-01 at 20:16:11 +0100, Janusz Krzysztofik wrote:
> Hi Kamil,
> 
> On Wednesday, 1 February 2023 19:21:57 CET Kamil Konieczny wrote:
> > Hi Janusz,
> > 
> > please send patches to igt ML and add other addresses to cc:
> > I have one nit, see below.
> > 
> > On 2023-01-31 at 10:17:31 +0100, Janusz Krzysztofik wrote:
> > > Add a new subtest focused on exercising interaction between perf open /
> > > close operations, which replace active barriers with perf requests, and
> > > concurrent barrier preallocate / acquire operations performed during
> > > context first pin / last unpin.
> > > 
> > > References: https://gitlab.freedesktop.org/drm/intel/-/issues/6333
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx>
> > > Cc: Chris Wilson <chris.p.wilson@xxxxxxxxxxxxxxx>
> > > ---
> > >  tests/i915/perf.c | 41 +++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 39 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/i915/perf.c b/tests/i915/perf.c
> > > index e33cacc443..11a3ec21ab 100644
> > > --- a/tests/i915/perf.c
> > > +++ b/tests/i915/perf.c
> > > @@ -39,6 +39,7 @@
> > >  #include <math.h>
> > >  
> > >  #include "i915/gem.h"
> > > +#include "i915/gem_create.h"
> > >  #include "i915/perf.h"
> > >  #include "igt.h"
> > >  #include "igt_perf.h"
> > > @@ -4885,7 +4886,27 @@ test_whitelisted_registers_userspace_config(void)
> > >  	i915_perf_remove_config(drm_fd, config_id);
> > >  }
> > >  
> > > -static void test_open_race(const struct intel_execution_engine2 *e, int timeout)
> > > +static void gem_exec_nop(int i915, const struct intel_execution_engine2 *e)
> > > +{
> > > +	const uint32_t bbe = MI_BATCH_BUFFER_END;
> > > +	struct drm_i915_gem_exec_object2 obj = { };
> > > +	struct drm_i915_gem_execbuffer2 execbuf = {
> > > +		.buffers_ptr = to_user_pointer(&obj),
> > > +		.buffer_count = 1,
> > > +	};
> > > +
> > > +	obj.handle = gem_create(i915, 4096);
> > > +	gem_write(i915, obj.handle, 0, &bbe, sizeof(bbe));
> > > +
> > > +	execbuf.flags = e->flags;
> > > +	gem_execbuf(i915, &execbuf);
> > > +
> > > +	gem_sync(i915, obj.handle);
> > > +	gem_close(i915, obj.handle);
> > > +}
> > > +
> > > +static void test_open_race(const struct intel_execution_engine2 *e, int timeout,
> > > +			   bool use_spin)
> > -------------------------- ^
> > This is not the best way to develop new code, it may be good
> > for fast development but imho it is better to refactor existing
> > code and avoiding to add bool logic into function.
> 
> I'm not sure what you mean.  By refactoring, do you mean moving the code 
> common to both processing paths out to a separate helper, then call it from 
> two distinct functions, each implementing only one scenario?  What's wrong 
> with passing flags that select one of alternative processing paths within one 
> function?  Or are you just not happy with bool type?

It is not wrong unless there is only one such var but even then
readability of code is reduced.

> 
> > It can be done later as this is revealing the bug.
> > 
> > >  {
> > >  	int *done;
> > >  
> > > @@ -4926,6 +4947,12 @@ static void test_open_race(const struct intel_execution_engine2 *e, int timeout)
> > >  				ctx = gem_context_create_for_engine(i915, e->class, e->instance);
> > >  				gem_context_set_persistence(i915, ctx, persistence);
> > >  
> > > +				if (!use_spin) {
> > > +					gem_exec_nop(i915, e);
> > > +					gem_context_destroy(i915, ctx);
> > > +					continue;
> > > +				}
> > > +
> > >  				spin = __igt_spin_new(i915, ctx, .ahnd = ahnd);
> > >  				for (int i = random() % 7; i--; ) {
> > >  					if (random() & 1) {
> > > @@ -5330,7 +5357,17 @@ igt_main
> > >  		for_each_physical_engine(drm_fd, e)
> > >  			if (e->class == I915_ENGINE_CLASS_RENDER)
> > >  				igt_dynamic_f("%s", e->name)
> > > -					test_open_race(e, 5);
> > > +					test_open_race(e, 5, true);
> > > +	}
> > > +
> > 
> > Please add here some TODO comments from discussions and a note
> > which will help bug filling team to classify that.
> 
> TODO about moving the subtest to a different test -- OK.  But instructions for 
> Bug Filling team?  Hmm, what if this subtest triggers a completely different 
> bug in the future?  Are we going to update the comments then?  Do Bug Filling 
> team members really read the sources while classifying bugs?
> 
> Please have another look at an example of expected dmesg from the currently 
> triggered bug:
> 
> <4> [192.634015] list_del corruption. prev->next should be ffff8881479d34e0, but was ffff888108af4ce0. (prev=ffff888127335160)
> <4> [192.634032] WARNING: CPU: 1 PID: 1286 at lib/list_debug.c:59 __list_del_entry_valid+0xb7/0xe0

Ok, I see that it is kernel bug here. You are right,
please add only TODO here.

Regards,
Kamil

> <4> [192.634041] Modules linked in: vgem drm_shmem_helper fuse snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio x86_pkg_temp_thermal coretemp i915 prime_numbers i2c_algo_bit kvm_intel ttm snd_hda_intel snd_intel_dspcfg kvm drm_buddy mei_pxp snd_hda_codec smsc75xx irqbypass mei_hdcp e1000e drm_display_helper usbnet snd_hwdep crct10dif_pclmul wmi_bmof mii crc32_pclmul ghash_clmulni_intel igc snd_hda_core drm_kms_helper ptp mei_me i2c_i801 syscopyarea snd_pcm pps_core sysfillrect i2c_smbus mei sysimgblt video intel_lpss_pci wmi
> <4> [192.634156] CPU: 1 PID: 1286 Comm: perf Not tainted 6.2.0-rc6-CI_DRM_12672-gdf5c31e78609+ #1
> <4> [192.634161] Hardware name: Intel Corporation Rocket Lake Client Platform/RocketLake S SODIMM RVP, BIOS RKLSFWI1.R00.1435.A00.2010232019 10/23/2020
> <4> [192.634165] RIP: 0010:__list_del_entry_valid+0xb7/0xe0
> <4> [192.634170] Code: cc cc cc 48 89 ca 48 c7 c7 78 00 46 82 e8 0d 7e 61 00 0f 0b 31 c0 c3 cc cc cc cc 4c 89 c2 48 c7 c7 b0 00 46 82 e8 f5 7d 61 00 <0f> 0b 31 c0 c3 cc cc cc cc 48 89 d1 4c 89 c6 4c 89 ca 48 c7 c7 f8
> <4> [192.634174] RSP: 0018:ffffc90001f2fcb0 EFLAGS: 00010082
> <4> [192.634180] RAX: 0000000000000000 RBX: ffff88812f8dec40 RCX: 0000000000000000
> <4> [192.634184] RDX: 0000000000000003 RSI: ffffffff823d7c88 RDI: 00000000ffffffff
> <4> [192.634187] RBP: ffff8881479d34d8 R08: 0000000000000000 R09: ffffc90001f2fb60
> <4> [192.634191] R10: 0000000000000001 R11: 0000000000000001 R12: ffff88812f8df440
> <4> [192.634194] R13: ffff88812f8df440 R14: ffff8881479d34e0 R15: 0000000000000246
> <4> [192.634197] FS:  00007fa38f9514c0(0000) GS:ffff888450080000(0000) knlGS:0000000000000000
> <4> [192.634201] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4> [192.634205] CR2: 00007fa392c76000 CR3: 000000010809e003 CR4: 0000000000770ee0
> <4> [192.634208] PKRU: 55555554
> <4> [192.634211] Call Trace:
> <4> [192.634214]  <TASK>
> <4> [192.634218]  __i915_active_fence_set+0x71/0xf0 [i915]
> <4> [192.634412]  __i915_request_commit+0x2e1/0x5b0 [i915]
> <4> [192.634597]  i915_request_add+0xa6/0x330 [i915]
> <4> [192.634783]  gen8_modify_context+0xc2/0x220 [i915]
> <4> [192.634958]  oa_configure_all_contexts.isra.0+0x183/0x400 [i915]
> <4> [192.635137]  gen12_disable_metric_set+0x98/0x160 [i915]
> <4> [192.635305]  i915_oa_stream_destroy+0x3c/0x330 [i915]
> <4> [192.635480]  i915_perf_destroy_locked+0x22/0x80 [i915]
> <4> [192.635646]  i915_perf_release+0x35/0x50 [i915]
> <4> [192.635808]  __fput+0x95/0x260
> 
> Isn't such information from actual bug occurrence, contained in CI reports the 
> Bug Filling team is working with, more meaningful than any comment added to 
> the test source code?  How can a test know in advance what bugs it will ever 
> trigger, and who exactly will be responsible for fixing them?
> 
> Thanks,
> Janusz
> 
> > 
> > Regards,
> > Kamil
> > 
> > > +	igt_describe("Exercise perf open/close against intensive barrier preallocate/acquire");
> > > +	igt_subtest_with_dynamic("barrier-race") {
> > > +		const struct intel_execution_engine2 *e;
> > > +
> > > +		for_each_physical_engine(drm_fd, e)
> > > +			if (e->class == I915_ENGINE_CLASS_RENDER)
> > > +				igt_dynamic_f("%s", e->name)
> > > +					test_open_race(e, 5, false);
> > >  	}
> > >  
> > >  	igt_fixture {
> > 
> 
> 
> 
> 



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

  Powered by Linux