On Mon, 28 Apr 2025 14:00:07 -0400
Mathieu Desnoyers <[email protected]> wrote:
> >>
> >> It is enough to guard with RCU ? See syscall_regfunc() from
> >> tracepoint.c where we do:
> >>
> >> read_lock(&tasklist_lock);
> >> for_each_process_thread(p, t) {
> >> set_task_syscall_work(t, SYSCALL_TRACEPOINT);
> >> }
> >> read_unlock(&tasklist_lock);
> >>
> >> To prevent concurrent fork from adding threads while we
> >> iterate, thus opening the possibility of missing a clear
> >> due to a concurrent fork + set bit.
> >
> > A set_bit only would happen if the callback was live and accepting new
> > callback requests. It's a bug for a tracer to call unwind_deferred_cancel()
> > and then call unwind_deferred_request() (which would set the bit). We could
> > possibly set the tracer's unwind descriptor id to -1, and do an
> > WARN_ON_ONCE() in unwind_deferred_request() if the tracer's id is negative.
> >
> > The loop is called under the callback_mutex, where no new tracer could
> > register and be assigned that bit.
>
> Ah, that's the piece I missed. The callback_mutex prevents reallocation
> of the ID by unwind_deferred_init while iterating on the tasks.
>
> One more comment: if we change the linked list for an array (or make the
> linked list an RCU list), can we remove the callback_mutex from
> unwind_deferred_task_work by turning it into an RCU read-side ?
>
> Then we just need to wait for a grace period before returning from
> unwind_deferred_cancel, which then allows the caller to reclaim "work".
>
> Taking the callback_mutex in unwind_deferred_task_work will end up being
> the single thing that does a lot of cache line bouncing across CPUs when
> hit heavily by tracers.
I'm not against this, but again, that's an optimization. I want to keep the
initial code simple. And then add the more complex optimizations when this
is stable.
-- Steve
