Messages in this thread | | | Date | Thu, 16 Feb 2023 12:39:47 -0600 | From | David Vernet <> | Subject | Re: [PATCH] tasks: Extract rcu_users out of union |
| |
On Thu, Feb 16, 2023 at 09:04:59AM +0100, Oleg Nesterov wrote: > I won't argue with this patch, but I can't understand the changelog... > > On 02/15, David Vernet wrote: > > > > Similarly, in sched_ext, schedulers are using integer pids to remember > > tasks, and then looking them up with find_task_by_pid_ns(). This is > > slow, error prone, and adds complexity. It would be more convenient and > > performant if BPF schedulers could instead store tasks directly in maps, > > and then leverage RCU to ensure they can be safely accessed with low > > overhead. > > To simplify, suppose we have > > int global_pid; > > void func(void) > { > rcu_read_lock(); > task = find_task_by_pid(global_pid); > do_something(task); > rcu_read_unlock(); > } > > Could you explain how exactly can this patch help to turn global_pid into > "task_struct *" ? Why do you need to increment task->rcu_users ?
If you're not persisting the task in a map / data structure, then I agree that find_task_by_pid_ns() is likely sufficient. What we want to be able to do is something like this:
void func(void) { rcu_read_lock(); task = peek_next_task(); if (task) do_something(task); rcu_read_unlock(); }
In such an example, we could be peeking into a statically allocated circular queue, and want to be able to ensure that a task we look at from the top is protected with rcu. The general mechanics would be that a task is inserted with a refcount_inc_not_zero(), and when it's removed, we do a put_task_struct_rcu_user().
Does that make sense?
> > > a task that's successfully looked > > up in e.g. the pid_list with find_task_by_pid_ns(), can always have a > > 'usage' reference acquired on them, as it's guaranteed to be > > > 0 until after the next gp. > > Yes. So it seems you need another key-to-task_struct map with rcu-safe > lookup/get and thus the add() method needs inc_not_zero(task->rcu_users) ?
Yes, exactly.
Thanks for taking a look at the patch.
- David
| |