Re: [PATCH v7 1/3] sched/core: warn on call put_task_struct in invalid context

[Wander Lairson Costa]

On Fri, Apr 28, 2023 at 06:17:58PM +0200, Sebastian Andrzej Siewior wrote:
> On 2023-04-25 08:43:01 [-0300], Wander Lairson Costa wrote:
> > Under PREEMPT_RT, spinlocks become sleepable locks. put_task_struct()
> > indirectly acquires a spinlock. Therefore, it can't be called in
> > atomic/interrupt context in RT kernels.
> > 
> > To prevent such conditions, add a check for atomic/interrupt context
> > before calling put_task_struct().
> > 
> > Signed-off-by: Wander Lairson Costa <wander@xxxxxxxxxx>
> > Suggested-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> 
> Been only CCed here.
> 

I relied on the get_maintainer script to generate the recipient list for
me. I will explicity add you to the CC list next time.

> I asked to not special case PREEMPT_RT but doing this (clean up via RCU)
> unconditionally. I don't remember that someone said "this is a bad
> because $reason".
> 

Yes, I can do it. Although I would prefer to do it in a follow up patch.
This way, if something goes wrong, it is easier to revert.

> Lockdep will complain about this on !RT.
> 

In my tests, it didn't.

> The below open codes rtlock_might_resched() with no explanation on why
> it works or where it comes from.
> 

I will add some comments on the next patch version.

> The function is named put_task_struct_atomic_safe() yet it behaves it
> differently on PREEMPT_RT otherwise it remains put_task_struct().
> 
> Not good.

That's intentional. We only need to do the cleanup under RT, but for !RT
we don't need it. Anyway, in the end we will endup with an
unconditional call_rcu().

> 
> > ---
> >  include/linux/sched/task.h | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
> > index 357e0068497c..b597b97b1f8f 100644
> > --- a/include/linux/sched/task.h
> > +++ b/include/linux/sched/task.h
> > @@ -113,14 +113,28 @@ static inline struct task_struct *get_task_struct(struct task_struct *t)
> >  
> >  extern void __put_task_struct(struct task_struct *t);
> >  
> > +#define PUT_TASK_RESCHED_OFFSETS \
> > +	(rcu_preempt_depth() << MIGHT_RESCHED_RCU_SHIFT)
> > +
> > +#define __put_task_might_resched() \
> > +	__might_resched(__FILE__, __LINE__, PUT_TASK_RESCHED_OFFSETS)
> > +
> > +#define put_task_might_resched()			\
> > +	do {						\
> > +		if (IS_ENABLED(CONFIG_PREEMPT_RT))	\
> > +			__put_task_might_resched();	\
> > +	} while (0)
> > +
> >  static inline void put_task_struct(struct task_struct *t)
> >  {
> > +	put_task_might_resched();
> >  	if (refcount_dec_and_test(&t->usage))
> >  		__put_task_struct(t);
> >  }
> >  
> >  static inline void put_task_struct_many(struct task_struct *t, int nr)
> >  {
> > +	put_task_might_resched();
> >  	if (refcount_sub_and_test(nr, &t->usage))
> >  		__put_task_struct(t);
> >  }
> > -- 
> > 2.40.0
> > 
> 
> Sebastian
>