[PATCH 0/2] sched: improve task_mm_cid_work impact on isolated systems

[PATCH 0/2] sched: improve task_mm_cid_work impact on isolated systems

[Gabriele Monaco]

This patchset introduces two small changes to make the task_mm_cid_work
lighter and less problematic for RT tasks.

We observed moderate latency spikes in a system with isolated cores but
multiple tasks running on those cores (e.g. one stressor and one
measuring thread).

If the nohz tick occurs during the measuring thread's execution (i.e.
the RT task), the task work calling task_mm_cid_work alone can take
around 30-35us, this is above the requirements for isolated cores.

The first patch reduces the runtime of the task by lowering the number
of cores that are checked during CID cleanup. Instead of iterating over
all possible cores, we only check the ones defined by the CID mask.

The second patch moves the work in a preemptible context (RCU callback),
making it harmless towards RT tasks.

We run the benchmark on a 128-core aarch64 box with 4 housekeping cores
and 124 (1-31,33-63,65-95,97-127) isolated cores.

Each isolated core is running an instance of stress-ng:
  # (foreach N in 1-31,33-63,65-95,97-127)
  # taskset -c N stress-ng --cpu 1 --cpu-load 80
And an rtla timerlat measuring thread (besides the first isolated core
running the main timerlat thread):
  # cpus=2-31,33-63,65-95,97-127
  # rtla timerlat top -q -P f:95 -c $cpus -H 1

Our 30min test run without this patch reaches a maximum latency on one
core (say cpu 113) of 48us.

After this patch, we get a latency below 20us on all cores.

Gabriele Monaco (2):
  sched: Optimise task_mm_cid_work duration
  sched: Move task_mm_cid_work to RCU callback

 include/linux/sched.h |  1 -
 kernel/sched/core.c   | 21 ++++++++-------------
 2 files changed, 8 insertions(+), 14 deletions(-)


base-commit: e70140ba0d2b1a30467d4af6bcfe761327b9ec95
-- 
2.47.0

[PATCH 1/2] sched: Optimise task_mm_cid_work duration

[Gabriele Monaco]

The current behaviour of task_mm_cid_work is to loop through all
possible CPUs twice to clean up old mm_cid remotely, this can be a waste
of resources especially on tasks with a CPU affinity.

This patch reduces the CPUs involved in the remote CID cleanup carried
on by task_mm_cid_work.

Using the mm_cidmask for the remote cleanup can considerably reduce the
function runtime in highly isolated environments, where each process has
affinity to a single core.  Likewise, in the worst case, the mask is
equivalent to all possible CPUs and we don't see any difference with the
current behaviour.

Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
---
 kernel/sched/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 95e40895a519..57b50b5952fa 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -10553,14 +10553,14 @@ static void task_mm_cid_work(struct callback_head *work)
 		return;
 	cidmask = mm_cidmask(mm);
 	/* Clear cids that were not recently used. */
-	for_each_possible_cpu(cpu)
+	for_each_cpu_from(cpu, cidmask)
 		sched_mm_cid_remote_clear_old(mm, cpu);
 	weight = cpumask_weight(cidmask);
 	/*
 	 * Clear cids that are greater or equal to the cidmask weight to
 	 * recompact it.
 	 */
-	for_each_possible_cpu(cpu)
+	for_each_cpu_from(cpu, cidmask)
 		sched_mm_cid_remote_clear_weight(mm, cpu, weight);
 }
 
-- 
2.47.0

Re: [PATCH 1/2] sched: Optimise task_mm_cid_work duration

[Mathieu Desnoyers]

On 2024-12-02 09:07, Gabriele Monaco wrote:
> The current behaviour of task_mm_cid_work is to loop through all
> possible CPUs twice to clean up old mm_cid remotely, this can be a waste
> of resources especially on tasks with a CPU affinity.
> 
> This patch reduces the CPUs involved in the remote CID cleanup carried
> on by task_mm_cid_work.
> 
> Using the mm_cidmask for the remote cleanup can considerably reduce the
> function runtime in highly isolated environments, where each process has
> affinity to a single core.  Likewise, in the worst case, the mask is
> equivalent to all possible CPUs and we don't see any difference with the
> current behaviour.
> 
> Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
> ---
>   kernel/sched/core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 95e40895a519..57b50b5952fa 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -10553,14 +10553,14 @@ static void task_mm_cid_work(struct callback_head *work)
>   		return;
>   	cidmask = mm_cidmask(mm);
>   	/* Clear cids that were not recently used. */
> -	for_each_possible_cpu(cpu)
> +	for_each_cpu_from(cpu, cidmask)

Hi Gabriele,

Thanks for looking into this. I understand that you are after minimizing the
latency introduced by task_mm_cid_work on isolated cores. I think we'll need
to think a bit harder, because the proposed solution does not work:

  * for_each_cpu_from - iterate over CPUs present in @mask, from @cpu to the end of @mask.

cpu is uninitialized. So this is completely broken. Was this tested
against a workload that actually uses concurrency IDs to ensure it does
not break the whole thing ? Did you run the rseq selftests ?

Also, the mm_cidmask is a mask of concurrency IDs, not a mask of CPUs. So
using it to iterate on CPUs is wrong.

Mathieu

>   		sched_mm_cid_remote_clear_old(mm, cpu);
>   	weight = cpumask_weight(cidmask);
>   	/*
>   	 * Clear cids that are greater or equal to the cidmask weight to
>   	 * recompact it.
>   	 */
> -	for_each_possible_cpu(cpu)
> +	for_each_cpu_from(cpu, cidmask)
>   		sched_mm_cid_remote_clear_weight(mm, cpu, weight);
>   }
>   

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH 1/2] sched: Optimise task_mm_cid_work duration

[Gabriele Monaco]

Hi Mathieu,

thanks for the quick reply.

> Thanks for looking into this. I understand that you are after
> minimizing the
> latency introduced by task_mm_cid_work on isolated cores. I think
> we'll need
> to think a bit harder, because the proposed solution does not work:
> 
>   * for_each_cpu_from - iterate over CPUs present in @mask, from @cpu
> to the end of @mask.
> 
> cpu is uninitialized. So this is completely broken. 

My bad, wrong macro.. Should be for_each_cpu

> Was this tested
> against a workload that actually uses concurrency IDs to ensure it
> does
> not break the whole thing ? Did you run the rseq selftests ?
> 

I did run the stress-ng --rseq command for a while and didn't see any
error reported, but it's probably not bulletproof. I'll use the
selftests for the next iterations.

> Also, the mm_cidmask is a mask of concurrency IDs, not a mask of
> CPUs. So
> using it to iterate on CPUs is wrong.
> 

Mmh I get it, during my tests I was definitely getting better results
than using the mm_cpus_allowed mask, but I guess that was a broken test
so it just doesn't count..
Do you think using mm_cpus_allowed would make more sense, with the
/risk/ of being a bit over-cautious?


Gabriele

Re: [PATCH 1/2] sched: Optimise task_mm_cid_work duration

[Mathieu Desnoyers]

On 2024-12-02 09:56, Gabriele Monaco wrote:
> Hi Mathieu,
> 
> thanks for the quick reply.
> 
>> Thanks for looking into this. I understand that you are after
>> minimizing the
>> latency introduced by task_mm_cid_work on isolated cores. I think
>> we'll need
>> to think a bit harder, because the proposed solution does not work:
>>
>>    * for_each_cpu_from - iterate over CPUs present in @mask, from @cpu
>> to the end of @mask.
>>
>> cpu is uninitialized. So this is completely broken.
> 
> My bad, wrong macro.. Should be for_each_cpu
> 
>> Was this tested
>> against a workload that actually uses concurrency IDs to ensure it
>> does
>> not break the whole thing ? Did you run the rseq selftests ?
>>
> 
> I did run the stress-ng --rseq command for a while and didn't see any
> error reported, but it's probably not bulletproof. I'll use the
> selftests for the next iterations.
> 
>> Also, the mm_cidmask is a mask of concurrency IDs, not a mask of
>> CPUs. So
>> using it to iterate on CPUs is wrong.
>>
> 
> Mmh I get it, during my tests I was definitely getting better results
> than using the mm_cpus_allowed mask, but I guess that was a broken test
> so it just doesn't count..
> Do you think using mm_cpus_allowed would make more sense, with the
> /risk/ of being a bit over-cautious?

mm_cpus_allowed can be updated dynamically by setting cpu affinity
and changing the cpusets. If we change the iteration from each possible
cpus to allowed cpus, then we need to adapt the allowed cpus updates
with the associated updates to the mm_cid as well. This is adding
complexity.

I understand that you wish to offload this task_work to a non-isolated
CPU (non-RT). If you do so, do you really care about the duration of
task_mm_cid_work enough to justify the added complexity to the
cpu affinity/cpusets updates ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH 1/2] sched: Optimise task_mm_cid_work duration

[Gabriele Monaco]

On Mon, 2024-12-02 at 10:01 -0500, Mathieu Desnoyers wrote:
> 
> mm_cpus_allowed can be updated dynamically by setting cpu affinity
> and changing the cpusets. If we change the iteration from each
> possible
> cpus to allowed cpus, then we need to adapt the allowed cpus updates
> with the associated updates to the mm_cid as well. This is adding
> complexity.
> 
> I understand that you wish to offload this task_work to a non-
> isolated
> CPU (non-RT). If you do so, do you really care about the duration of
> task_mm_cid_work enough to justify the added complexity to the
> cpu affinity/cpusets updates ?
> 

Well, no we don't really care at this point.. If it isn't a quick win
I'd say I can simply remove it from the patchset, for the current use
case it doesn't really matter.

Thanks for the analysis.
Gabriele

Re: [PATCH 1/2] sched: Optimise task_mm_cid_work duration

[kernel test robot]

Hi Gabriele,

kernel test robot noticed the following build warnings:

[auto build test WARNING on e70140ba0d2b1a30467d4af6bcfe761327b9ec95]

url:    https://github.com/intel-lab-lkp/linux/commits/Gabriele-Monaco/sched-Optimise-task_mm_cid_work-duration/20241203-003033
base:   e70140ba0d2b1a30467d4af6bcfe761327b9ec95
patch link:    https://lore.kernel.org/r/20241202140735.56368-2-gmonaco%40redhat.com
patch subject: [PATCH 1/2] sched: Optimise task_mm_cid_work duration
config: arm64-randconfig-004-20241203 (https://download.01.org/0day-ci/archive/20241203/202412031029.B7lLh63F-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241203/202412031029.B7lLh63F-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412031029.B7lLh63F-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from kernel/sched/core.c:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/arm64/include/asm/cacheflush.h:11:
   In file included from include/linux/kgdb.h:19:
   In file included from include/linux/kprobes.h:28:
   In file included from include/linux/ftrace.h:13:
   In file included from include/linux/kallsyms.h:13:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> kernel/sched/core.c:10552:6: warning: variable 'cpu' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    10552 |         if (!try_cmpxchg(&mm->mm_cid_next_scan, &old_scan, next_scan))
          |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   kernel/sched/core.c:10556:20: note: uninitialized use occurs here
    10556 |         for_each_cpu_from(cpu, cidmask)
          |                           ^~~
   include/linux/cpumask.h:391:24: note: expanded from macro 'for_each_cpu_from'
     391 |         for_each_set_bit_from(cpu, cpumask_bits(mask), small_cpumask_bits)
         |                               ^~~
   include/linux/find.h:605:48: note: expanded from macro 'for_each_set_bit_from'
     605 |         for (; (bit) = find_next_bit((addr), (size), (bit)), (bit) < (size); (bit)++)
         |                                                       ^~~
   kernel/sched/core.c:10552:2: note: remove the 'if' if its condition is always true
    10552 |         if (!try_cmpxchg(&mm->mm_cid_next_scan, &old_scan, next_scan))
          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    10553 |                 return;
   kernel/sched/core.c:10529:17: note: initialize the variable 'cpu' to silence this warning
    10529 |         int weight, cpu;
          |                        ^
          |                         = 0
   kernel/sched/core.c:1809:1: warning: unused function 'uclamp_update_active' [-Wunused-function]
    1809 | uclamp_update_active(struct task_struct *p)
         | ^~~~~~~~~~~~~~~~~~~~
   kernel/sched/core.c:6430:1: warning: unused function 'class_core_lock_lock_ptr' [-Wunused-function]
    6430 | DEFINE_LOCK_GUARD_1(core_lock, int,
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    6431 |                     sched_core_lock(*_T->lock, &_T->flags),
         |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    6432 |                     sched_core_unlock(*_T->lock, &_T->flags),
         |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    6433 |                     unsigned long flags)
         |                     ~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:416:49: note: expanded from macro 'DEFINE_LOCK_GUARD_1'
     416 | __DEFINE_CLASS_IS_CONDITIONAL(_name, false);                            \
         |                                                                         ^
     417 | __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__)               \
         | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/cleanup.h:392:21: note: expanded from macro '\
   __DEFINE_UNLOCK_GUARD'
     392 | static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T)     \
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~
   <scratch space>:63:1: note: expanded from here
      63 | class_core_lock_lock_ptr
         | ^~~~~~~~~~~~~~~~~~~~~~~~
   4 warnings generated.


vim +10552 kernel/sched/core.c

223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10522  
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10523  static void task_mm_cid_work(struct callback_head *work)
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10524  {
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10525  	unsigned long now = jiffies, old_scan, next_scan;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10526  	struct task_struct *t = current;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10527  	struct cpumask *cidmask;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10528  	struct mm_struct *mm;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10529  	int weight, cpu;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10530  
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10531  	SCHED_WARN_ON(t != container_of(work, struct task_struct, cid_work));
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10532  
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10533  	work->next = work;	/* Prevent double-add */
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10534  	if (t->flags & PF_EXITING)
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10535  		return;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10536  	mm = t->mm;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10537  	if (!mm)
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10538  		return;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10539  	old_scan = READ_ONCE(mm->mm_cid_next_scan);
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10540  	next_scan = now + msecs_to_jiffies(MM_CID_SCAN_DELAY);
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10541  	if (!old_scan) {
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10542  		unsigned long res;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10543  
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10544  		res = cmpxchg(&mm->mm_cid_next_scan, old_scan, next_scan);
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10545  		if (res != old_scan)
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10546  			old_scan = res;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10547  		else
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10548  			old_scan = next_scan;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10549  	}
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10550  	if (time_before(now, old_scan))
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10551  		return;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20 @10552  	if (!try_cmpxchg(&mm->mm_cid_next_scan, &old_scan, next_scan))
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10553  		return;
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10554  	cidmask = mm_cidmask(mm);
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10555  	/* Clear cids that were not recently used. */
2909dedb7586e2c Gabriele Monaco   2024-12-02  10556  	for_each_cpu_from(cpu, cidmask)
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10557  		sched_mm_cid_remote_clear_old(mm, cpu);
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10558  	weight = cpumask_weight(cidmask);
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10559  	/*
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10560  	 * Clear cids that are greater or equal to the cidmask weight to
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10561  	 * recompact it.
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10562  	 */
2909dedb7586e2c Gabriele Monaco   2024-12-02  10563  	for_each_cpu_from(cpu, cidmask)
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10564  		sched_mm_cid_remote_clear_weight(mm, cpu, weight);
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10565  }
223baf9d17f25e2 Mathieu Desnoyers 2023-04-20  10566  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/2] sched: Move task_mm_cid_work to RCU callback

[Gabriele Monaco]

Currently, the task_mm_cid_work function is called in a task work
triggered by a scheduler tick. This can delay the execution of the
task for the entire duration of the function.

This patch runs the task_mm_cid_work in the RCU callback thread rather
than in the task context before returning to userspace.

The main advantage of this change is that the function can be offloaded
to a different CPU and even preempted by RT tasks.

On a busy system, this may mean the function gets called less often, but
the current behaviour already doesn't provide guarantees.

Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
---
 include/linux/sched.h |  1 -
 kernel/sched/core.c   | 17 ++++++-----------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index d380bffee2ef..5d141c310917 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1374,7 +1374,6 @@ struct task_struct {
 	int				last_mm_cid;	/* Most recent cid in mm */
 	int				migrate_from_cpu;
 	int				mm_cid_active;	/* Whether cid bitmap is active */
-	struct callback_head		cid_work;
 #endif
 
 	struct tlbflush_unmap_batch	tlb_ubc;
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 57b50b5952fa..0fc1a972fd4f 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -10520,17 +10520,15 @@ static void sched_mm_cid_remote_clear_weight(struct mm_struct *mm, int cpu,
 	sched_mm_cid_remote_clear(mm, pcpu_cid, cpu);
 }
 
-static void task_mm_cid_work(struct callback_head *work)
+static void task_mm_cid_work(struct rcu_head *rhp)
 {
 	unsigned long now = jiffies, old_scan, next_scan;
-	struct task_struct *t = current;
+	struct task_struct *t = container_of(rhp, struct task_struct, rcu);
 	struct cpumask *cidmask;
 	struct mm_struct *mm;
 	int weight, cpu;
 
-	SCHED_WARN_ON(t != container_of(work, struct task_struct, cid_work));
-
-	work->next = work;	/* Prevent double-add */
+	rhp->next = rhp;	/* Prevent double-add */
 	if (t->flags & PF_EXITING)
 		return;
 	mm = t->mm;
@@ -10574,23 +10572,20 @@ void init_sched_mm_cid(struct task_struct *t)
 		if (mm_users == 1)
 			mm->mm_cid_next_scan = jiffies + msecs_to_jiffies(MM_CID_SCAN_DELAY);
 	}
-	t->cid_work.next = &t->cid_work;	/* Protect against double add */
-	init_task_work(&t->cid_work, task_mm_cid_work);
 }
 
 void task_tick_mm_cid(struct rq *rq, struct task_struct *curr)
 {
-	struct callback_head *work = &curr->cid_work;
+	struct rcu_head *rhp = &curr->rcu;
 	unsigned long now = jiffies;
 
 	if (!curr->mm || (curr->flags & (PF_EXITING | PF_KTHREAD)) ||
-	    work->next != work)
+	    rhp->next != rhp)
 		return;
 	if (time_before(now, READ_ONCE(curr->mm->mm_cid_next_scan)))
 		return;
 
-	/* No page allocation under rq lock */
-	task_work_add(curr, work, TWA_RESUME | TWAF_NO_ALLOC);
+	call_rcu(rhp, task_mm_cid_work);
 }
 
 void sched_mm_cid_exit_signals(struct task_struct *t)
-- 
2.47.0

Re: [PATCH 2/2] sched: Move task_mm_cid_work to RCU callback

[Mathieu Desnoyers]

+= CC RCU maintainers, reviewers and list.
+= RSEQ maintainers.

On 2024-12-02 09:07, Gabriele Monaco wrote:
> Currently, the task_mm_cid_work function is called in a task work
> triggered by a scheduler tick. This can delay the execution of the
> task for the entire duration of the function.
> 
> This patch runs the task_mm_cid_work in the RCU callback thread rather
> than in the task context before returning to userspace.
> 
> The main advantage of this change is that the function can be offloaded
> to a different CPU and even preempted by RT tasks.
> 
> On a busy system, this may mean the function gets called less often, but
> the current behaviour already doesn't provide guarantees.

I've used the same task work pattern as NUMA here. What makes it
OK for NUMA and not for mm_cid ?

I wonder why we'd want to piggy-back on call_rcu here when
this has nothing to do with RCU. There is likely a characteristic
of the call_rcu worker threads that we want to import into
task_tick_mm_cid(), or change task_work.c to add a new flag
that says the work can be dispatched to any CPU.

> 
> Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
> ---
>   include/linux/sched.h |  1 -
>   kernel/sched/core.c   | 17 ++++++-----------
>   2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d380bffee2ef..5d141c310917 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1374,7 +1374,6 @@ struct task_struct {
>   	int				last_mm_cid;	/* Most recent cid in mm */
>   	int				migrate_from_cpu;
>   	int				mm_cid_active;	/* Whether cid bitmap is active */
> -	struct callback_head		cid_work;
>   #endif
>   
>   	struct tlbflush_unmap_batch	tlb_ubc;
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 57b50b5952fa..0fc1a972fd4f 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -10520,17 +10520,15 @@ static void sched_mm_cid_remote_clear_weight(struct mm_struct *mm, int cpu,
>   	sched_mm_cid_remote_clear(mm, pcpu_cid, cpu);
>   }
>   
> -static void task_mm_cid_work(struct callback_head *work)
> +static void task_mm_cid_work(struct rcu_head *rhp)
>   {
>   	unsigned long now = jiffies, old_scan, next_scan;
> -	struct task_struct *t = current;
> +	struct task_struct *t = container_of(rhp, struct task_struct, rcu);
>   	struct cpumask *cidmask;
>   	struct mm_struct *mm;
>   	int weight, cpu;
>   
> -	SCHED_WARN_ON(t != container_of(work, struct task_struct, cid_work));
> -
> -	work->next = work;	/* Prevent double-add */
> +	rhp->next = rhp;	/* Prevent double-add */
>   	if (t->flags & PF_EXITING)
>   		return;
>   	mm = t->mm;
> @@ -10574,23 +10572,20 @@ void init_sched_mm_cid(struct task_struct *t)
>   		if (mm_users == 1)
>   			mm->mm_cid_next_scan = jiffies + msecs_to_jiffies(MM_CID_SCAN_DELAY);
>   	}
> -	t->cid_work.next = &t->cid_work;	/* Protect against double add */
> -	init_task_work(&t->cid_work, task_mm_cid_work);
>   }
>   
>   void task_tick_mm_cid(struct rq *rq, struct task_struct *curr)
>   {
> -	struct callback_head *work = &curr->cid_work;
> +	struct rcu_head *rhp = &curr->rcu;

Why is it OK to re-use the task struct rcu field ? Where else is it
used, and is there a risk of being inserted twice ?

Thanks,

Mathieu

>   	unsigned long now = jiffies;
>   
>   	if (!curr->mm || (curr->flags & (PF_EXITING | PF_KTHREAD)) ||
> -	    work->next != work)
> +	    rhp->next != rhp)
>   		return;
>   	if (time_before(now, READ_ONCE(curr->mm->mm_cid_next_scan)))
>   		return;
>   
> -	/* No page allocation under rq lock */
> -	task_work_add(curr, work, TWA_RESUME | TWAF_NO_ALLOC);
> +	call_rcu(rhp, task_mm_cid_work);
>   }
>   
>   void sched_mm_cid_exit_signals(struct task_struct *t)

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH 2/2] sched: Move task_mm_cid_work to RCU callback

[Gabriele Monaco]

> I've used the same task work pattern as NUMA here. What makes it
> OK for NUMA and not for mm_cid ?
> 

I didn't investigate the behaviour with the NUMA work, but my rough
guess is that this wouldn't even be visible in an isolated environment
(i.e. no migrations).
Also it doesn't seem to scale linearly with the number of cores.

Your approach (or the NUMA's) isn't wrong, in my opinion, it just
doesn't necessarily require to run in that context.

In an environment with isolated CPUs, we want the lowest latency
possible, that kind of work before switching to userspace imposes a
latency that could simply be elsewhere, even on another core since we
are doing remote accesses.
We are talking about 35us on a rather big system, not many applications
are sensitive to that kind of latency.

> I wonder why we'd want to piggy-back on call_rcu here when
> this has nothing to do with RCU. There is likely a characteristic
> of the call_rcu worker threads that we want to import into
> task_tick_mm_cid(), or change task_work.c to add a new flag
> that says the work can be dispatched to any CPU.
> 

Alright, taking the RCU path was probably a bit lazy, another thought I
had was to run it in a workqueue, perhaps tied to the mm rather than to
the task struct itself. I'm also not entirely sure running it with the
scheduler tick is the best approach, since it doesn't seem quite
predictable, but I didn't really get the full requirements, so a
discussion on this can surely help.
 
> >   void task_tick_mm_cid(struct rq *rq, struct task_struct *curr)
> >   {
> > - struct callback_head *work = &curr->cid_work;
> > + struct rcu_head *rhp = &curr->rcu;
> 
> Why is it OK to re-use the task struct rcu field ? Where else is it
> used, and is there a risk of being inserted twice ?
> 

The same approach is used in
https://elixir.bootlin.com/linux/v6.12/source/include/linux/sched/task.h#L169
also there it was probably chosen for its simplicity and it isn't the
absolute best approach.
There may be a risk of messing things up, again this was the lazy path
and probably a more robust approach would work better.

Thanks for your comments.
Gabriele

Re: [PATCH 2/2] sched: Move task_mm_cid_work to RCU callback

[Joel Fernandes]

On Mon, Dec 2, 2024 at 9:34 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> += CC RCU maintainers, reviewers and list.
> += RSEQ maintainers.
>
> On 2024-12-02 09:07, Gabriele Monaco wrote:
> > Currently, the task_mm_cid_work function is called in a task work
> > triggered by a scheduler tick. This can delay the execution of the
> > task for the entire duration of the function.
> >
> > This patch runs the task_mm_cid_work in the RCU callback thread rather
> > than in the task context before returning to userspace.
> >
> > The main advantage of this change is that the function can be offloaded
> > to a different CPU and even preempted by RT tasks.
> >
> > On a busy system, this may mean the function gets called less often, but
> > the current behaviour already doesn't provide guarantees.
>
> I've used the same task work pattern as NUMA here. What makes it
> OK for NUMA and not for mm_cid ?
>
> I wonder why we'd want to piggy-back on call_rcu here when
> this has nothing to do with RCU. There is likely a characteristic
> of the call_rcu worker threads that we want to import into
> task_tick_mm_cid(), or change task_work.c to add a new flag
> that says the work can be dispatched to any CPU.

Also there is no guarantee that RCU callback will run within a thread
context (example, some configurations run it in softirq). Further,
call_rcu() usage as shown in this patch can also delay callback runs
by seconds (with RCU_LAZY enabled).

See also #5 in checklist: https://docs.kernel.org/RCU/checklist.html

thanks,

 - Joel

Re: [PATCH 2/2] sched: Move task_mm_cid_work to RCU callback

[Gabriele Monaco]

On Tue, 2024-12-03 at 10:00 -0500, Joel Fernandes wrote:
> 
> Also there is no guarantee that RCU callback will run within a thread
> context (example, some configurations run it in softirq). Further,
> call_rcu() usage as shown in this patch can also delay callback runs
> by seconds (with RCU_LAZY enabled).
> 
> See also #5 in checklist: https://docs.kernel.org/RCU/checklist.html
> 

Thanks for the tips, I get the RCU callbacks are clearly not the right
place where to put this function.
I'm working on a V2 where the same tick is scheduling a new work_struct
in the mm_struct instead, let's see where that brings.

Thanks all for the constructive comments.
Gabriele