Em Wed, Dec 16, 2020 at 06:05:56PM +0900, Namhyung Kim escreveu:
> Currently it parses the /proc file everytime it opens a file in the
> cgroupfs. Save the last result to avoid it (assuming it won't be
> changed between the accesses).
Which is the most likely case, but can't we use something like inotify
to detect that and bail out or warn the user?
- Arnaldo
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/lib/api/fs/cgroup.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/tools/lib/api/fs/cgroup.c b/tools/lib/api/fs/cgroup.c
> index 262a4229e293..1573dae4259d 100644
> --- a/tools/lib/api/fs/cgroup.c
> +++ b/tools/lib/api/fs/cgroup.c
> @@ -8,6 +8,14 @@
> #include <string.h>
> #include "fs.h"
>
> +struct cgroupfs_cache_entry {
> + char subsys[32];
> + char mountpoint[PATH_MAX];
> +};
> +
> +/* just cache last used one */
> +static struct cgroupfs_cache_entry cached;
> +
> int cgroupfs_find_mountpoint(char *buf, size_t maxlen, const char *subsys)
> {
> FILE *fp;
> @@ -16,6 +24,14 @@ int cgroupfs_find_mountpoint(char *buf, size_t maxlen, const char *subsys)
> char *p, *path;
> char mountpoint[PATH_MAX];
>
> + if (!strcmp(cached.subsys, subsys)) {
> + if (strlen(cached.mountpoint) < maxlen) {
> + strcpy(buf, cached.mountpoint);
> + return 0;
> + }
> + return -1;
> + }
> +
> fp = fopen("/proc/mounts", "r");
> if (!fp)
> return -1;
> @@ -75,6 +91,9 @@ int cgroupfs_find_mountpoint(char *buf, size_t maxlen, const char *subsys)
> free(line);
> fclose(fp);
>
> + strncpy(cached.subsys, subsys, sizeof(cached.subsys) - 1);
> + strcpy(cached.mountpoint, mountpoint);
> +
> if (mountpoint[0] && strlen(mountpoint) < maxlen) {
> strcpy(buf, mountpoint);
> return 0;
> --
> 2.29.2.684.gfbc64c5ab5-goog
>
--
- Arnaldo
Hi Arnaldo,
On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> Em Wed, Dec 16, 2020 at 06:05:56PM +0900, Namhyung Kim escreveu:
> > Currently it parses the /proc file everytime it opens a file in the
> > cgroupfs. Save the last result to avoid it (assuming it won't be
> > changed between the accesses).
>
> Which is the most likely case, but can't we use something like inotify
> to detect that and bail out or warn the user?
Hmm.. looks doable. Will check.
Thanks,
Namhyung
On Wed, Jan 6, 2021 at 10:33 AM Namhyung Kim <[email protected]> wrote:
>
> Hi Arnaldo,
>
> On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
> <[email protected]> wrote:
> >
> > Em Wed, Dec 16, 2020 at 06:05:56PM +0900, Namhyung Kim escreveu:
> > > Currently it parses the /proc file everytime it opens a file in the
> > > cgroupfs. Save the last result to avoid it (assuming it won't be
> > > changed between the accesses).
> >
> > Which is the most likely case, but can't we use something like inotify
> > to detect that and bail out or warn the user?
>
> Hmm.. looks doable. Will check.
So I've played with inotify a little bit, and it seems it needs to monitor
changes on the file or the directory. I didn't get any notification from
the /proc/mounts file even if I did some mount/umount.
Instead, I could get IN_UNMOUNT when the cgroup filesystem was
unmounted. But for the monitoring, we need to do one of a) select-like
syscall to wait for the events, b) signal-driven IO notification or c) read
the inotify file with non-block mode everytime.
In a library code, I don't think we can do a) or b) since it can affect
user program behaviors. Then we should go with c) but I think
it's opposite to the purpose of this patch. :)
As you said, I think mostly we don't care as the accesses will happen
in a short period of time. But if you really care, maybe for the upcoming
perf daemon changes, I think we can add an API to invalidate the cache
or internal time-based invalidation logic (like remove it after 10 sec.).
Thoughts?
Thanks,
Namhyung
Hi Arnaldo,
Can you share your thoughts on this?
On Fri, Jan 8, 2021 at 2:51 PM Namhyung Kim <[email protected]> wrote:
>
> On Wed, Jan 6, 2021 at 10:33 AM Namhyung Kim <[email protected]> wrote:
> >
> > Hi Arnaldo,
> >
> > On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
> > <[email protected]> wrote:
> > >
> > > Em Wed, Dec 16, 2020 at 06:05:56PM +0900, Namhyung Kim escreveu:
> > > > Currently it parses the /proc file everytime it opens a file in the
> > > > cgroupfs. Save the last result to avoid it (assuming it won't be
> > > > changed between the accesses).
> > >
> > > Which is the most likely case, but can't we use something like inotify
> > > to detect that and bail out or warn the user?
> >
> > Hmm.. looks doable. Will check.
>
> So I've played with inotify a little bit, and it seems it needs to monitor
> changes on the file or the directory. I didn't get any notification from
> the /proc/mounts file even if I did some mount/umount.
>
> Instead, I could get IN_UNMOUNT when the cgroup filesystem was
> unmounted. But for the monitoring, we need to do one of a) select-like
> syscall to wait for the events, b) signal-driven IO notification or c) read
> the inotify file with non-block mode everytime.
>
> In a library code, I don't think we can do a) or b) since it can affect
> user program behaviors. Then we should go with c) but I think
> it's opposite to the purpose of this patch. :)
>
> As you said, I think mostly we don't care as the accesses will happen
> in a short period of time. But if you really care, maybe for the upcoming
> perf daemon changes, I think we can add an API to invalidate the cache
> or internal time-based invalidation logic (like remove it after 10 sec.).
>
> Thoughts?
>
> Thanks,
> Namhyung
Em Fri, Jan 08, 2021 at 02:51:44PM +0900, Namhyung Kim escreveu:
> On Wed, Jan 6, 2021 at 10:33 AM Namhyung Kim <[email protected]> wrote:
> >
> > Hi Arnaldo,
> >
> > On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
> > <[email protected]> wrote:
> > >
> > > Em Wed, Dec 16, 2020 at 06:05:56PM +0900, Namhyung Kim escreveu:
> > > > Currently it parses the /proc file everytime it opens a file in the
> > > > cgroupfs. Save the last result to avoid it (assuming it won't be
> > > > changed between the accesses).
> > >
> > > Which is the most likely case, but can't we use something like inotify
> > > to detect that and bail out or warn the user?
> >
> > Hmm.. looks doable. Will check.
>
> So I've played with inotify a little bit, and it seems it needs to monitor
> changes on the file or the directory. I didn't get any notification from
> the /proc/mounts file even if I did some mount/umount.
>
> Instead, I could get IN_UNMOUNT when the cgroup filesystem was
> unmounted. But for the monitoring, we need to do one of a) select-like
> syscall to wait for the events, b) signal-driven IO notification or c) read
> the inotify file with non-block mode everytime.
>
> In a library code, I don't think we can do a) or b) since it can affect
> user program behaviors. Then we should go with c) but I think
> it's opposite to the purpose of this patch. :)
>
> As you said, I think mostly we don't care as the accesses will happen
> in a short period of time. But if you really care, maybe for the upcoming
> perf daemon changes, I think we can add an API to invalidate the cache
> or internal time-based invalidation logic (like remove it after 10 sec.).
Ok, we can have something in 'perf daemon' to periodically invalidate
this, maybe do a poor man inotify and when asking for the cgroup
mountpoint, check some characteristic of that file that changes when it
is modified, or plain use a timestamp and have some threshold.
- Arnaldo
> Thoughts?
>
> Thanks,
> Namhyung
--
- Arnaldo
Hi Arnaldo,
On Wed, Feb 17, 2021 at 9:58 PM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> Em Fri, Jan 08, 2021 at 02:51:44PM +0900, Namhyung Kim escreveu:
> > On Wed, Jan 6, 2021 at 10:33 AM Namhyung Kim <[email protected]> wrote:
> > >
> > > Hi Arnaldo,
> > >
> > > On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
> > > <[email protected]> wrote:
> > > >
> > > > Em Wed, Dec 16, 2020 at 06:05:56PM +0900, Namhyung Kim escreveu:
> > > > > Currently it parses the /proc file everytime it opens a file in the
> > > > > cgroupfs. Save the last result to avoid it (assuming it won't be
> > > > > changed between the accesses).
> > > >
> > > > Which is the most likely case, but can't we use something like inotify
> > > > to detect that and bail out or warn the user?
> > >
> > > Hmm.. looks doable. Will check.
> >
> > So I've played with inotify a little bit, and it seems it needs to monitor
> > changes on the file or the directory. I didn't get any notification from
> > the /proc/mounts file even if I did some mount/umount.
> >
> > Instead, I could get IN_UNMOUNT when the cgroup filesystem was
> > unmounted. But for the monitoring, we need to do one of a) select-like
> > syscall to wait for the events, b) signal-driven IO notification or c) read
> > the inotify file with non-block mode everytime.
> >
> > In a library code, I don't think we can do a) or b) since it can affect
> > user program behaviors. Then we should go with c) but I think
> > it's opposite to the purpose of this patch. :)
> >
> > As you said, I think mostly we don't care as the accesses will happen
> > in a short period of time. But if you really care, maybe for the upcoming
> > perf daemon changes, I think we can add an API to invalidate the cache
> > or internal time-based invalidation logic (like remove it after 10 sec.).
>
> Ok, we can have something in 'perf daemon' to periodically invalidate
> this, maybe do a poor man inotify and when asking for the cgroup
> mountpoint, check some characteristic of that file that changes when it
> is modified, or plain use a timestamp and have some threshold.
I thought about this again.
We don't directly access the cgroups in the perf daemon.
It just creates new record processes so they'll see a new
mountpoint whenever they started since this cache is
shared within the process only.
That means we don't need to care about the invalidate in the
daemon but each perf record and perf stat should do it when
they are required to do the work repeatedly.
But looking at the code, the cgroup is set during event parsing
(-G option) or early in the command (--for-each-cgroup option).
So cgroup info would not be changed even if the command
runs repeatedly.
So I think you can take the patch as is.
Thanks,
Namhyung
Em Fri, Feb 19, 2021 at 07:05:59PM +0900, Namhyung Kim escreveu:
> On Wed, Feb 17, 2021 at 9:58 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
> > Em Fri, Jan 08, 2021 at 02:51:44PM +0900, Namhyung Kim escreveu:
> > > On Wed, Jan 6, 2021 at 10:33 AM Namhyung Kim <[email protected]> wrote:
> > > As you said, I think mostly we don't care as the accesses will happen
> > > in a short period of time. But if you really care, maybe for the upcoming
> > > perf daemon changes, I think we can add an API to invalidate the cache
> > > or internal time-based invalidation logic (like remove it after 10 sec.).
> > Ok, we can have something in 'perf daemon' to periodically invalidate
> > this, maybe do a poor man inotify and when asking for the cgroup
> > mountpoint, check some characteristic of that file that changes when it
> > is modified, or plain use a timestamp and have some threshold.
> I thought about this again.
> We don't directly access the cgroups in the perf daemon. It just
> creates new record processes so they'll see a new mountpoint whenever
> they started since this cache is shared within the process only.
> That means we don't need to care about the invalidate in the daemon
> but each perf record and perf stat should do it when they are required
> to do the work repeatedly.
> But looking at the code, the cgroup is set during event parsing (-G
> option) or early in the command (--for-each-cgroup option). So cgroup
> info would not be changed even if the command runs repeatedly.
> So I think you can take the patch as is.
Its in perf/core branch on its way to Linus soon :-)
Thanks for checking it.
- Arnaldo