Re: [PATCH i-g-t 3/3] lib/xe_eudebug: Warn on pipe timeouts

["Grzegorzek, Dominik" <dominik.grzegorzek@intel.com>]

On Thu, 2024-09-26 at 13:39 +0300, Mika Kuoppala wrote:
> eudebug test helpers use pipe between debugger/debuggee
> communication. If we ever fail even a single pipe read
> the whole test framework sync is lost. So if we get a
> timeout we should be quite vocal about in instead of
> just notifying that the pipe read size was not met.
> 
> Further with kernel debugs on, some discovery tests
> can starve some of the debugger threads for quite
> a long time so increase the default timeout value.
> 
> Future improvement could be to parametrize the
> timeout value so that simple tests with quite
> deterministic runtime could specify a shorter
> timeout.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  lib/xe/xe_eudebug.c | 43 +++++++++++++++++++++++++++++--------------
>  lib/xe/xe_eudebug.h |  2 +-
>  2 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/xe/xe_eudebug.c b/lib/xe/xe_eudebug.c
> index 2c7cd2ae1..da93e9785 100644
> --- a/lib/xe/xe_eudebug.c
> +++ b/lib/xe/xe_eudebug.c
> @@ -47,6 +47,23 @@ struct match_dto {
>  #define CLIENT_STAGE 5
>  #define DEBUGGER_STAGE 6
>  
> +static const char *token_to_str(uint64_t token)
> +{
> +	static const char *s[] = {
> +		"client pid",
> +		"client run",
> +		"client fini",
> +		"client stop",
> +		"client stage",
> +		"debugger stage",
> +	};
> +
> +	if (token >= ARRAY_SIZE(s))
> +		return "unknown";
> +
> +	return s[token];
> +}
> +
>  static const char *type_to_str(unsigned int type)
>  {
>  	switch (type) {
> @@ -248,18 +265,7 @@ static int safe_pipe_read(int pipe[2], void *buf, int nbytes, int timeout_ms)
>  	if (ret > 0)
>  		return read(pipe[0], buf, nbytes);
>  
> -	return 0;
> -}
> -
> -static uint64_t pipe_read(int pipe[2], int timeout_ms)
> -{
> -	uint64_t in;
> -	uint64_t ret;
> -
> -	ret = safe_pipe_read(pipe, &in, sizeof(in), timeout_ms);
> -	igt_assert(ret == sizeof(in));
> -
> -	return in;
> +	return -ETIMEDOUT;
>  }
>  
>  static void pipe_signal(int pipe[2], uint64_t token)
> @@ -279,12 +285,21 @@ static void pipe_close(int pipe[2])
>  static uint64_t __wait_token(int pipe[2], const uint64_t token, int timeout_ms)
>  {
>  	uint64_t in;
> +	int ret;
>  
> -	in = pipe_read(pipe, timeout_ms);
> +	ret = safe_pipe_read(pipe, &in, sizeof(in), timeout_ms);
> +	igt_assert_f(ret > 0,
> +		     "Pipe read timeout waiting for token '%s:(%ld)'\n",
> +		     token_to_str(token), token);
>  
>  	igt_assert_eq(in, token);
>  
> -	return pipe_read(pipe, timeout_ms);
> +	ret = safe_pipe_read(pipe, &in, sizeof(in), timeout_ms);
> +	igt_assert_f(ret > 0,
> +		     "Pipe read timeout waiting for token '%s:(%ld)'\n",
> +		     token_to_str(token), token);

Can we differentiate debug prints between those two asserts? Let's say
"Pipe read timeout waiting for token's payload '%s:(%ld)'" ? Or sth simmilar.

> +
> +	return in;
>  }
>  
>  static uint64_t client_wait_token(struct xe_eudebug_client *c, const uint64_t token)
> diff --git a/lib/xe/xe_eudebug.h b/lib/xe/xe_eudebug.h
> index 29ab68fee..6e4666333 100644
> --- a/lib/xe/xe_eudebug.h
> +++ b/lib/xe/xe_eudebug.h
> @@ -112,7 +112,7 @@ typedef void (*xe_eudebug_trigger_fn)(struct xe_eudebug_debugger *,
>   * Default abort timeout to use across xe_eudebug lib and tests if no specific
>   * timeout value is required.
>   */
> -#define XE_EUDEBUG_DEFAULT_TIMEOUT_SEC		25ULL
> +#define XE_EUDEBUG_DEFAULT_TIMEOUT_SEC		60ULL
I have a feeling this should go in a separate patch. 
Can you split it? Add to both:

Reviewed-by: Dominik Grzegorzek <dominik.grzegorzek@intel.com>

Clear assert messages are always welcome!

Regards,
Dominik
>  
>  #define XE_EUDEBUG_FILTER_EVENT_NONE		BIT(DRM_XE_EUDEBUG_EVENT_NONE)
>  #define XE_EUDEBUG_FILTER_EVENT_READ		BIT(DRM_XE_EUDEBUG_EVENT_READ)