Messages in this thread | | | Date | Thu, 4 May 2023 09:24:59 -0300 | From | Wander Lairson Costa <> | Subject | Re: [PATCH v7 2/3] sched/task: Add the put_task_struct_atomic_safe() function |
| |
On Thu, May 04, 2023 at 10:32:31AM +0100, Valentin Schneider wrote: > On 04/05/23 10:42, Peter Zijlstra wrote: > > On Tue, Apr 25, 2023 at 08:43:02AM -0300, Wander Lairson Costa wrote: > >> diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h > >> index b597b97b1f8f..cf774b83b2ec 100644 > >> --- a/include/linux/sched/task.h > >> +++ b/include/linux/sched/task.h > >> @@ -141,6 +141,41 @@ static inline void put_task_struct_many(struct task_struct *t, int nr) > >> > >> void put_task_struct_rcu_user(struct task_struct *task); > >> > >> +extern void __delayed_put_task_struct(struct rcu_head *rhp); > >> + > >> +static inline void put_task_struct_atomic_safe(struct task_struct *task) > >> +{ > >> + if (IS_ENABLED(CONFIG_PREEMPT_RT)) { > >> + /* > >> + * Decrement the refcount explicitly to avoid unnecessarily > >> + * calling call_rcu. > >> + */ > >> + if (refcount_dec_and_test(&task->usage)) > >> + /* > >> + * under PREEMPT_RT, we can't call put_task_struct > >> + * in atomic context because it will indirectly > >> + * acquire sleeping locks. > >> + * call_rcu() will schedule __delayed_put_task_struct() > >> + * to be called in process context. > >> + * > >> + * __put_task_struct() is called when > >> + * refcount_dec_and_test(&t->usage) succeeds. > >> + * > >> + * This means that it can't conflict with > >> + * put_task_struct_rcu_user() which abuses ->rcu the same > >> + * way; rcu_users has a reference so task->usage can't be > >> + * zero after rcu_users 1 -> 0 transition. > >> + * > >> + * delayed_free_task() also uses ->rcu, but it is only called > >> + * when it fails to fork a process. Therefore, there is no > >> + * way it can conflict with put_task_struct(). > >> + */ > >> + call_rcu(&task->rcu, __delayed_put_task_struct); > >> + } else { > >> + put_task_struct(task); > >> + } > >> +} > > > > Urgh.. that's plenty horrible. And I'm sure everybody plus kitchen sink > > has already asked why can't we just rcu free the thing unconditionally. > > > > Google only found me an earlier version of this same patch set, but I'm > > sure we've had that discussion many times over the past several years. > > The above and your follow up patch is just horrible. > > > > So on v3/v4 we got to doing that unconditionally for PREEMPT_RT, but per > [1] Wander went back to hand-fixing the problematic callsites. > > Now that I'm looking at it again, I couldn't find a concrete argument from > Oleg against doing this unconditionally - as Wander is pointing out in the > changelog and comments, reusing task_struct.rcu for that purpose is safe > (although not necessarily obviously so). > > Is this just miscommunication, or is there a genuine issue with doing this > unconditionally? As argued before, I'd also much rather have this be an > unconditional call_rcu() (regardless of context or PREEMPT_RT). >
Yeah, I think it was a misunderstanding of mine.
| |