Re: [PATCH 2/2][v2] x86/resctrl: Add task resctrl information display

[Ryan Chen]

Hi Boris,
On Fri, Nov 15, 2019 at 5:25 PM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> On Fri, Nov 15, 2019 at 01:25:06PM +0800, Chen Yu wrote:
> > Monitoring tools that want to find out which resctrl CTRL
> > and MONITOR groups a task belongs to must currently read
> > the "tasks" file in every group until they locate the process
> > ID.
> >
> > Add an additional file /proc/{pid}/resctrl to provide this
> > information.
> >
> > For example:
> >  cat /proc/1193/resctrl
> > CTRL_MON:/ctrl_grp0
> > MON:/ctrl_grp0/mon_groups/mon_grp0
> >
> > If the resctrl filesystem has not been mounted,
> > reading /proc/{pid}/resctrl returns an error:
> > cat: /proc/1193/resctrl: No such device
>
> Eww, this doesn't sound very user-friendly. How is the user supposed to
> know that the resctrl fs needs to be mounted for this to work?
>
> Why does the resctrl fs need to be mounted at all to show this?
>
> I'm guessing if it is not mounted, you have no groups so you don't have
> to return an error - you simply return "". Right?
>
Right, we can return 'blank' to user and let the user to parse the information.
And there is a similar behavior in cgroup that, for kernel thread that
does not belong
to any cgroup, /proc/{pid}/cgroup just show 'blank' without returning an error.

> > Tested-by: Jinshi Chen <jinshi.chen@xxxxxxxxx>
> > Reviewed-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> > Reviewed-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
>
> When you send a new version which has non-trivial changes, you should
> drop those tags because they don't apply anymore. Unless those people
> have managed to review and test the new version ...
>
Got it, thanks for telling me this.
> Looking at CONFIG_PROC_PID_ARCH_STATUS for an example of proc/ calling
> arch-specific functions, I guess you need to do:
>
> select CPU_RESCTRL if PROC_FS
>
Yes, only when PROC_FS is set, /proc/{pid}/resctrl
can be displayed. However, CPU_RESCTRL might not
depend on proc fs, it is possible that the CPU_RESCTRL
is enabled but without PROC_FS set. If I understand correctly,
 CPU_RESCTRL is the 'root' config for X86_CPU_RESCTRL,
after reading this thread:
https://lists.gt.net/linux/kernel/3211659
 If this is the case, shall we add the new file at kernel/resctrl/resctrl.c?
And the generic proc_resctrl_show() could be put into this file. In the future
the generic code for resctrl could be added/moved to kernel/resctrl/resctrl.c

Thanks,
Chenyu

> Thx.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette