Re: [PATCH v4 i-g-t 4/6] lib/xe_eudebug: Improve SIGINT handling

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

Hi Dominik,

On 24.09.2025 14:38, Piatkowski, Dominik Karol wrote:
> Hi Christoph,
> 
>> -----Original Message-----
>> From: Manszewski, Christoph <christoph.manszewski@intel.com>
>> Sent: Wednesday, September 24, 2025 1:43 PM
>> To: Piatkowski, Dominik Karol <dominik.karol.piatkowski@intel.com>; igt-
>> dev@lists.freedesktop.org
>> Cc: Kempczynski, Zbigniew <zbigniew.kempczynski@intel.com>; Mika
>> Kuoppala <mika.kuoppala@linux.intel.com>
>> Subject: Re: [PATCH v4 i-g-t 4/6] lib/xe_eudebug: Improve SIGINT handling
>>
>> Hi Dominik,
>>
>> On 23.09.2025 15:10, Dominik Karol Piątkowski wrote:
>>> Rewrite SIGINT signal handler to toggle a variable instead of calling
>>> pthread_exit instantly, as the thread could be in critical section,
>>> causing a deadlock on the attempt of next entry into that critical
>>> section.
>>>
>>> Introduce received_signal, received_sigint and handled_sigint variables
>>> to help with coordination of handling SIGINT and other signals.
>>>
>>> Use the received_sigint variable in debugger_worker_loop to safely kill
>>> the thread, set handled_sigint to help with coordination outside the
>>> debugger loop.
>>>
>>> Rename terminate_debugger to debugger_signal_handler to describe its
>>> new function better. Add SIGTERM to signals that are handled by
>>> debugger_signal_handler.
>>>
>>> Don't kill debugger thread on non-SIGINT signals. Don't break on poll
>>> failure if signal was received.
>>>
>>> Signed-off-by: Dominik Karol Piątkowski
>> <dominik.karol.piatkowski@intel.com>
>>> ---
>>>    lib/xe/xe_eudebug.c | 26 ++++++++++++++++++++++----
>>>    lib/xe/xe_eudebug.h |  4 ++++
>>>    2 files changed, 26 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/xe/xe_eudebug.c b/lib/xe/xe_eudebug.c
>>> index deaea25cd..647da7b32 100644
>>> --- a/lib/xe/xe_eudebug.c
>>> +++ b/lib/xe/xe_eudebug.c
>>> @@ -1072,9 +1072,14 @@ xe_eudebug_read_event(int fd, struct
>> drm_xe_eudebug_event *event)
>>>    	return ret;
>>>    }
>>>
>>> -static void terminate_debugger(int sig)
>>> +static void debugger_signal_handler(int sig, siginfo_t *info, void *context)
>>>    {
>>> -	pthread_exit(NULL);
>>> +	struct xe_eudebug_debugger *d = info->si_ptr;
>>> +
>>> +	d->received_signal = true;
>>
>> If I understand correctly, without the two commits that follow, this is
>> a NULL pointer dereference. But even after combining those commits,
>> won't this cause a NULL pointer dereference if a IGT user presses 'CTRL-C'?
> 
> True, without these commits it is a NULL pointer dereference.

While it is not a critical issue, it would make sense to not have 
commits that are buggy the same way as we want to have commits that 
compile without commits that follow. It's one thing to have some tests 
that no longer pass after a commit and another to have undefined 
behavior or non-compiling code.


> 
> Regarding the ctrl-C pressed by IGT user - I just checked and indeed it will
> cause a NULL pointer dereference. I can add the check for that, but the
> question is if this is a scenario we need to defend from. If yes - I'll
> happily add such check.

I think it's good practice to have programs that exit cleanly. Imagine 
doing some debugging/repro with a test that hangs for whatever reason 
and seeing a SIGSEGV after ctrl-c. The suspicion would go towards some 
invalid memory handling just to find that this is an unrelated issue.

Regards,
Christoph
> 
>>> +
>>> +	if (sig == SIGINT)
>>> +		d->received_sigint = true;
>>>    }
>>>
>>>    static void *debugger_worker_loop(void *data)
>>> @@ -1092,14 +1097,24 @@ static void *debugger_worker_loop(void
>> *data)
>>>    	igt_assert(d->master_fd >= 0);
>>>
>>>    	igt_assert_eq(sigaction(SIGINT, NULL, &sa), 0);
>>> -	sa.sa_handler = terminate_debugger;
>>> +	sa.sa_sigaction = debugger_signal_handler;
>>> +	sa.sa_flags |= SA_SIGINFO;
>>>    	igt_assert_eq(sigaction(SIGINT, &sa, NULL), 0);
>>>
>>> +	igt_assert_eq(sigaction(SIGTERM, NULL, &sa), 0);
>>> +	sa.sa_sigaction = debugger_signal_handler;
>>> +	sa.sa_flags |= SA_SIGINFO;
>>> +	igt_assert_eq(sigaction(SIGTERM, &sa, NULL), 0);
>>> +
>>>    	do {
>>>    		p.fd = d->fd;
>>>    		ret = poll(&p, 1, timeout_ms);
>>> +		if (d->received_sigint) {
>>> +			d->handled_sigint = true;
>>> +			pthread_exit(NULL);
>>> +		}
>>>
>>> -		if (ret == -1) {
>>> +		if (!d->received_signal && ret == -1) {
>>
>> I am not sure I understand what the SIGTERM handling is supposed to
>> influence here? From patch 6/6 I gather that it is supposed to maximize
>> the chance of hitting the read IOCTL but I don't see how.
> 
> The SIGTERM handling here is stopping the loop from breaking and being unable
> to reach `if (d->received_sigint)`. I'll shortly send a v5 that has this part
> a bit improved from Zbigniew's suggestions.
> 
> Thanks,
> Dominik Karol
> 
>>
>> Regards,
>> Christoph
>>
>>
>>>    			igt_info("poll failed with errno %d\n", errno);
>>>    			break;
>>>    		}
>>> @@ -1168,6 +1183,9 @@ xe_eudebug_debugger_create(int master_fd,
>> uint64_t flags, void *data)
>>>    	d->fd = -1;
>>>    	d->master_fd = master_fd;
>>>    	d->ptr = data;
>>> +	d->received_signal = false;
>>> +	d->received_sigint = false;
>>> +	d->handled_sigint = false;
>>>
>>>    	return d;
>>>    }
>>> diff --git a/lib/xe/xe_eudebug.h b/lib/xe/xe_eudebug.h
>>> index 8f7f66220..46cd3463e 100644
>>> --- a/lib/xe/xe_eudebug.h
>>> +++ b/lib/xe/xe_eudebug.h
>>> @@ -44,6 +44,10 @@ struct xe_eudebug_debugger {
>>>    	pthread_t worker_thread;
>>>    	enum xe_eudebug_debugger_worker_state worker_state;
>>>
>>> +	bool received_signal;
>>> +	bool received_sigint;
>>> +	bool handled_sigint;
>>> +
>>>    	int p_client[2];
>>>    };
>>>
>