Re: [RFC PATCH 2/3] x86/resctrl: Move the task's threads to the group automatically

[Moger, Babu]

Hi Fenghua,

On 1/3/23 23:55, Yu, Fenghua wrote:
> Hi, Babu,
>
>> Some micro benchmarks run multiple threads when started. Monitoring (or
>> controlling) the benchmark using the task id is bit tricky. Users need to track all
>> the threads and assign them individually to monitor or control. For example:
>>   $stream_lowOverhead -codeAlg 13 -nRep 100000 -cores 0 1 2 3 -memMB 32
>>   -alignKB 8192 -aPadB 0 -bPadB 0 -cPadB 0 -testMask 1
>>
>>   $pidof stream_lowOverhead
>>   6793
>>
>> This benchmark actually runs multiple threads underneath on the cores listed
>> above. It can be seen with the command:
>>   $ps -T -p 6793
>>    PID   SPID TTY          TIME CMD
>>   6793   6793 pts/2    00:00:00 stream_lowOverh
>>   6793   6802 pts/2    00:01:25 stream_lowOverh
>>   6793   6803 pts/2    00:01:25 stream_lowOverh
>>   6793   6804 pts/2    00:01:25 stream_lowOverh
>>   6793   6805 pts/2    00:01:25 stream_lowOverh
>>
>> Users need to assign these threads individually to the resctrl group for
>> monitoring or controlling.
>>
>>   $echo 6793 > /sys/fs/restrl/clos1/tasks
>>   $echo 6802 > /sys/fs/restrl/clos1/tasks
>>   $echo 6803 > /sys/fs/restrl/clos1/tasks
>>   $echo 6804 > /sys/fs/restrl/clos1/tasks
>>   $echo 6805 > /sys/fs/restrl/clos1/tasks
>>
>> That is not easy when dealing with numerous threads.
>>
>> Detect the task's threads automatically and assign them to the resctrl group
>> when parent task is assigned. For example:
> But user may choose not to move threads along with the parent.
> You will need to have an option to opt in.
Yes. I agree.
>
>>   $echo 6793 > /sys/fs/restrl/clos1/tasks
>>
>> All the threads will be assigned to the group automatically.
>>   $cat /sys/fs/restrl/clos1/tasks
>>   6793
>>   6793
>>   6802
>>   6803
>>   6804
>>   6805
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |    9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 344607853f4c..0d71ed22cfa9 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -685,6 +685,7 @@ static int rdtgroup_move_task(pid_t pid, struct rdtgroup
>> *rdtgrp,  static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>>  				    char *buf, size_t nbytes, loff_t off)  {
>> +	struct task_struct *task, *thread;
>>  	struct rdtgroup *rdtgrp;
>>  	char *pid_str;
>>  	int ret = 0;
>> @@ -723,7 +724,13 @@ static ssize_t rdtgroup_tasks_write(struct
>> kernfs_open_file *of,
>>  		goto exit;
>>  	}
>>
>> -	ret = rdtgroup_move_task(pid, rdtgrp, of);
>> +	task = find_task_by_vpid(pid);
>> +	thread = task;
>> +	do {
>> +		ret = rdtgroup_move_task(thread->pid, rdtgrp, of);
>> +		if (ret)
>> +			goto exit;
> If failure happens in the middle of threads, will you reverse the previous
> moved threads (or even the task) or will you report this failure and move
> to the next thread? Seems to me you need to either move all threads or
> no thread moved at all.

Yes. We should handle the failures properly.  Reversing all previous
movements requires quite a bit of book keeping.

As a work-around Reinette's comment "echo $$ >
/sys/fs/resctrl/clos1/tasks" seems to move all the threads.

I will have to think about this more closely.

Thanks

Babu


>
>> +	} while_each_thread(task, thread);
>>
>>  	goto next;
>>
>>
> Thanks.
>
> -Fenghua

-- 
Thanks
Babu Moger