[PATCH v3 0/2] sched/numa: Skip VMA scanning on memory pinned

[PATCH v3 0/2] sched/numa: Skip VMA scanning on memory pinned

[Libo Chen]

v1->v2:
1. add perf improvment numbers in commit log. Yet to find perf diff on
will-it-scale, so not included here. Plan to run more workloads.
2. add tracepoint.
3. To peterz's comment, this will make it impossible to attract tasks to
those memory just like other VMA skippings. This is the current
implementation, I think we can improve that in the future, but at the
moment it's probabaly better to keep it consistent.

v2->v3:
1. add enable_cpuset() based on Mel's suggestion but again I think it's
redundant
2. print out nodemask with %*p.. format in the tracepoint

Libo Chen (2):
  sched/numa: Skip VMA scanning on memory pinned to one NUMA node via
    cpuset.mems
  sched/numa: Add tracepoint that tracks the skipping of numa balancing
    due to cpuset memory pinning

 include/trace/events/sched.h | 30 ++++++++++++++++++++++++++++++
 kernel/sched/fair.c          |  9 +++++++++
 2 files changed, 39 insertions(+)

-- 
2.43.5

[PATCH v3 1/2] sched/numa: Skip VMA scanning on memory pinned to one NUMA node via cpuset.mems

[Libo Chen]

When the memory of the current task is pinned to one NUMA node by cgroup,
there is no point in continuing the rest of VMA scanning and hinting page
faults as they will just be overhead. With this change, there will be no
more unnecessary PTE updates or page faults in this scenario.

We have seen up to a 6x improvement on a typical java workload running on
VMs with memory and CPU pinned to one NUMA node via cpuset in a two-socket
AARCH64 system. With the same pinning, on a 18-cores-per-socket Intel
platform, we have seen 20% improvment in a microbench that creates a
30-vCPU selftest KVM guest with 4GB memory, where each vCPU reads 4KB
pages in a fixed number of loops.

Signed-off-by: Libo Chen <libo.chen@oracle.com>
---
 kernel/sched/fair.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index e43993a4e5807..c9903b1b39487 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3329,6 +3329,13 @@ static void task_numa_work(struct callback_head *work)
 	if (p->flags & PF_EXITING)
 		return;
 
+	/*
+	 * Memory is pinned to only one NUMA node via cpuset.mems, naturally
+	 * no page can be migrated.
+	 */
+	if (cpusets_enabled() && nodes_weight(cpuset_current_mems_allowed) == 1)
+		return;
+
 	if (!mm->numa_next_scan) {
 		mm->numa_next_scan = now +
 			msecs_to_jiffies(sysctl_numa_balancing_scan_delay);
-- 
2.43.5

[PATCH v3 2/2] sched/numa: Add tracepoint that tracks the skipping of numa balancing due to cpuset memory pinning

[Libo Chen]

Unlike sched_skip_vma_numa tracepoint which tracks skipped VMAs, this
tracks the task subjected to cpuset.mems pinning and prints out its
allowed memory node mask.

Signed-off-by: Libo Chen <libo.chen@oracle.com>
---
 include/trace/events/sched.h | 30 ++++++++++++++++++++++++++++++
 kernel/sched/fair.c          |  4 +++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 8994e97d86c13..25ee542fa0063 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -745,6 +745,36 @@ TRACE_EVENT(sched_skip_vma_numa,
 		  __entry->vm_end,
 		  __print_symbolic(__entry->reason, NUMAB_SKIP_REASON))
 );
+
+TRACE_EVENT(sched_skip_cpuset_numa,
+
+	TP_PROTO(struct task_struct *tsk, nodemask_t *mem_allowed_ptr),
+
+	TP_ARGS(tsk, mem_allowed_ptr),
+
+	TP_STRUCT__entry(
+		__array( char,		comm,		TASK_COMM_LEN	)
+		__field( pid_t,		pid				)
+		__field( pid_t,		tgid				)
+		__field( pid_t,		ngid				)
+		__field( nodemask_t *,	mem_allowed_ptr			)
+	),
+
+	TP_fast_assign(
+		memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
+		__entry->pid		 = task_pid_nr(tsk);
+		__entry->tgid		 = task_tgid_nr(tsk);
+		__entry->ngid		 = task_numa_group_id(tsk);
+		__entry->mem_allowed_ptr = mem_allowed_ptr;
+	),
+
+	TP_printk("comm=%s pid=%d tgid=%d ngid=%d mem_nodes_allowed=%*pbl",
+		  __entry->comm,
+		  __entry->pid,
+		  __entry->tgid,
+		  __entry->ngid,
+		  nodemask_pr_args(__entry->mem_allowed_ptr))
+);
 #endif /* CONFIG_NUMA_BALANCING */
 
 /*
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index c9903b1b39487..cc892961ce157 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3333,8 +3333,10 @@ static void task_numa_work(struct callback_head *work)
 	 * Memory is pinned to only one NUMA node via cpuset.mems, naturally
 	 * no page can be migrated.
 	 */
-	if (cpusets_enabled() && nodes_weight(cpuset_current_mems_allowed) == 1)
+	if (cpusets_enabled() && nodes_weight(cpuset_current_mems_allowed) == 1) {
+		trace_sched_skip_cpuset_numa(current, &cpuset_current_mems_allowed);
 		return;
+	}
 
 	if (!mm->numa_next_scan) {
 		mm->numa_next_scan = now +
-- 
2.43.5

Re: [PATCH v3 0/2] sched/numa: Skip VMA scanning on memory pinned

[Andrew Morton]

On Thu, 17 Apr 2025 12:15:41 -0700 Libo Chen <libo.chen@oracle.com> wrote:

> v1->v2:
> 1. add perf improvment numbers in commit log. Yet to find perf diff on
> will-it-scale, so not included here. Plan to run more workloads.
> 2. add tracepoint.
> 3. To peterz's comment, this will make it impossible to attract tasks to
> those memory just like other VMA skippings. This is the current
> implementation, I think we can improve that in the future, but at the
> moment it's probabaly better to keep it consistent.
> 
> v2->v3:
> 1. add enable_cpuset() based on Mel's suggestion but again I think it's
> redundant
> 2. print out nodemask with %*p.. format in the tracepoint

I do agree with Mel - bitmap_weight() is somewhat expensive and
cpusets_enabled() is super fast.  So the benefit to
cpusets_enabled()=false kernels will exceed to cost to
cpusets_enabled()=true kernels.

This isn't traditionally mm.git material, but it's close.  I'll grab
the patchset for some testing.  and shall drop it again if it turns up
via another tree.

Re: [PATCH v3 0/2] sched/numa: Skip VMA scanning on memory pinned

[Libo Chen]

On 4/17/25 13:12, Andrew Morton wrote:
> On Thu, 17 Apr 2025 12:15:41 -0700 Libo Chen <libo.chen@oracle.com> wrote:
> 
>> v1->v2:
>> 1. add perf improvment numbers in commit log. Yet to find perf diff on
>> will-it-scale, so not included here. Plan to run more workloads.
>> 2. add tracepoint.
>> 3. To peterz's comment, this will make it impossible to attract tasks to
>> those memory just like other VMA skippings. This is the current
>> implementation, I think we can improve that in the future, but at the
>> moment it's probabaly better to keep it consistent.
>>
>> v2->v3:
>> 1. add enable_cpuset() based on Mel's suggestion but again I think it's
>> redundant
>> 2. print out nodemask with %*p.. format in the tracepoint
> 
> I do agree with Mel - bitmap_weight() is somewhat expensive and
> cpusets_enabled() is super fast.  So the benefit to
> cpusets_enabled()=false kernels will exceed to cost to
> cpusets_enabled()=true kernels.
> 
Ah yes, that's right. Thanks for grabbing it~

Libo

> This isn't traditionally mm.git material, but it's close.  I'll grab
> the patchset for some testing.  and shall drop it again if it turns up
> via another tree.
> 

Re: [PATCH v3 1/2] sched/numa: Skip VMA scanning on memory pinned to one NUMA node via cpuset.mems

[Chen, Yu C]

Hi Libo,

On 4/18/2025 3:15 AM, Libo Chen wrote:
> When the memory of the current task is pinned to one NUMA node by cgroup,
> there is no point in continuing the rest of VMA scanning and hinting page
> faults as they will just be overhead. With this change, there will be no
> more unnecessary PTE updates or page faults in this scenario.
> 
> We have seen up to a 6x improvement on a typical java workload running on
> VMs with memory and CPU pinned to one NUMA node via cpuset in a two-socket
> AARCH64 system. With the same pinning, on a 18-cores-per-socket Intel
> platform, we have seen 20% improvment in a microbench that creates a
> 30-vCPU selftest KVM guest with 4GB memory, where each vCPU reads 4KB
> pages in a fixed number of loops.
> 
> Signed-off-by: Libo Chen <libo.chen@oracle.com>

I think this is a promising change that we can perform fine-grain NUMA
balance control on a per-cgroup basis rather than system-wide NUMA
balance for every task, which is costly.

> ---
>   kernel/sched/fair.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index e43993a4e5807..c9903b1b39487 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3329,6 +3329,13 @@ static void task_numa_work(struct callback_head *work)
>   	if (p->flags & PF_EXITING)
>   		return;
>   
> +	/*
> +	 * Memory is pinned to only one NUMA node via cpuset.mems, naturally
> +	 * no page can be migrated.
> +	 */
> +	if (cpusets_enabled() && nodes_weight(cpuset_current_mems_allowed) == 1)
> +		return;
> +

I found that you had a proposal in V1 to address Peter's concern[1]:
Allow the task to be migrated to its preferred Node, even if the task's
memory policy is restricted to 1 Node. In your previous proposal, only 
if the task's cpumask is bound to the same Node as its memory policy 
node, the NUMA balance scanning is skipped, because a cgroup usually 
binds its tasks and memory allocation policy to the same node. Not sure 
if that could be turned into:

If the task's memory policy node's CPU mask is a subset of the task's 
cpumask, the NUMA balance scan is allowed.

For example,
Suppose p's memory is only allocated on node0, which contains CPU2, CPU3.
1. If p's CPU affinity is CPU0, CPU1, there is no need to do NUMA 
balancing scanning, because CPU0,1 are not in p's legitimate cpumask.
2. If p's CPU affinity is CPU3, there is no need to do NUMA balancing 
scanning. p is already on its preferred node.
3. But if p's CPU affinity is CPU2, CPU3, CPU6, the NUMA balancing scan 
should be allowed. Because it is possible to migrate p from CPU6 to 
either CPU2 or CPU3.

What I'm thinking of is something as follows(untested):
if (cpusets_enabled() &&
     nodes_weight(cpuset_current_mems_allowed) == 1 &&
     !cpumask_subset(cpumask_of_node(cpuset_current_mems_allowed),
		    p->cpus_ptr))
	return;


I tested your patch on top of the latest sched/core,
binding task CPU affinity to Node1 and memory allocation node on
Node1:
echo "8-15" > /sys/fs/cgroup/mytest/cpuset.cpus
echo "1" > /sys/fs/cgroup/mytest/cpuset.mems
cgexec -g cpuset:mytest ./run-mmtests.sh --no-monitor --config 
config-numa skip_scan

And it works as expected:
# bpftrace numa_trace.bt

@sched_skip_cpuset_numa: 133


thanks,
Chenyu

[1] 
https://lore.kernel.org/lkml/cde7af54-5481-499e-8a42-0111f555f2b1@oracle.com/


>   	if (!mm->numa_next_scan) {
>   		mm->numa_next_scan = now +
>   			msecs_to_jiffies(sysctl_numa_balancing_scan_delay);

Re: [PATCH v3 1/2] sched/numa: Skip VMA scanning on memory pinned to one NUMA node via cpuset.mems

[Libo Chen]

Hi Yu

On 4/19/25 04:16, Chen, Yu C wrote:
> Hi Libo,
> 
> On 4/18/2025 3:15 AM, Libo Chen wrote:
>> When the memory of the current task is pinned to one NUMA node by cgroup,
>> there is no point in continuing the rest of VMA scanning and hinting page
>> faults as they will just be overhead. With this change, there will be no
>> more unnecessary PTE updates or page faults in this scenario.
>>
>> We have seen up to a 6x improvement on a typical java workload running on
>> VMs with memory and CPU pinned to one NUMA node via cpuset in a two-socket
>> AARCH64 system. With the same pinning, on a 18-cores-per-socket Intel
>> platform, we have seen 20% improvment in a microbench that creates a
>> 30-vCPU selftest KVM guest with 4GB memory, where each vCPU reads 4KB
>> pages in a fixed number of loops.
>>
>> Signed-off-by: Libo Chen <libo.chen@oracle.com>
> 
> I think this is a promising change that we can perform fine-grain NUMA
> balance control on a per-cgroup basis rather than system-wide NUMA
> balance for every task, which is costly.
> 

Yes indeed, the cost, from we have seen, can be quite astonishing 

>> ---
>>   kernel/sched/fair.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index e43993a4e5807..c9903b1b39487 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -3329,6 +3329,13 @@ static void task_numa_work(struct callback_head *work)
>>       if (p->flags & PF_EXITING)
>>           return;
>>   +    /*
>> +     * Memory is pinned to only one NUMA node via cpuset.mems, naturally
>> +     * no page can be migrated.
>> +     */
>> +    if (cpusets_enabled() && nodes_weight(cpuset_current_mems_allowed) == 1)
>> +        return;
>> +
> 
> I found that you had a proposal in V1 to address Peter's concern[1]:
> Allow the task to be migrated to its preferred Node, even if the task's
> memory policy is restricted to 1 Node. In your previous proposal, only if the task's cpumask is bound to the same Node as its memory policy node, the NUMA balance scanning is skipped, because a cgroup usually binds its tasks and memory allocation policy to the same node. Not sure if that could be turned into:
> 
> If the task's memory policy node's CPU mask is a subset of the task's cpumask, the NUMA balance scan is allowed.
> 

I guess fundamentally is this really worth it? Do the benefits of NUMA task migrations only outweigh the overheads of VMA scanning, PTE updates and page faults etc? I suppose this is workload-dependent, but what about the best-case scenario? I think we probably need more data.  Also if we do that, we also need to do the same for other VMA skipping scenarios.

Thanks,
Libo 

> For example,
> Suppose p's memory is only allocated on node0, which contains CPU2, CPU3.
> 1. If p's CPU affinity is CPU0, CPU1, there is no need to do NUMA balancing scanning, because CPU0,1 are not in p's legitimate cpumask.
> 2. If p's CPU affinity is CPU3, there is no need to do NUMA balancing scanning. p is already on its preferred node.
> 3. But if p's CPU affinity is CPU2, CPU3, CPU6, the NUMA balancing scan should be allowed. Because it is possible to migrate p from CPU6 to either CPU2 or CPU3.
> 
> What I'm thinking of is something as follows(untested):
> if (cpusets_enabled() &&
>     nodes_weight(cpuset_current_mems_allowed) == 1 &&
>     !cpumask_subset(cpumask_of_node(cpuset_current_mems_allowed),
>             p->cpus_ptr))
>     return;
> 
> 
> I tested your patch on top of the latest sched/core,
> binding task CPU affinity to Node1 and memory allocation node on
> Node1:
> echo "8-15" > /sys/fs/cgroup/mytest/cpuset.cpus
> echo "1" > /sys/fs/cgroup/mytest/cpuset.mems
> cgexec -g cpuset:mytest ./run-mmtests.sh --no-monitor --config config-numa skip_scan
> 
> And it works as expected:
> # bpftrace numa_trace.bt
> 
> @sched_skip_cpuset_numa: 133
> 
> 
> thanks,
> Chenyu
> 
> [1] https://urldefense.com/v3/__https://lore.kernel.org/lkml/cde7af54-5481-499e-8a42-0111f555f2b1@oracle.com/__;!!ACWV5N9M2RV99hQ!OvO0A__dCkeSB4eze2TYZYDHWGg0ubi04-u8lW5NCQGRE6vZkCGahdWMzHtpKMMDSt-L1wCkM8ILMIP3YA$
> 
>>       if (!mm->numa_next_scan) {
>>           mm->numa_next_scan = now +
>>               msecs_to_jiffies(sysctl_numa_balancing_scan_delay);

Re: [PATCH v3 1/2] sched/numa: Skip VMA scanning on memory pinned to one NUMA node via cpuset.mems

[Chen, Yu C]

On 4/23/2025 6:20 AM, Libo Chen wrote:
> Hi Yu
> 
> On 4/19/25 04:16, Chen, Yu C wrote:
>> Hi Libo,
>>
>> On 4/18/2025 3:15 AM, Libo Chen wrote:
>>> When the memory of the current task is pinned to one NUMA node by cgroup,
>>> there is no point in continuing the rest of VMA scanning and hinting page
>>> faults as they will just be overhead. With this change, there will be no
>>> more unnecessary PTE updates or page faults in this scenario.
>>>
>>> We have seen up to a 6x improvement on a typical java workload running on
>>> VMs with memory and CPU pinned to one NUMA node via cpuset in a two-socket
>>> AARCH64 system. With the same pinning, on a 18-cores-per-socket Intel
>>> platform, we have seen 20% improvment in a microbench that creates a
>>> 30-vCPU selftest KVM guest with 4GB memory, where each vCPU reads 4KB
>>> pages in a fixed number of loops.
>>>
>>> Signed-off-by: Libo Chen <libo.chen@oracle.com>
>>
>> I think this is a promising change that we can perform fine-grain NUMA
>> balance control on a per-cgroup basis rather than system-wide NUMA
>> balance for every task, which is costly.
>>
> 
> Yes indeed, the cost, from we have seen, can be quite astonishing
> 
>>> ---
>>>    kernel/sched/fair.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index e43993a4e5807..c9903b1b39487 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -3329,6 +3329,13 @@ static void task_numa_work(struct callback_head *work)
>>>        if (p->flags & PF_EXITING)
>>>            return;
>>>    +    /*
>>> +     * Memory is pinned to only one NUMA node via cpuset.mems, naturally
>>> +     * no page can be migrated.
>>> +     */
>>> +    if (cpusets_enabled() && nodes_weight(cpuset_current_mems_allowed) == 1)
>>> +        return;
>>> +
>>
>> I found that you had a proposal in V1 to address Peter's concern[1]:
>> Allow the task to be migrated to its preferred Node, even if the task's
>> memory policy is restricted to 1 Node. In your previous proposal, only if the task's cpumask is bound to the same Node as its memory policy node, the NUMA balance scanning is skipped, because a cgroup usually binds its tasks and memory allocation policy to the same node. Not sure if that could be turned into:
>>
>> If the task's memory policy node's CPU mask is a subset of the task's cpumask, the NUMA balance scan is allowed.
>>
> 
> I guess fundamentally is this really worth it? Do the benefits of NUMA task migrations only outweigh the overheads of VMA scanning, PTE updates and page faults etc? I suppose this is workload-dependent, but what about the best-case scenario? I think we probably need more data.  Also if we do that, we also need to do the same for other VMA skipping scenarios.
> 

Overall that can be a future work and I agree for now this patch is 
simple enough and feel free to add:

Tested-by: Chen Yu <yu.c.chen@intel.com>

thanks,
Chenyu

Re: [PATCH v3 2/2] sched/numa: Add tracepoint that tracks the skipping of numa balancing due to cpuset memory pinning

[Steven Rostedt]

On Thu, 17 Apr 2025 12:15:43 -0700
Libo Chen <libo.chen@oracle.com> wrote:

> diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
> index 8994e97d86c13..25ee542fa0063 100644
> --- a/include/trace/events/sched.h
> +++ b/include/trace/events/sched.h
> @@ -745,6 +745,36 @@ TRACE_EVENT(sched_skip_vma_numa,
>  		  __entry->vm_end,
>  		  __print_symbolic(__entry->reason, NUMAB_SKIP_REASON))
>  );
> +
> +TRACE_EVENT(sched_skip_cpuset_numa,
> +
> +	TP_PROTO(struct task_struct *tsk, nodemask_t *mem_allowed_ptr),
> +
> +	TP_ARGS(tsk, mem_allowed_ptr),
> +
> +	TP_STRUCT__entry(
> +		__array( char,		comm,		TASK_COMM_LEN	)
> +		__field( pid_t,		pid				)
> +		__field( pid_t,		tgid				)
> +		__field( pid_t,		ngid				)
> +		__field( nodemask_t *,	mem_allowed_ptr			)
> +	),
> +
> +	TP_fast_assign(
> +		memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
> +		__entry->pid		 = task_pid_nr(tsk);
> +		__entry->tgid		 = task_tgid_nr(tsk);
> +		__entry->ngid		 = task_numa_group_id(tsk);
> +		__entry->mem_allowed_ptr = mem_allowed_ptr;

This is a bug. You can't save random pointers in the TP_fast_assign() and
reference it later in the TP_printk().

The TP_fast_assign() is executed during the normal kernel workflow when the
tracepoint is triggered. The pointer is saved into the ring buffer.

> +	),
> +
> +	TP_printk("comm=%s pid=%d tgid=%d ngid=%d mem_nodes_allowed=%*pbl",
> +		  __entry->comm,
> +		  __entry->pid,
> +		  __entry->tgid,
> +		  __entry->ngid,
> +		  nodemask_pr_args(__entry->mem_allowed_ptr))

The TP_printk() is executed when a user reads the /sys/kernel/tracing/trace
file. Which could be literally months later.

The nodemask_pr_args() will dereference the __entry->mem_allowed_ptr from
what was saved in the ring buffer, which the content it points to could
have been freed days ago.

If that happens, then BOOM! Kernel goes bye-bye!

The trace event verifier is made to find bugs like his. And with the recent
update to handle "%*p" it found this bug. ;-)

-- Steve


> +);
>  #endif /* CONFIG_NUMA_BALANCING */

Re: [PATCH v3 2/2] sched/numa: Add tracepoint that tracks the skipping of numa balancing due to cpuset memory pinning

[Libo Chen]

> On Apr 23, 2025, at 8:34 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Thu, 17 Apr 2025 12:15:43 -0700
> Libo Chen <libo.chen@oracle.com> wrote:
> 
>> diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
>> index 8994e97d86c13..25ee542fa0063 100644
>> --- a/include/trace/events/sched.h
>> +++ b/include/trace/events/sched.h
>> @@ -745,6 +745,36 @@ TRACE_EVENT(sched_skip_vma_numa,
>>  __entry->vm_end,
>>  __print_symbolic(__entry->reason, NUMAB_SKIP_REASON))
>> );
>> +
>> +TRACE_EVENT(sched_skip_cpuset_numa,
>> +
>> + TP_PROTO(struct task_struct *tsk, nodemask_t *mem_allowed_ptr),
>> +
>> + TP_ARGS(tsk, mem_allowed_ptr),
>> +
>> + TP_STRUCT__entry(
>> + __array( char, comm, TASK_COMM_LEN )
>> + __field( pid_t, pid )
>> + __field( pid_t, tgid )
>> + __field( pid_t, ngid )
>> + __field( nodemask_t *, mem_allowed_ptr )
>> + ),
>> +
>> + TP_fast_assign(
>> + memcpy(__entry->comm, tsk->comm, TASK_COMM_LEN);
>> + __entry->pid = task_pid_nr(tsk);
>> + __entry->tgid = task_tgid_nr(tsk);
>> + __entry->ngid = task_numa_group_id(tsk);
>> + __entry->mem_allowed_ptr = mem_allowed_ptr;
> 
> This is a bug. You can't save random pointers in the TP_fast_assign() and
> reference it later in the TP_printk().
> 

Admittedly I was a bit nervous about dereferencing this pointer at TP_printk()
time. Will fix it!

Also wondering if we can fail the build in this scenario so it will be easier to
catch this bug at the build time.

Thanks
Libo
 
> The TP_fast_assign() is executed during the normal kernel workflow when the
> tracepoint is triggered. The pointer is saved into the ring buffer.
> 
>> + ),
>> +
>> + TP_printk("comm=%s pid=%d tgid=%d ngid=%d mem_nodes_allowed=%*pbl",
>> +  __entry->comm,
>> +  __entry->pid,
>> +  __entry->tgid,
>> +  __entry->ngid,
>> +  nodemask_pr_args(__entry->mem_allowed_ptr))
> 
> The TP_printk() is executed when a user reads the /sys/kernel/tracing/trace
> file. Which could be literally months later.
> 
> The nodemask_pr_args() will dereference the __entry->mem_allowed_ptr from
> what was saved in the ring buffer, which the content it points to could
> have been freed days ago.
> 
> If that happens, then BOOM! Kernel goes bye-bye!
> 
> The trace event verifier is made to find bugs like his. And with the recent
> update to handle "%*p" it found this bug. ;-)
> 
> -- Steve
> 
> 
>> +);
>> #endif /* CONFIG_NUMA_BALANCING */

Re: [PATCH v3 2/2] sched/numa: Add tracepoint that tracks the skipping of numa balancing due to cpuset memory pinning

[Steven Rostedt]

On Wed, 23 Apr 2025 16:05:44 +0000
Libo Chen <libo.chen@oracle.com> wrote:

> Also wondering if we can fail the build in this scenario so it will be easier to
> catch this bug at the build time.

 return -EPONYS ;-)

I wish. It's hard enough to catch this at runtime.

-- Steve

Re: [PATCH v3 2/2] sched/numa: Add tracepoint that tracks the skipping of numa balancing due to cpuset memory pinning

[Libo Chen]

> On Apr 23, 2025, at 9:12 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Wed, 23 Apr 2025 16:05:44 +0000
> Libo Chen <libo.chen@oracle.com> wrote:
> 
>> Also wondering if we can fail the build in this scenario so it will be easier to
>> catch this bug at the build time.
> 
> return -EPONYS ;-)
> 
> I wish. It's hard enough to catch this at runtime.
> 

Correct me if I'm wrong but can you disallow any passed-in pointers to be
dereferenced when TP_printk() is executed? This is something you can check
at the build time, right?

Libo

> -- Steve

Re: [PATCH v3 2/2] sched/numa: Add tracepoint that tracks the skipping of numa balancing due to cpuset memory pinning

[Steven Rostedt]

On Wed, 23 Apr 2025 16:50:15 +0000
Libo Chen <libo.chen@oracle.com> wrote:

> Correct me if I'm wrong but can you disallow any passed-in pointers to be
> dereferenced when TP_printk() is executed? This is something you can check
> at the build time, right?

You can dereference if the pointer is to the content on the ring buffer.
For instance, you can have:

  "%p*h", &__entry->val

It dereferences to the content stored on the ring buffer.

What we can't have is:

  "%p*h", __entry->val

-- Steve

Re: [PATCH v3 2/2] sched/numa: Add tracepoint that tracks the skipping of numa balancing due to cpuset memory pinning

[Steven Rostedt]

On Wed, 23 Apr 2025 12:56:59 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> For instance, you can have:
> 
>   "%p*h", &__entry->val

That should have been:

   "%p*h", __entry->size, &__entry->val


-- Steve

Re: [PATCH v3 2/2] sched/numa: Add tracepoint that tracks the skipping of numa balancing due to cpuset memory pinning

[Libo Chen]

> On Apr 23, 2025, at 9:56 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Wed, 23 Apr 2025 16:50:15 +0000
> Libo Chen <libo.chen@oracle.com> wrote:
> 
>> Correct me if I'm wrong but can you disallow any passed-in pointers to be
>> dereferenced when TP_printk() is executed? This is something you can check
>> at the build time, right?
> 
> You can dereference if the pointer is to the content on the ring buffer.
> For instance, you can have:
> 
>  "%p*h", &__entry->val
> 
> It dereferences to the content stored on the ring buffer.
> 
> What we can't have is:
> 
>  "%p*h", __entry->val

Right, I was thinking something stricter such as disallowing point-type
field in TP_STRUCT__entry {} to avoid direct assignment to point-type
field so there will be no chance to have unsafe dereference but then I
realize C doesn’t have built-in mechanism to detect various types of
pointers at the compile time, maybe rust can do that. Anyway I give up.

Thanks,
Libo

> 
> -- Steve