diff mbox series

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

Message ID 20230425114307.36889-2-wander@redhat.com
State New
Headers
Series Introduce put_task_struct_atomic_sleep() |

Commit Message

Wander Lairson Costa April 25, 2023, 11:43 a.m. UTC 
  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@redhat.com>
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/sched/task.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Sebastian Andrzej Siewior April 28, 2023, 4:17 p.m. UTC | #1 
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@redhat.com>
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Been only CCed here.

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".

Lockdep will complain about this on !RT.

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

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.

> ---
>  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
Wander Lairson Costa May 2, 2023, 2:46 p.m. UTC | #2 
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@redhat.com>
> > Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> 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
>
diff mbox series

Patch

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);
 }