Re: [PATCH 3/3] tools/lib/fs: Cache cgroupfs mount point

[Namhyung Kim]

Hi Arnaldo,

On Wed, Feb 17, 2021 at 9:58 PM Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> 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 <namhyung@xxxxxxxxxx> wrote:
> > >
> > > Hi Arnaldo,
> > >
> > > On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
> > > <acme@xxxxxxxxxx> 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