Re: [PATCH] sched: Allow sleeping spinlocks on PREEMPT_RT within non_block_start()/end block.

[Michal Hocko <mhocko@suse.com>]

On Fri 21-08-26 11:57:55, Sebastian Andrzej Siewior wrote:
> Commit 312364f3534cc ("kernel.h: Add non_block_start/end()") added
> non_block_star()/end(). The intention was to catch any callbacks which
> depend on locks or sleep able conditionals in order to ensure forward
> progress. According to the commit message spinlocks were excluded
> "because spinlocks can't have an indirect dependency upon the page
> allocator…".
> On PREEMPT_RT this leads to a splat because spinlock_t and rwlock_t are
> turned into a sleeping spinlock which have a might_sleep() as they could
> schedule() on lock contention.
> 
> Besids mm, the other used of non_block_start() is pwm. Here the hrtimer
> can acnqure a spinlock_t on RT and I can't tell other user but I assume
> it is not atomic on RT since it wouldn't be able to acquire its own
> (spinlock_t) lock.
> 
> Since spinlock_t (and rwlock_t) are not problem, add a sleeping_lock
> argument to __might_resched() signaling if this is scheduling request
> is from a sleeping lock (true) or regular scheduling request (false).
> Add the `true' argument to rtlock_might_resched() which is used on
> PREEMPT_RT for both sleeping lock types.

This definitely makes a lot of sense to me. It looks like a
straightforward solution for false positives. The patch looks good to
me, I do not feel qualified to give you Acked-by though.

Thanks!
 
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Sean Young <sean@mess.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  include/linux/kernel.h       |  5 +++--
>  include/linux/sched.h        | 24 ++++++++++++------------
>  kernel/locking/spinlock_rt.c |  2 +-
>  kernel/sched/core.c          |  7 ++++---
>  4 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index e5570a16cbb1a..fbafdfe9bdb78 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -70,7 +70,8 @@ extern int dynamic_might_resched(void);
>  #endif /* CONFIG_PREEMPT_* */
>  
>  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> -extern void __might_resched(const char *file, int line, unsigned int offsets);
> +extern void __might_resched(const char *file, int line, unsigned int offsets,
> +			    bool rt_sleeping_lock);
>  extern void __might_sleep(const char *file, int line);
>  extern void __cant_sleep(const char *file, int line, int preempt_offset);
>  extern void __cant_migrate(const char *file, int line);
> @@ -128,7 +129,7 @@ extern void __cant_migrate(const char *file, int line);
>  # define non_block_end() WARN_ON(current->non_block_count-- == 0)
>  #else
>    static inline void __might_resched(const char *file, int line,
> -				     unsigned int offsets) { }
> +				     unsigned int offsets, bool rt_sleeping_lock) { }
>  static inline void __might_sleep(const char *file, int line) { }
>  # define might_sleep() do { might_resched(); } while (0)
>  # define cant_sleep() do { } while (0)
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 373bcc0598d10..650e42d55e1e8 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2158,9 +2158,9 @@ static inline int _cond_resched(void)
>  
>  #endif /* !CONFIG_PREEMPTION || CONFIG_PREEMPT_DYNAMIC */
>  
> -#define cond_resched() ({			\
> -	__might_resched(__FILE__, __LINE__, 0);	\
> -	_cond_resched();			\
> +#define cond_resched() ({				\
> +	__might_resched(__FILE__, __LINE__, 0, false);	\
> +	_cond_resched();				\
>  })
>  
>  extern int __cond_resched_lock(spinlock_t *lock) __must_hold(lock);
> @@ -2186,19 +2186,19 @@ extern int __cond_resched_rwlock_write(rwlock_t *lock) __must_hold(lock);
>  	(PREEMPT_LOCK_OFFSET + (1U << MIGHT_RESCHED_RCU_SHIFT))
>  #endif
>  
> -#define cond_resched_lock(lock) ({						\
> -	__might_resched(__FILE__, __LINE__, PREEMPT_LOCK_RESCHED_OFFSETS);	\
> -	__cond_resched_lock(lock);						\
> +#define cond_resched_lock(lock) ({							\
> +	__might_resched(__FILE__, __LINE__, PREEMPT_LOCK_RESCHED_OFFSETS, false);	\
> +	__cond_resched_lock(lock);							\
>  })
>  
> -#define cond_resched_rwlock_read(lock) ({					\
> -	__might_resched(__FILE__, __LINE__, PREEMPT_LOCK_RESCHED_OFFSETS);	\
> -	__cond_resched_rwlock_read(lock);					\
> +#define cond_resched_rwlock_read(lock) ({						\
> +	__might_resched(__FILE__, __LINE__, PREEMPT_LOCK_RESCHED_OFFSETS, false);	\
> +	__cond_resched_rwlock_read(lock);						\
>  })
>  
> -#define cond_resched_rwlock_write(lock) ({					\
> -	__might_resched(__FILE__, __LINE__, PREEMPT_LOCK_RESCHED_OFFSETS);	\
> -	__cond_resched_rwlock_write(lock);					\
> +#define cond_resched_rwlock_write(lock) ({						\
> +	__might_resched(__FILE__, __LINE__, PREEMPT_LOCK_RESCHED_OFFSETS, false);	\
> +	__cond_resched_rwlock_write(lock);						\
>  })
>  
>  #ifndef CONFIG_PREEMPT_RT
> diff --git a/kernel/locking/spinlock_rt.c b/kernel/locking/spinlock_rt.c
> index 1d5e1b3c60bfa..0a89e7f10af18 100644
> --- a/kernel/locking/spinlock_rt.c
> +++ b/kernel/locking/spinlock_rt.c
> @@ -33,7 +33,7 @@
>  	(rcu_preempt_depth() << MIGHT_RESCHED_RCU_SHIFT)
>  
>  #define rtlock_might_resched()						\
> -	__might_resched(__FILE__, __LINE__, RTLOCK_RESCHED_OFFSETS)
> +	__might_resched(__FILE__, __LINE__, RTLOCK_RESCHED_OFFSETS, true)
>  
>  static __always_inline void rtlock_lock(struct rt_mutex_base *rtm)
>  {
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 96226707c2f61..ee0fbb41fc61c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -9123,7 +9123,7 @@ void __might_sleep(const char *file, int line)
>  			(void *)current->task_state_change,
>  			(void *)current->task_state_change);
>  
> -	__might_resched(file, line, 0);
> +	__might_resched(file, line, 0, false);
>  }
>  EXPORT_SYMBOL(__might_sleep);
>  
> @@ -9148,7 +9148,8 @@ static inline bool resched_offsets_ok(unsigned int offsets)
>  	return nested == offsets;
>  }
>  
> -void __might_resched(const char *file, int line, unsigned int offsets)
> +void __might_resched(const char *file, int line, unsigned int offsets,
> +		     bool rt_sleeping_lock)
>  {
>  	/* Ratelimiting timestamp: */
>  	static unsigned long prev_jiffy;
> @@ -9159,7 +9160,7 @@ void __might_resched(const char *file, int line, unsigned int offsets)
>  	rcu_sleep_check();
>  
>  	if ((resched_offsets_ok(offsets) && !irqs_disabled() &&
> -	     !is_idle_task(current) && !current->non_block_count) ||
> +	     !is_idle_task(current) && (rt_sleeping_lock || !current->non_block_count)) ||
>  	    system_state == SYSTEM_BOOTING || system_state > SYSTEM_RUNNING ||
>  	    oops_in_progress)
>  		return;
> -- 
> 2.55.0

-- 
Michal Hocko
SUSE Labs