Re: [PATCH v8 1/8] x86/resctrl: Add multiple tasks to the resctrl group at once

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Fenghua,

On 9/6/2023 3:42 PM, Fenghua Yu wrote:
Hi, Babu,

On 9/6/23 07:56, Moger, Babu wrote:
@@ -715,7 +714,27 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
          goto unlock;
      }
  -    ret = rdtgroup_move_task(pid, rdtgrp, of);
+    while (buf && buf[0] != '\0' && buf[0] != '\n') {
+        pid_str = strim(strsep(&buf, ","));
+
+        if (kstrtoint(pid_str, 0, &pid)) {
+            rdt_last_cmd_puts("Task list parsing error\n");

It would be better to show the failed pid string in the failure report:
+            rdt_last_cmd_puts("Task list parsing error pid %s\n", pid_str);

So user will know which pid string causes the failure?

It was already discussed. Printing the characters during parsing error may not be much useful.

Could you please let me know where printing "pid_str" is discussed?

My bad.  Should have read your comments more carefully.



My understanding is a similar thing is discussed in v3:
https://lore.kernel.org/all/167778866506.1053859.2329229096484796501.stgit@bmoger-ubuntu/

Then v4 has this code without printing pid_str. In v4, there is a similar discussion of printing pid, but not pid_str.

But I cannot find a discussion of why "pid_str" is not printed.

If kstritoint(pid_str, 0, &pid) fails, without printing pid_str, how can user know which pid string fails? e.g. user tries to move 100 pids and the 51st pid parsing fails. It's hard for user to know the 51st pid fails without showing pid_str in the error info and then it's hard for the user to decide to re-do moving or aborting moving etc.

That is correct.  Will add following print statement o print the pid_str.

rdt_last_cmd_printf("Task list parsing error pid %s\n", pid_str);

Thanks

Babu





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux