Re: [PATCH v4 4/4] workqueue: Unbind workers before sending them to exit()

Re: [PATCH v4 4/4] workqueue: Unbind workers before sending them to exit()

[Hillf Danton]

On 4 Oct 2022 16:05:21 +0100 Valentin Schneider <vschneid@redhat.com>
> It has been reported that isolated CPUs can suffer from interference due to
> per-CPU kworkers waking up just to die.
> 
> A surge of workqueue activity during initial setup of a latency-sensitive
> application (refresh_vm_stats() being one of the culprits) can cause extra
> per-CPU kworkers to be spawned. Then, said latency-sensitive task can be
> running merrily on an isolated CPU only to be interrupted sometime later by
> a kworker marked for death (cf. IDLE_WORKER_TIMEOUT, 5 minutes after last
> kworker activity).
> 
Is tick stopped on the isolated CPU? If tick can hit it then it can accept
more than exiting kworker. Another option is exclude isolated CPUs from
active CPUs because workqueue has other works to do than isolating CPUs.

> Prevent this by affining kworkers to the wq_unbound_cpumask (which doesn't
> contain isolated CPUs, cf. HK_TYPE_WQ) before waking them up after marking
> them with WORKER_DIE.
> 

Re: [PATCH v4 4/4] workqueue: Unbind workers before sending them to exit()

[Valentin Schneider]

On 05/10/22 09:08, Hillf Danton wrote:
> On 4 Oct 2022 16:05:21 +0100 Valentin Schneider <vschneid@redhat.com>
>> It has been reported that isolated CPUs can suffer from interference due to
>> per-CPU kworkers waking up just to die.
>>
>> A surge of workqueue activity during initial setup of a latency-sensitive
>> application (refresh_vm_stats() being one of the culprits) can cause extra
>> per-CPU kworkers to be spawned. Then, said latency-sensitive task can be
>> running merrily on an isolated CPU only to be interrupted sometime later by
>> a kworker marked for death (cf. IDLE_WORKER_TIMEOUT, 5 minutes after last
>> kworker activity).
>>
> Is tick stopped on the isolated CPU? If tick can hit it then it can accept
> more than exiting kworker.

From what I've seen in the scenarios where that happens, yes. The
pool->idle_timer gets queued from an isolated CPU and ends up on a
housekeeping CPU (cf. get_target_base()).


> Another option is exclude isolated CPUs from
> active CPUs because workqueue has other works to do than isolating CPUs.
>

With nohz_full on the cmdline, wq_unbound_cpumask already excludes isolated
CPU, but that doesn't apply to per-CPU kworkers. Or did you mean some other
mechanism?

>> Prevent this by affining kworkers to the wq_unbound_cpumask (which doesn't
>> contain isolated CPUs, cf. HK_TYPE_WQ) before waking them up after marking
>> them with WORKER_DIE.
>>

Re: [PATCH v4 4/4] workqueue: Unbind workers before sending them to exit()

[Hillf Danton]

On 05 Oct 2022 12:13:17 +0100 Valentin Schneider <vschneid@redhat.com>
>On 05/10/22 09:08, Hillf Danton wrote:
>> On 4 Oct 2022 16:05:21 +0100 Valentin Schneider <vschneid@redhat.com>
>>> It has been reported that isolated CPUs can suffer from interference due to
>>> per-CPU kworkers waking up just to die.
>>>
>>> A surge of workqueue activity during initial setup of a latency-sensitive
>>> application (refresh_vm_stats() being one of the culprits) can cause extra
>>> per-CPU kworkers to be spawned. Then, said latency-sensitive task can be
>>> running merrily on an isolated CPU only to be interrupted sometime later by
>>> a kworker marked for death (cf. IDLE_WORKER_TIMEOUT, 5 minutes after last
>>> kworker activity).
>>>
>> Is tick stopped on the isolated CPU? If tick can hit it then it can accept
>> more than exiting kworker.
>
>From what I've seen in the scenarios where that happens, yes. The
>pool->idle_timer gets queued from an isolated CPU and ends up on a
>housekeeping CPU (cf. get_target_base()).

Yes, you are right.

>With nohz_full on the cmdline, wq_unbound_cpumask already excludes isolated
>CPU, but that doesn't apply to per-CPU kworkers. Or did you mean some other
>mechanism?

Bound kworkers can be destroyed by the idle timer on a housekeeping CPU.

Diff is only for thoughts.

+++ b/kernel/workqueue.c
@@ -1985,6 +1985,7 @@ fail:
 static void destroy_worker(struct worker *worker)
 {
 	struct worker_pool *pool = worker->pool;
+	int cpu = smp_processor_id();
 
 	lockdep_assert_held(&pool->lock);
 
@@ -1999,6 +2000,12 @@ static void destroy_worker(struct worker
 
 	list_del_init(&worker->entry);
 	worker->flags |= WORKER_DIE;
+
+	if (!(pool->flags & POOL_DISASSOCIATED) && pool->cpu != cpu) {
+		/* send worker to die on a housekeeping cpu */
+		cpumask_clear(&worker->task->cpus_mask);
+		cpumask_set_cpu(cpu, &worker->task->cpus_mask);
+	}
 	wake_up_process(worker->task);
 }
 

Re: [PATCH v4 4/4] workqueue: Unbind workers before sending them to exit()

[Valentin Schneider]

On 05/10/22 22:50, Hillf Danton wrote:
> On 05 Oct 2022 12:13:17 +0100 Valentin Schneider <vschneid@redhat.com>
>
> Bound kworkers can be destroyed by the idle timer on a housekeeping CPU.
>
> Diff is only for thoughts.
>
> +++ b/kernel/workqueue.c
> @@ -1985,6 +1985,7 @@ fail:
>  static void destroy_worker(struct worker *worker)
>  {
>       struct worker_pool *pool = worker->pool;
> +	int cpu = smp_processor_id();
>
>       lockdep_assert_held(&pool->lock);
>
> @@ -1999,6 +2000,12 @@ static void destroy_worker(struct worker
>
>       list_del_init(&worker->entry);
>       worker->flags |= WORKER_DIE;
> +
> +	if (!(pool->flags & POOL_DISASSOCIATED) && pool->cpu != cpu) {
> +		/* send worker to die on a housekeeping cpu */
> +		cpumask_clear(&worker->task->cpus_mask);
> +		cpumask_set_cpu(cpu, &worker->task->cpus_mask);
> +	}
>       wake_up_process(worker->task);
>  }
>

The proper interface to play with that cpumask is set_cpus_allowed_ptr(),
which requires a sleepable context, hence the whole series.

Re: [PATCH v4 4/4] workqueue: Unbind workers before sending them to exit()

[Hillf Danton]

On 05 Oct 2022 17:14:17 +0100 Valentin Schneider <vschneid@redhat.com>
> 
> The proper interface to play with that cpumask is set_cpus_allowed_ptr(),
> which requires a sleepable context, hence the whole series.

Given "nohz_full is a gimmick that shouldn't be used outside of very specific
cases. Regular nohz otoh is used by everybody always." [1], I will take
another look at this series next week.

[1] https://lore.kernel.org/lkml/YzXGdEzkiw+5X8pC@hirez.programming.kicks-ass.net/