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
