Avoid resched_cpu() when rescheduling the current CPU

Hello, Peter,

It occurred to me that it is wasteful to let resched_cpu() acquire ->pi_lock when doing something like resched_cpu(smp_processor_id()),

and that it would be better to instead use set_tsk_need_resched(current) and set_preempt_need_resched().

But is doing so really worthwhile? For that matter, are there some constraints on the use of those two functions that I am failing to allow for in the patch below?

The resched_cpu() interface is quite handy, but it does acquire the specified CPU's runqueue lock, which does not come for free. This commit therefore substitutes the following when directing resched_cpu() at the current CPU:

set_tsk_need_resched(current);
set_preempt_need_resched();

Quoted 4 lines · Paul E. McKenney

Hello, Peter,

It occurred to me that it is wasteful to let resched_cpu() acquire ->pi_lock when doing something like resched_cpu(smp_processor_id()),

rq->lock

Quoted 13 lines

and that it would be better to instead use set_tsk_need_resched(current) and set_preempt_need_resched().

But is doing so really worthwhile? For that matter, are there some constraints on the use of those two functions that I am failing to allow for in the patch below?

The resched_cpu() interface is quite handy, but it does acquire the specified CPU's runqueue lock, which does not come for free. This commit therefore substitutes the following when directing resched_cpu() at the current CPU:

Quoted 2 lines

            set_tsk_need_resched(current);
            set_preempt_need_resched();

That is only a valid substitute for resched_cpu(smp_processor_id()).

But also note how this can cause more context switches over resched_curr() for not checking if TIF_NEED_RESCHED wasn't already set.

Something that might be more in line with
resched_curr(smp_processor_id()) would be:

	preempt_disable();
	if (!test_tsk_need_resched(current)) {
		set_tsk_need_resched(current);
		set_preempt_need_resched();
	}
	preempt_enable();

Where the preempt_enable() could of course instantly trigger the reschedule if it was the outer most one.

kernel/rcu/tree.c static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused · line 2678

Quoted 6 lines

kernel/rcu/tree.c +2 -1

@@ -2674,10 +2675,12 @@ static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused
 
-		resched_cpu(rdp->cpu); /* Provoke future context switch. */
 
+		set_tsk_need_resched(current);
+		set_preempt_need_resched();

That's not obviously correct. rdp->cpu had better be smp_processor_id().

kernel/rcu/tree_exp.h static void sync_rcu_exp_handler(void *unused) · line 676

Quoted 7 lines

kernel/rcu/tree_exp.h +2 -1

@@ -672,7 +672,8 @@ static void sync_rcu_exp_handler(void *unused)
 			rcu_report_exp_rdp(rdp);
 		} else {
 			rdp->deferred_qs = true;
-			resched_cpu(rdp->cpu);
+			set_tsk_need_resched(t);
+			set_preempt_need_resched();

That only works if @t == current.

kernel/rcu/tree_exp.h static void sync_rcu_exp_handler(void *unused) · line 722

Quoted 9 lines

kernel/rcu/tree_exp.h +3 -2

 		}
 		return;
 	}
 
-	else
-		resched_cpu(rdp->cpu);
+	} else {
+		set_tsk_need_resched(t);
+		set_preempt_need_resched();

Similar...

kernel/rcu/tree_plugin.h static void rcu_flavor_check_callbacks(int user) · line 797

Quoted 12 lines

}

kernel/rcu/tree_plugin.h +4 -2

@@ -791,8 +791,10 @@ static void rcu_flavor_check_callbacks(int user)
 	if (t->rcu_read_lock_nesting > 0 ||
 	    (preempt_count() & (PREEMPT_MASK | SOFTIRQ_MASK))) {
 		/* No QS, force context switch if deferred. */
-		if (rcu_preempt_need_deferred_qs(t))
-			resched_cpu(smp_processor_id());
+		if (rcu_preempt_need_deferred_qs(t)) {
+			set_tsk_need_resched(t);
+			set_preempt_need_resched();
+		}

And another dodgy one..

Quoted 10 lines

Something that might be more in line with
resched_curr(smp_processor_id()) would be:

	preempt_disable();
	if (!test_tsk_need_resched(current)) {
		set_tsk_need_resched(current);
		set_preempt_need_resched();
	}
	preempt_enable();

Quoted 2 lines

Where the preempt_enable() could of course instantly trigger the reschedule if it was the outer most one.

Ah. So should I use resched_curr() from rcu_check_callbacks(), which is invoked from the scheduling-clock interrupt? Right now I have calls to set_tsk_need_resched() and set_preempt_need_resched().

Quoted 8 lines

kernel/rcu/tree.c static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused · line 2678

Quoted 6 lines

kernel/rcu/tree.c +2 -1

@@ -2674,10 +2675,12 @@ static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused
 
-		resched_cpu(rdp->cpu); /* Provoke future context switch. */
 
+		set_tsk_need_resched(current);
+		set_preempt_need_resched();

That's not obviously correct. rdp->cpu had better be smp_processor_id().

At the beginning of the function, we have:

	struct rcu_data *rdp = raw_cpu_ptr(&rcu_data);

And this is in a softirq handler, so we are OK.

Quoted 9 lines

kernel/rcu/tree_exp.h static void sync_rcu_exp_handler(void *unused) · line 676

Quoted 7 lines

kernel/rcu/tree_exp.h +2 -1

@@ -672,7 +672,8 @@ static void sync_rcu_exp_handler(void *unused)
 			rcu_report_exp_rdp(rdp);
 		} else {
 			rdp->deferred_qs = true;
-			resched_cpu(rdp->cpu);
+			set_tsk_need_resched(t);
+			set_preempt_need_resched();

That only works if @t == current.

At the beginning of the function, we have:

	struct task_struct *t = current;

So we should be OK.

Should I be instead using resched_curr() on some or all of these?

Quoted 26 lines · Paul E. McKenney

Quoted 10 lines

Something that might be more in line with
resched_curr(smp_processor_id()) would be:

	preempt_disable();
	if (!test_tsk_need_resched(current)) {
		set_tsk_need_resched(current);
		set_preempt_need_resched();
	}
	preempt_enable();

Quoted 2 lines

Where the preempt_enable() could of course instantly trigger the reschedule if it was the outer most one.

Ah. So should I use resched_curr() from rcu_check_callbacks(), which is invoked from the scheduling-clock interrupt? Right now I have calls to set_tsk_need_resched() and set_preempt_need_resched().

Quoted 8 lines

kernel/rcu/tree.c static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused · line 2678

Quoted 6 lines

kernel/rcu/tree.c +2 -1

@@ -2674,10 +2675,12 @@ static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused
 
-		resched_cpu(rdp->cpu); /* Provoke future context switch. */
 
+		set_tsk_need_resched(current);
+		set_preempt_need_resched();

That's not obviously correct. rdp->cpu had better be smp_processor_id().

Quoted 5 lines

At the beginning of the function, we have:

	struct rcu_data *rdp = raw_cpu_ptr(&rcu_data);

And this is in a softirq handler, so we are OK.

Agreed.

Quoted 10 lines

Quoted 9 lines

kernel/rcu/tree_exp.h static void sync_rcu_exp_handler(void *unused) · line 676

Quoted 7 lines

kernel/rcu/tree_exp.h +2 -1

@@ -672,7 +672,8 @@ static void sync_rcu_exp_handler(void *unused)
 			rcu_report_exp_rdp(rdp);
 		} else {
 			rdp->deferred_qs = true;
-			resched_cpu(rdp->cpu);
+			set_tsk_need_resched(t);
+			set_preempt_need_resched();

That only works if @t == current.

Quoted 5 lines

At the beginning of the function, we have:

	struct task_struct *t = current;

So we should be OK.

Ah, the scheduler and locking code typically use to call that curr, to be more explicit that it is the current task.

Quoted 1 line

Should I be instead using resched_curr() on some or all of these?

If, as it seems is the case, they are all targeting the current cpu and have (soft) interrupts disabled, then what you propose is indeed fine.