Re: [PATCH v2 7/9] sched: define TIF_ALLOW_RESCHED

[Ankur Arora]

Peter Zijlstra <peterz@xxxxxxxxxxxxx> writes:

> On Wed, Aug 30, 2023 at 11:49:56AM -0700, Ankur Arora wrote:
>
>> +#ifdef TIF_RESCHED_ALLOW
>> +/*
>> + * allow_resched() .. disallow_resched() demarcate a preemptible section.
>> + *
>> + * Used around primitives where it might not be convenient to periodically
>> + * call cond_resched().
>> + */
>> +static inline void allow_resched(void)
>> +{
>> +	might_sleep();
>> +	set_tsk_thread_flag(current, TIF_RESCHED_ALLOW);
>
> So the might_sleep() ensures we're not currently having preemption
> disabled; but there's nothing that ensures we don't do stupid things
> like:
>
> 	allow_resched();
> 	spin_lock();
> 	...
> 	spin_unlock();
> 	disallow_resched();
>
> Which on a PREEMPT_COUNT=n build will cause preemption while holding the
> spinlock. I think something like the below will cause sufficient
> warnings to avoid growing patterns like that.

Yeah, I agree this is a problem. I'll expand on the comment above
allow_resched() detailing this scenario.

> Index: linux-2.6/kernel/sched/core.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched/core.c
> +++ linux-2.6/kernel/sched/core.c
> @@ -5834,6 +5834,13 @@ void preempt_count_add(int val)
>  {
>  #ifdef CONFIG_DEBUG_PREEMPT
>  	/*
> +	 * Disabling preemption under TIF_RESCHED_ALLOW doesn't
> +	 * work for PREEMPT_COUNT=n builds.
> +	 */
> +	if (WARN_ON(resched_allowed()))
> +		return;
> +
> +	/*
>  	 * Underflow?
>  	 */
>  	if (DEBUG_LOCKS_WARN_ON((preempt_count() < 0)))

And, maybe something like this to guard against __this_cpu_read()
etc:

diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
index a2bb7738c373..634788f16e9e 100644
--- a/lib/smp_processor_id.c
+++ b/lib/smp_processor_id.c
@@ -13,6 +13,9 @@ unsigned int check_preemption_disabled(const char *what1, const char *what2)
 {
        int this_cpu = raw_smp_processor_id();

+       if (unlikely(resched_allowed()))
+               goto out_error;
+
        if (likely(preempt_count()))
                goto out;

@@ -33,6 +36,7 @@ unsigned int check_preemption_disabled(const char *what1, const char *what2)
        if (system_state < SYSTEM_SCHEDULING)
                goto out;

+out_error:
        /*
         * Avoid recursion:
         */

--
ankur