Hi Babu,
On 3/20/2023 8:07 AM, Moger, Babu wrote:
> On 3/16/23 15:33, Reinette Chatre wrote:
>> On 3/16/2023 12:51 PM, Moger, Babu wrote:
>>> On 3/16/23 12:12, Reinette Chatre wrote:
>>>> On 3/16/2023 9:27 AM, Moger, Babu wrote:
>>>>>> -----Original Message-----
>>>>>> From: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>>>>>> Sent: Wednesday, March 15, 2023 1:33 PM
>>>>>> To: Moger, Babu <Babu.Moger@xxxxxxx>; corbet@xxxxxxx;
>>>>>> tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx
>>>>>> Cc: fenghua.yu@xxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx;
>>>>>> hpa@xxxxxxxxx; paulmck@xxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx;
>>>>>> quic_neeraju@xxxxxxxxxxx; rdunlap@xxxxxxxxxxxxx;
>>>>>> damien.lemoal@xxxxxxxxxxxxxxxxxx; songmuchun@xxxxxxxxxxxxx;
>>>>>> peterz@xxxxxxxxxxxxx; jpoimboe@xxxxxxxxxx; pbonzini@xxxxxxxxxx;
>>>>>> chang.seok.bae@xxxxxxxxx; pawan.kumar.gupta@xxxxxxxxxxxxxxx;
>>>>>> jmattson@xxxxxxxxxx; daniel.sneddon@xxxxxxxxxxxxxxx; Das1, Sandipan
>>>>>> <Sandipan.Das@xxxxxxx>; tony.luck@xxxxxxxxx; james.morse@xxxxxxx;
>>>>>> linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>>>>>> bagasdotme@xxxxxxxxx; eranian@xxxxxxxxxx; christophe.leroy@xxxxxxxxxx;
>>>>>> jarkko@xxxxxxxxxx; adrian.hunter@xxxxxxxxx; quic_jiles@xxxxxxxxxxx;
>>>>>> peternewman@xxxxxxxxxx
>>>>>> Subject: Re: [PATCH v3 1/7] x86/resctrl: Add multiple tasks to the resctrl group
>>>>>> at once
>>>>>>
>>>>>> Hi Babu,
>>>>>>
>>>>>> On 3/2/2023 12:24 PM, Babu Moger wrote:
>>>>>>> The resctrl task assignment for MONITOR or CONTROL group needs to be
>>>>>>> done one at a time. For example:
>>>>>>>
>>>>>>> $mount -t resctrl resctrl /sys/fs/resctrl/
>>>>>>> $mkdir /sys/fs/resctrl/clos1
>>>>>>> $echo 123 > /sys/fs/resctrl/clos1/tasks
>>>>>>> $echo 456 > /sys/fs/resctrl/clos1/tasks
>>>>>>> $echo 789 > /sys/fs/resctrl/clos1/tasks
>>>>>>>
>>>>>>> This is not user-friendly when dealing with hundreds of tasks. Also,
>>>>>>> there is a syscall overhead for each command executed from user space.
>>>>>>
>>>>>> To support this change it may also be helpful to add that moving tasks take the
>>>>>> mutex so attempting to move tasks in parallel will not achieve a significant
>>>>>> performance gain.
>>>>>
>>>>> Agree. It may not be significant performance gain. Will remove this line.
>>>>
>>>> It does not sound as though you are actually responding to my comment.
>>>
>>> I am confused. I am already saying there is syscall overhead for each
>>> command if we move the tasks one by one. Now do you want me to add "moving
>>> tasks take the mutex so attempting to move tasks in parallel will not
>>> achieve a significant performance gain".
>>>
>>> It is contradictory, So, I wanted to remove the line about performance.
>>> Did I still miss something?
>>
>> Where is the contradiction?
>>
>> Consider your example:
>> $echo 123 > /sys/fs/resctrl/clos1/tasks
>> $echo 456 > /sys/fs/resctrl/clos1/tasks
>> $echo 789 > /sys/fs/resctrl/clos1/tasks
>>
>> Yes, there is syscall overhead for each of the above lines. My statement was in
>> support of this work by stating that a user aiming to improve performance by
>> attempting the above in parallel would not be able to see achieve significant
>> performance gain since the calls would end up being serialized.
>
> ok. Sure. Will add the text. I may modify little bit.
>>
>> You are providing two motivations (a) "user-friendly when dealing with
>> hundreds of tasks", and (b) syscall overhead. Have you measured the
>> improvement this solution provides?
>
> No. I have not measured the performance improvement.
The changelog makes a claim that the current implementation has overhead
that is removed with this change. There is no data to support this claim.
...
>>>>>>> +
>>>>>>> + buf[nbytes - 1] = '\0';
>>>>>>> +
>>>>>>> rdtgrp = rdtgroup_kn_lock_live(of->kn);
>>>>>>> if (!rdtgrp) {
>>>>>>> rdtgroup_kn_unlock(of->kn);
>>>>>>> return -ENOENT;
>>>>>>> }
>>>>>>> +
>>>>>>> +next:
>>>>>>> + if (!buf || buf[0] == '\0')
>>>>>>> + goto unlock;
>>>>>>> +
>>>>>>> + pid_str = strim(strsep(&buf, ","));
>>>>>>> +
>>>>>>
>>>>>> Could lib/cmdline.c:get_option() be useful?
>>>>>
>>>>> Yes. We could that also. May not be required for the simple case like this.
>>>>
>>>> Please keep an eye out for how much of it you end up duplicating ....
>>>
>>> Using the get_options will require at least two calls(one to get the
>>> length and then read the integers). Also need to allocate the integers
>>> array dynamically. That is lot code if we are going that route.
>>>
>>
>> I did not ask about get_options(), I asked about get_option().
>
> If you insist, will use get_option. But we still have to loop thru all the
> string till get_option returns 0. I can try that.
I just asked whether get_option() could be useful. Could you please point out what
I said that made you think that I insist on this change being made? If it matches
your usage, then know it is available, if it does not, then don't use it.
...
>>> I can say "The failure pid will be logged in
>>> /sys/fs/resctrl/info/last_cmd_status file."
>>
>> That will not be accurate. Not all errors include the pid.
>
> Can you please suggest?
last_cmd_status provides a 512 char buffer to communicate details
to the user. The buffer is cleared before the loop that moves all the
tasks start. If an error is encountered, a detailed message is written
to the buffer. One option may be to append a string to the buffer that
includes the pid? Perhaps something like:
rdt_last_cmd_printf("Error encountered while moving task %d\n", pid);
Please feel free to improve.
Reinette
