Re: [PATCH i-g-t 3/3] tests/intel/xe_eudebug: Add basic-vm-bind-ufence-sigint-client test

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

Hi Dominik,

On 9.12.2024 15:13, Dominik Karol Piątkowski wrote:
> Add a test that gives user fence in application, holds it, sends SIGINT
> to client and checks if anything breaks.
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> ---
>   tests/intel/xe_eudebug.c | 44 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
> 
> diff --git a/tests/intel/xe_eudebug.c b/tests/intel/xe_eudebug.c
> index 0a4bebf1e..8728d5b23 100644
> --- a/tests/intel/xe_eudebug.c
> +++ b/tests/intel/xe_eudebug.c
> @@ -2218,6 +2218,47 @@ static void test_ufence_drop_debugger_before_ack(int fd)
>   	ufence_priv_destroy(priv);
>   }
>   
> +/**
> + * SUBTEST: basic-vm-bind-ufence-sigint-client
> + * Description:
> + *	Give user fence in application, hold it, send SIGINT to client and check if anything breaks.
> + */
> +static void test_ufence_sigint_client(int fd)
> +{
> +	struct xe_eudebug_debugger *d;
> +	struct xe_eudebug_session *s;
> +	struct xe_eudebug_client *c;
> +	struct ufence_priv *priv;
> +
> +	priv = ufence_priv_create();
> +	s = xe_eudebug_session_create(fd, basic_ufence_client, 0, priv);
> +	c = s->client;
> +	d = s->debugger;
> +
> +	xe_eudebug_debugger_add_trigger(d,
> +					DRM_XE_EUDEBUG_EVENT_VM_BIND_UFENCE,
> +					basic_ufence_trigger);
> +
> +	igt_assert_eq(xe_eudebug_debugger_attach(d, c), 0);
> +	xe_eudebug_debugger_start_worker(d);
> +	xe_eudebug_client_start(c);
> +
> +	xe_eudebug_debugger_wait_stage(s, STAGE_CLIENT_WAIT_ON_UFENCE_DONE);
> +	xe_eudebug_assert_f(d, wait_for_ufence_events(priv, XE_EUDEBUG_DEFAULT_TIMEOUT_SEC * MSEC_PER_SEC) == 0,
> +			    "missing ufence events\n");
> +
> +	kill(c->pid, SIGINT);
> +
> +	xe_eudebug_debugger_stop_worker(d, 1);
> +
> +	xe_eudebug_event_log_print(d->log, true);
> +
> +	xe_eudebug_debugger_destroy(s->debugger);
> +	free(s);

Shouldn't we somehow cleanup after the client? A hacky way would be to 
set the c->pid to 0 and just call the session_destroy function. 
Alternatively we could improve the 'xe_eudebug_client_wait_done' 
function to handle a dead process with some return value and which I 
imagine could improve the log readability on error/crash conditions (we 
wouldn't try to read an invalid pipe).

If you would for some reason go the second route it would also be 
rewarded by the possibility to cleanly merge this with the other ufence 
test functions =) If not then apart from the ack/reconnect/kill block we 
would also need to make the client wait done call conditional and... I 
don't know about that, maybe that would also be worth it to reduce code 
duplication. But no strong opinion on that.

Thanks,
Christoph

> +
> +	ufence_priv_destroy(priv);
> +}
> +
>   struct vm_bind_clear_thread_priv {
>   	struct drm_xe_engine_class_instance *hwe;
>   	struct xe_eudebug_client *c;
> @@ -2875,6 +2916,9 @@ igt_main
>   	igt_subtest("basic-vm-bind-drop-debugger-before-ufence-ack")
>   		test_ufence_drop_debugger_before_ack(fd);
>   
> +	igt_subtest("basic-vm-bind-ufence-sigint-client")
> +		test_ufence_sigint_client(fd);
> +
>   	igt_subtest("vma-ufence")
>   		test_vma_ufence(fd, 0);
>