Re: [PATCH v4] perf fdarray: Fix destructor invocation and event counting in fdarray__filter

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The refinement to fdarray__filter causes an immediate 'pollfd index mismatch' error and premature termination on control descriptor POLLHUP by prematurely clearing fd and revents.
--

--- Patch [1]: [PATCH v4] perf fdarray: Fix destructor invocation and event counting in fdarray__filter ---
commit 495f184eb7be01a8af2c7157e4d7f142c89228b0
Author: Ian Rogers <[email protected]>

perf fdarray: Fix destructor invocation and event counting in fdarray__filter

This commit refines the early continue filter in fdarray__filter to only skip
system-wide perf events. Control descriptors now fall through to have their fd
values unset to -1 on POLLHUP while avoiding their destructors. It also ensures
the active event counter increments strictly for completely filterable events.

> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> index 67b73481df27e..3681ad7c65278 100644
> --- a/tools/lib/api/fd/array.c
> +++ b/tools/lib/api/fd/array.c
> @@ -116,14 +116,23 @@ int fdarray__filter(struct fdarray *fda, short revents,
>  		return 0;
>  
>  	for (fd = 0; fd < fda->nr; ++fd) {
> -		if (fda->priv[fd].flags & fdarray_flag__nonfilterable)
> +		/*
> +		 * System-wide perf events are nonfilterable but not non_perf_event.
> +		 * We want to skip them entirely and never process revents on them.
> +		 */
> +		if ((fda->priv[fd].flags & fdarray_flag__nonfilterable) &&
> +		    !(fda->priv[fd].flags & fdarray_flag__non_perf_event))
>  			continue;
>  
>  		if (!fda->entries[fd].events)
>  			continue;
>  
>  		if (fda->entries[fd].revents & revents) {
> -			if (entry_destructor)
> +			/*
> +			 * Control descriptors are non_perf_event and don't need
> +			 * their perf-specific destructors triggered.
> +			 */
> +			if (entry_destructor && !(fda->priv[fd].flags & fdarray_flag__non_perf_event))
>  				entry_destructor(fda, fd, arg);
>  
>  			/*
>  			 * Set fd to -1 so poll() ignores this entry; otherwise
>  			 * POLLHUP/POLLERR are still reported for events=0 fds
>  			 * (POSIX: always checked), causing a poll storm.
>  			 */
>  			fda->entries[fd].fd = -1;
>  			fda->entries[fd].revents = fda->entries[fd].events = 0;

[Severity: High]
Does unsetting the file descriptor to -1 and clearing revents for control
descriptors cause a pollfd index mismatch error during teardown?

When __cmd_record() handles a POLLHUP on the control pipe, it calls
fdarray__filter() which now mutates the thread's control descriptor:

tools/perf/builtin-record.c:__cmd_record() {
    ...
    if (fdarray__filter(&thread->pollfd, POLLERR | POLLHUP,
                        record__thread_munmap_filtered, NULL) == 0)
        draining = true;

    err = record__update_evlist_pollfd_from_thread(rec, rec->evlist, thread);
    ...
}

Immediately after, record__update_evlist_pollfd_from_thread() compares the
mutated thread array against evlist->pollfd, which retains the original
valid file descriptor. This strict check fails and returns -EINVAL, causing
an early ungraceful abort:

tools/perf/builtin-record.c:record__update_evlist_pollfd_from_thread() {
    ...
    if (e_entries[e_pos].fd != t_entries[t_pos].fd ||
        e_entries[e_pos].events != t_entries[t_pos].events) {
        pr_err("Thread and evlist pollfd index mismatch\n");
        err = -EINVAL;
        continue;
    }
}

Furthermore, does clearing revents to 0 here ensure evlist__ctlfd_process()
will never see the POLLHUP to perform a clean teardown via
evlist__finalize_ctlfd()?

>  			continue;
>  		}
>  
> -		++nr;
> +		if (!(fda->priv[fd].flags & fdarray_flag__nonfilterable))
> +			++nr;
>  	}
>  
>  	return nr;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1