Re: [RFC v2 PATCH 1/7] 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 2/16/2023 4:27 PM, Fenghua Yu wrote:
Hi, Babu,

On 2/2/23 13:46, 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.

Maybe add something like "poor performance due to syscall overhead...".
Ok. Sure


Improve the user experience by supporting the multiple task assignment
in one command with the tasks separated by commas. For example:

   $echo 123,456,789 > /sys/fs/resctrl/clos1/tasks

Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
---
  Documentation/x86/resctrl.rst          |    9 +++++++--
  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   24 +++++++++++++++++++++++-
  2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
index 058257dc56c8..58b76fc75cb7 100644
--- a/Documentation/x86/resctrl.rst
+++ b/Documentation/x86/resctrl.rst
@@ -292,13 +292,18 @@ All groups contain the following files:
  "tasks":
      Reading this file shows the list of all tasks that belong to
      this group. Writing a task id to the file will add a task to the
-    group. If the group is a CTRL_MON group the task is removed from
+    group. Multiple tasks can be assigned together in one command by
+    inputting the tasks separated by commas. Tasks will be assigned
+    sequentially in the order it is provided. Failure while assigning
+    the tasks will be aborted immediately and tasks next in the
+    sequence will not be assigned. Users may need to retry them again.

May need to add "tasks before the failure are assigned...".
Sure.

To retry movement, user needs to know which pid fails. So it's better
to add "last_command_status shows the failure pid and user can parse
it to retry assignment starting from the failure pid".
Sure.

+
+    If the group is a CTRL_MON group the task is removed from
      whichever previous CTRL_MON group owned the task and also from
      any MON group that owned the task. If the group is a MON group,
      then the task must already belong to the CTRL_MON parent of this
      group. The task is removed from any previous MON group.
  -
  "cpus":
      Reading this file shows a bitmask of the logical CPUs owned by
      this group. Writing a mask to this file will add and remove
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e2c1599d1b37..13b7c5f3a27c 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -683,16 +683,34 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
                      char *buf, size_t nbytes, loff_t off)
  {
      struct rdtgroup *rdtgrp;
+    char *pid_str;
      int ret = 0;
      pid_t pid;
  -    if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
+    /* Valid input requires a trailing newline */
+    if (nbytes == 0 || buf[nbytes - 1] != '\n')
          return -EINVAL;
+
+    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, ","));
+
+    if (kstrtoint(pid_str, 0, &pid) || pid < 0) {
+        rdt_last_cmd_puts("Invalid pid value\n");

Better to add pid_str in failure info. Then user knows where the failure pid happens and can re-do the movement starting from the failed pid.

ok.


+        ret = -EINVAL;
+        goto unlock;
+    }
+
      rdt_last_cmd_clear();
        if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
@@ -703,6 +721,10 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
      }
        ret = rdtgroup_move_task(pid, rdtgrp, of);
+    if (ret)

May need to report "Failed at %d\n", pid;

Ok. Dont want to overwrite the last command status. I may need to update it with pid. Will check on this.

thanks

Babu


+        goto unlock;
+    else
+        goto next;
    unlock:
      rdtgroup_kn_unlock(of->kn);



Thanks.

-Fenghua



[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