Re: question re klp_transition_work kickoff timeout

[Alexey Dobriyan <adobriyan@xxxxxxxxx>]

On Wed, Mar 22, 2023 at 05:07:09PM +0100, Miroslav Benes wrote:
> On Wed, 22 Mar 2023, Alexey Dobriyan wrote:
> 
> > Hi, Josh.
> > 
> > I've been profiling how much time livepatching takes and I have a question
> > regarding these lines:
> > 
> > 	void klp_try_complete_transition(void)
> > 	{
> > 		...
> > 		if (!complete) {
> > 			schedule_delayed_work(&klp_transition_work, round_jiffies_relative(HZ));
> > 			return;
> > 		}
> > 
> > 	}
> > 
> > The problem here is that if the system is idle, then the previous loop
> > checking idle tasks will reliably sets "complete = false" and then
> > patching wastes time waiting for next second so that klp_transition_work
> > will repeat the same code without reentering itself.
> 
> Only if klp_try_switch_task() cannot switch the idle task right away. We 
> then prod it using wake_up_if_idle() and handle it in the next iteration.
> 
> So the question might be why klp_try_switch_task() did not succeed in the 
> first place.

Yes, sort of. Transitioning never happens on the first try becase of idle
tasks (see debugging patch below). But it should happen immediately
because machine is idle!

> > I've created trivial patch which changes 2 functions and it takes
> > ~1.3 seconds to complete transition:
> > 
> > 	[   33.829506] livepatch: 'main': starting patching transition
> > 	[   35.190721] livepatch: 'main': patching complete
> > 
> > I don't know what's the correct timeout to retry transition work
> > but it can be made near-instant:
> > 
> > 	[  100.930758] livepatch: 'main': starting patching transition
> > 	[  100.956190] livepatch: 'main': patching complete
> 
> There is really no correct timeout. The application of a live patch is 
> a lazy slow path. We generally do not care when it is finished unless it 
> is stuck for some reason or takes really long.
> 
> What is your motivation in measuring it?

Just measuring... :-)

	main: tainting kernel with TAINT_LIVEPATCH
	livepatch: enabling patch 'main'
	livepatch: 'main': starting patching transition
	livepatch: XXX 002 klp_try_switch_task 'swapper/0'
		...
	livepatch: XXX 002 klp_try_switch_task 'swapper/31'
	livepatch: XXX nr_a 0, nr_b 30
	livepatch: 'main': patching complete

--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -386,7 +386,8 @@ void klp_try_complete_transition(void)
 	unsigned int cpu;
 	struct task_struct *g, *task;
 	struct klp_patch *patch;
-	bool complete = true;
+	unsigned int nr_a = 0;
+	unsigned int nr_b = 0;
 
 	WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
 
@@ -401,8 +402,10 @@ void klp_try_complete_transition(void)
 	 */
 	read_lock(&tasklist_lock);
 	for_each_process_thread(g, task)
-		if (!klp_try_switch_task(task))
-			complete = false;
+		if (!klp_try_switch_task(task)) {
+			pr_err("XXX 001 klp_try_switch_task '%s'\n", task->comm);
+			nr_a += 1;
+		}
 	read_unlock(&tasklist_lock);
 
 	/*
@@ -413,7 +416,8 @@ void klp_try_complete_transition(void)
 		task = idle_task(cpu);
 		if (cpu_online(cpu)) {
 			if (!klp_try_switch_task(task)) {
-				complete = false;
+				pr_err("XXX 002 klp_try_switch_task '%s'\n", task->comm);
+				nr_b += 1;
 				/* Make idle task go through the main loop. */
 				wake_up_if_idle(cpu);
 			}
@@ -425,7 +429,9 @@ void klp_try_complete_transition(void)
 	}
 	cpus_read_unlock();
 
-	if (!complete) {
+	if (nr_a > 0 || nr_b > 0) {
+		pr_err("XXX nr_a %u, nr_b %d\n", nr_a, nr_b);
+
 		if (klp_signals_cnt && !(klp_signals_cnt % SIGNALS_TIMEOUT))
 			klp_send_signals();
 		klp_signals_cnt++;