Re: [PATCH v3] kernel/fork: beware of __put_task_struct calling context

[Wander Lairson Costa]

On Thu, Feb 2, 2023 at 3:37 PM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> On 02/01, Wander Lairson Costa wrote:
> >
> > Instead of calling __put_task_struct() directly, we defer it using
> > call_rcu(). A more natural approach would use a workqueue, but since
> > in PREEMPT_RT, we can't allocate dynamic memory from atomic context,
> > the code would become more complex because we would need to put the
> > work_struct instance in the task_struct and initialize it when we
> > allocate a new task_struct.
>
> I don't think I can ack the changes in PREEMPT_RT but this version LGTM.
>
>
>
>
> just a couple of purely cosmetic nits, feel free to ignore...
>
> > +static void __delayed_put_task_struct(struct rcu_head *rhp)
> > +{
> > +     struct task_struct *task = container_of(rhp, struct task_struct, rcu);
> > +
> > +     ___put_task_struct(task);
> > +}
>
> We already have delayed_put_task_struct() which differs very much.
> Perhaps something like ___put_task_struct() will look less confusing.
>

___put_task_struct()? I already added a function with this name below.

> > +void __put_task_struct(struct task_struct *tsk)
> > +{
> > +     if (IS_ENABLED(CONFIG_PREEMPT_RT) && (!preemptible() || !in_task()))
> > +             /*
> > +              * under PREEMPT_RT, we can't call put_task_struct
> > +              * in atomic context because it will indirectly
> > +              * acquire sleeping locks.
> > +              */
> > +             call_rcu(&tsk->rcu, __delayed_put_task_struct);
>
> Perhaps this deserves additional note to explain why is it safe to use tsk->rcu
> union. May be this is obvious, but I was confused when I looked at the previous
> version ;)
>

Makes sense, I will add it in the next version.

> but again, feel free to ignore.
>
> Oleg.
>