On Sat, May 10, 2025 at 6:41 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Fri, 9 May 2025 14:49:37 -0700
> Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> > > @@ -133,13 +135,15 @@ static void unwind_deferred_task_work(struct callback_head *head)
> > >
> > > timestamp = info->timestamp;
> > >
> > > - guard(mutex)(&callback_mutex);
> > > - list_for_each_entry(work, &callbacks, list) {
> > > + idx = srcu_read_lock(&unwind_srcu);
> >
> > nit: you could have used guard(srcu)(&unwind_srcu) ?
>
> Then it would be a scope_guard() as it is only needed for the list. I
> prefer using guard() when it is most of the function that is being
> protected. Here it's just the list and nothing else.
>
> One issue I have with guard() is that it tends to "leak". That is, if you
> use it to protect only one thing and then add more after what you are
> protecting, then the guard ends up protecting more than it needs to.
>
Yep, makes sense. I just noticed the use of guard() for mutex before,
so assumes the same could be done for SRCU, but what you are saying
makes sense, no problem.
> If anything, I would make the loop into its own function with the guard()
> then it is more obvious. But for now, I think it's fine as is, unless
> others prefer the switch.
>
> -- Steve
>
> >
> > > + list_for_each_entry_srcu(work, &callbacks, list,
> > > + srcu_read_lock_held(&unwind_srcu)) {
> > > if (task->unwind_mask & (1UL << work->bit)) {
> > > work->func(work, &trace, timestamp);
> > > clear_bit(work->bit, ¤t->unwind_mask);
> > > }
> > > }
> > > + srcu_read_unlock(&unwind_srcu, idx);
> > > }
> > >
