Re: [PATCH i-g-t 3/3] tests/xe_eudebug_online: Add set-breakpoint-sigint-debugger test

["Manszewski, Christoph" <christoph.manszewski@intel.com>]

Hi Dominik,

On 24.02.2025 14:08, Dominik Karol Piątkowski wrote:
> Add a test that sends SIGINT to the debugger thread with random timing
> and checks if nothing breaks, exercising the scenario multiple times.

I have to admit I don't quite understand what this test is supposed to 
test. The worker loop you are interrupting polls for new events and 
reads them via a ioctl. Trying to understand what this test could test I 
think I found a corner case of our read_event ioctl. We can end up with 
a resource leak and missing event when the interrupt would happen during 
copy_to_user (after the event got pulled from the fifo, and before it 
got freed) but besides the possibility that this case would be triggered 
it would still go unnoticed by this test.

It feels to me like it is more a test for our IGT lib and session 
handling than actual Xe eudebug functionality. But I may be missing 
something here and perhaps a more verbose explanation of the intention 
of this test would prove me wrong.

Regards,
Christoph


> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> ---
>   tests/intel/xe_eudebug_online.c | 75 +++++++++++++++++++++++++++++++++
>   1 file changed, 75 insertions(+)
> 
> diff --git a/tests/intel/xe_eudebug_online.c b/tests/intel/xe_eudebug_online.c
> index 472013fe1..8a97c8066 100644
> --- a/tests/intel/xe_eudebug_online.c
> +++ b/tests/intel/xe_eudebug_online.c
> @@ -1506,6 +1506,77 @@ static void test_set_breakpoint_online(int fd, struct drm_xe_engine_class_instan
>   	online_debug_data_destroy(data);
>   }
>   
> +/**
> + * SUBTEST: set-breakpoint-sigint-debugger
> + * Description:
> + *	A variant of set-breakpoint that sends SIGINT to the debugger thread with random timing
> + *	and checks if nothing breaks, exercising the scenario multiple times.
> + */
> +static void test_set_breakpoint_online_sigint_debugger(int fd,
> +						       struct drm_xe_engine_class_instance *hwe,
> +						       int flags)
> +{
> +	struct xe_eudebug_session *s;
> +	struct online_debug_data *data;
> +	struct timespec ts = { };
> +	int loop_count = 0;
> +	uint64_t sleep_time;
> +	uint64_t set_breakpoint_time;
> +	uint64_t max_sleep_time;
> +
> +	/*
> +	 * Measure the average time required for basic set-breakpoint variant,
> +	 * so sleep_time range is correct.
> +	 */
> +	igt_nsec_elapsed(&ts);
> +	for (int i = 0; i < 10; i++)
> +		test_set_breakpoint_online(fd, hwe, SHADER_NOP | TRIGGER_UFENCE_SET_BREAKPOINT);
> +	set_breakpoint_time = igt_nsec_elapsed(&ts) / (NSEC_PER_MSEC / USEC_PER_MSEC) / 10;
> +	igt_info("Average set-breakpoint execution time: %" PRIu64 " us\n", set_breakpoint_time);
> +	max_sleep_time = set_breakpoint_time * 11 / 10;
> +	igt_info("Maximum sleep_time: %" PRIu64 " us\n", max_sleep_time);
> +
> +	ts = (struct timespec) { };
> +	igt_nsec_elapsed(&ts);
> +
> +	while (igt_seconds_elapsed(&ts) < 60) {
> +		sleep_time = rand() % max_sleep_time;
> +		igt_debug("Loop %d: SIGINT after %" PRIu64 " us\n", ++loop_count, sleep_time);
> +
> +		data = online_debug_data_create(hwe);
> +		s = xe_eudebug_session_create(fd, run_online_client, flags, data);
> +		xe_eudebug_debugger_add_trigger(s->debugger, DRM_XE_EUDEBUG_EVENT_OPEN,
> +						open_trigger);
> +		xe_eudebug_debugger_add_trigger(s->debugger, DRM_XE_EUDEBUG_EVENT_EXEC_QUEUE,
> +						exec_queue_trigger);
> +		xe_eudebug_debugger_add_trigger(s->debugger, DRM_XE_EUDEBUG_EVENT_VM,
> +						vm_open_trigger);
> +		xe_eudebug_debugger_add_trigger(s->debugger, DRM_XE_EUDEBUG_EVENT_METADATA,
> +						create_metadata_trigger);
> +		xe_eudebug_debugger_add_trigger(s->debugger, DRM_XE_EUDEBUG_EVENT_VM_BIND_UFENCE,
> +						ufence_ack_set_bp_trigger);
> +		xe_eudebug_debugger_add_trigger(s->debugger, DRM_XE_EUDEBUG_EVENT_EU_ATTENTION,
> +						eu_attention_resume_trigger);
> +
> +		igt_assert_eq(xe_eudebug_debugger_attach(s->debugger, s->client), 0);
> +		xe_eudebug_debugger_start_worker(s->debugger);
> +		xe_eudebug_client_start(s->client);
> +
> +		usleep(sleep_time);
> +		igt_assert_eq(pthread_kill(s->debugger->worker_thread, SIGINT), 0);
> +		close(s->debugger->fd);
> +		s->debugger->worker_state = DEBUGGER_WORKER_INACTIVE;
> +
> +		xe_eudebug_client_wait_done(s->client);
> +
> +		xe_eudebug_event_log_print(s->debugger->log, true);
> +		xe_eudebug_event_log_print(s->client->log, true);
> +
> +		xe_eudebug_session_destroy(s);
> +		online_debug_data_destroy(data);
> +	}
> +}
> +
>   /**
>    * SUBTEST: pagefault-read
>    * Description:
> @@ -2426,6 +2497,10 @@ igt_main
>   	test_gt_render_or_compute("set-breakpoint", fd, hwe)
>   		test_set_breakpoint_online(fd, hwe, SHADER_NOP | TRIGGER_UFENCE_SET_BREAKPOINT);
>   
> +	test_gt_render_or_compute("set-breakpoint-sigint-debugger", fd, hwe)
> +		test_set_breakpoint_online_sigint_debugger(fd, hwe,
> +							   SHADER_NOP | TRIGGER_UFENCE_SET_BREAKPOINT);
> +
>   	test_gt_render_or_compute("breakpoint-not-in-debug-mode", fd, hwe)
>   		test_basic_online(fd, hwe, SHADER_BREAKPOINT | DISABLE_DEBUG_MODE);
>