[PATCH 1/3] tools/lib/fs: Prefer cgroup v1 path

[PATCH 1/3] tools/lib/fs: Prefer cgroup v1 path

[Namhyung Kim]

The cgroupfs_find_mountpoint() looks up the /proc/mounts file to find
a directory for the given cgroup subsystem.  It keeps both cgroup v1
and v2 path since there's a possibility of the mixed hierarchly.

But we can simply use v1 path if it's found as it will override the v2
hierarchy.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/api/fs/cgroup.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/tools/lib/api/fs/cgroup.c b/tools/lib/api/fs/cgroup.c
index 889a6eb4aaca..813236d8ca48 100644
--- a/tools/lib/api/fs/cgroup.c
+++ b/tools/lib/api/fs/cgroup.c
@@ -12,7 +12,7 @@ int cgroupfs_find_mountpoint(char *buf, size_t maxlen, const char *subsys)
 {
 	FILE *fp;
 	char mountpoint[PATH_MAX + 1], tokens[PATH_MAX + 1], type[PATH_MAX + 1];
-	char path_v1[PATH_MAX + 1], path_v2[PATH_MAX + 2], *path;
+	char path_v2[PATH_MAX + 1];
 	char *token, *saved_ptr = NULL;
 
 	fp = fopen("/proc/mounts", "r");
@@ -22,45 +22,41 @@ int cgroupfs_find_mountpoint(char *buf, size_t maxlen, const char *subsys)
 	/*
 	 * in order to handle split hierarchy, we need to scan /proc/mounts
 	 * and inspect every cgroupfs mount point to find one that has
-	 * perf_event subsystem
+	 * the given subsystem.  If we found v1, just use it.  If not we can
+	 * use v2 path as a fallback.
 	 */
-	path_v1[0] = '\0';
 	path_v2[0] = '\0';
 
 	while (fscanf(fp, "%*s %"__stringify(PATH_MAX)"s %"__stringify(PATH_MAX)"s %"
 				__stringify(PATH_MAX)"s %*d %*d\n",
 				mountpoint, type, tokens) == 3) {
 
-		if (!path_v1[0] && !strcmp(type, "cgroup")) {
+		if (!strcmp(type, "cgroup")) {
 
 			token = strtok_r(tokens, ",", &saved_ptr);
 
 			while (token != NULL) {
 				if (subsys && !strcmp(token, subsys)) {
-					strcpy(path_v1, mountpoint);
-					break;
+					/* found */
+					fclose(fp);
+
+					if (strlen(mountpoint) < maxlen) {
+						strcpy(buf, mountpoint);
+						return 0;
+					}
+					return -1;
 				}
 				token = strtok_r(NULL, ",", &saved_ptr);
 			}
 		}
 
-		if (!path_v2[0] && !strcmp(type, "cgroup2"))
+		if (!strcmp(type, "cgroup2"))
 			strcpy(path_v2, mountpoint);
-
-		if (path_v1[0] && path_v2[0])
-			break;
 	}
 	fclose(fp);
 
-	if (path_v1[0])
-		path = path_v1;
-	else if (path_v2[0])
-		path = path_v2;
-	else
-		return -1;
-
-	if (strlen(path) < maxlen) {
-		strcpy(buf, path);
+	if (path_v2[0] && strlen(path_v2) < maxlen) {
+		strcpy(buf, path_v2);
 		return 0;
 	}
 	return -1;
-- 
2.29.2.684.gfbc64c5ab5-goog

[PATCH 2/3] tools/lib/fs: Diet cgroupfs_find_mountpoint()

[Namhyung Kim]

Reduce the number of buffers and hopefully make it more efficient. :)

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/api/fs/cgroup.c | 70 +++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 25 deletions(-)

diff --git a/tools/lib/api/fs/cgroup.c b/tools/lib/api/fs/cgroup.c
index 813236d8ca48..262a4229e293 100644
--- a/tools/lib/api/fs/cgroup.c
+++ b/tools/lib/api/fs/cgroup.c
@@ -11,9 +11,10 @@
 int cgroupfs_find_mountpoint(char *buf, size_t maxlen, const char *subsys)
 {
 	FILE *fp;
-	char mountpoint[PATH_MAX + 1], tokens[PATH_MAX + 1], type[PATH_MAX + 1];
-	char path_v2[PATH_MAX + 1];
-	char *token, *saved_ptr = NULL;
+	char *line = NULL;
+	size_t len = 0;
+	char *p, *path;
+	char mountpoint[PATH_MAX];
 
 	fp = fopen("/proc/mounts", "r");
 	if (!fp)
@@ -25,38 +26,57 @@ int cgroupfs_find_mountpoint(char *buf, size_t maxlen, const char *subsys)
 	 * the given subsystem.  If we found v1, just use it.  If not we can
 	 * use v2 path as a fallback.
 	 */
-	path_v2[0] = '\0';
+	mountpoint[0] = '\0';
 
-	while (fscanf(fp, "%*s %"__stringify(PATH_MAX)"s %"__stringify(PATH_MAX)"s %"
-				__stringify(PATH_MAX)"s %*d %*d\n",
-				mountpoint, type, tokens) == 3) {
+	/*
+	 * The /proc/mounts has the follow format:
+	 *
+	 *   <devname> <mount point> <fs type> <options> ...
+	 *
+	 */
+	while (getline(&line, &len, fp) != -1) {
+		/* skip devname */
+		p = strchr(line, ' ');
+		if (p == NULL)
+			continue;
 
-		if (!strcmp(type, "cgroup")) {
+		/* save the mount point */
+		path = ++p;
+		p = strchr(p, ' ');
+		if (p == NULL)
+			continue;
 
-			token = strtok_r(tokens, ",", &saved_ptr);
+		*p++ = '\0';
 
-			while (token != NULL) {
-				if (subsys && !strcmp(token, subsys)) {
-					/* found */
-					fclose(fp);
+		/* check filesystem type */
+		if (strncmp(p, "cgroup", 6))
+			continue;
 
-					if (strlen(mountpoint) < maxlen) {
-						strcpy(buf, mountpoint);
-						return 0;
-					}
-					return -1;
-				}
-				token = strtok_r(NULL, ",", &saved_ptr);
-			}
+		if (p[6] == '2') {
+			/* save cgroup v2 path */
+			strcpy(mountpoint, path);
+			continue;
 		}
 
-		if (!strcmp(type, "cgroup2"))
-			strcpy(path_v2, mountpoint);
+		/* now we have cgroup v1, check the options for subsystem */
+		p += 7;
+
+		p = strstr(p, subsys);
+		if (p == NULL)
+			continue;
+
+		/* sanity check: it should be separated by a space or a comma */
+		if (!strchr(" ,", p[-1]) || !strchr(" ,", p[strlen(subsys)]))
+			continue;
+
+		strcpy(mountpoint, path);
+		break;
 	}
+	free(line);
 	fclose(fp);
 
-	if (path_v2[0] && strlen(path_v2) < maxlen) {
-		strcpy(buf, path_v2);
+	if (mountpoint[0] && strlen(mountpoint) < maxlen) {
+		strcpy(buf, mountpoint);
 		return 0;
 	}
 	return -1;
-- 
2.29.2.684.gfbc64c5ab5-goog

Re: [PATCH 2/3] tools/lib/fs: Diet cgroupfs_find_mountpoint()

[Jiri Olsa]

On Wed, Dec 16, 2020 at 06:05:55PM +0900, Namhyung Kim wrote:

SNIP

> +		*p++ = '\0';
>  
> -			while (token != NULL) {
> -				if (subsys && !strcmp(token, subsys)) {
> -					/* found */
> -					fclose(fp);
> +		/* check filesystem type */
> +		if (strncmp(p, "cgroup", 6))
> +			continue;
>  
> -					if (strlen(mountpoint) < maxlen) {
> -						strcpy(buf, mountpoint);
> -						return 0;
> -					}
> -					return -1;
> -				}
> -				token = strtok_r(NULL, ",", &saved_ptr);
> -			}
> +		if (p[6] == '2') {
> +			/* save cgroup v2 path */
> +			strcpy(mountpoint, path);
> +			continue;
>  		}
>  
> -		if (!strcmp(type, "cgroup2"))
> -			strcpy(path_v2, mountpoint);
> +		/* now we have cgroup v1, check the options for subsystem */
> +		p += 7;
> +
> +		p = strstr(p, subsys);

not sure this is a real problem, but this would mixe up for
cpu/cpuacct/cpuset no? we are using the function for perf_event
subsys only, but it's globaly availble

jirka

> +		if (p == NULL)
> +			continue;
> +
> +		/* sanity check: it should be separated by a space or a comma */
> +		if (!strchr(" ,", p[-1]) || !strchr(" ,", p[strlen(subsys)]))
> +			continue;
> +
> +		strcpy(mountpoint, path);
> +		break;
>  	}
> +	free(line);
>  	fclose(fp);
>  
> -	if (path_v2[0] && strlen(path_v2) < maxlen) {
> -		strcpy(buf, path_v2);
> +	if (mountpoint[0] && strlen(mountpoint) < maxlen) {
> +		strcpy(buf, mountpoint);
>  		return 0;
>  	}
>  	return -1;
> -- 
> 2.29.2.684.gfbc64c5ab5-goog
> 

Re: [PATCH 2/3] tools/lib/fs: Diet cgroupfs_find_mountpoint()

[Namhyung Kim]

Hi Jiri,

On Mon, Dec 28, 2020 at 5:31 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Dec 16, 2020 at 06:05:55PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > +             *p++ = '\0';
> >
> > -                     while (token != NULL) {
> > -                             if (subsys && !strcmp(token, subsys)) {
> > -                                     /* found */
> > -                                     fclose(fp);
> > +             /* check filesystem type */
> > +             if (strncmp(p, "cgroup", 6))
> > +                     continue;
> >
> > -                                     if (strlen(mountpoint) < maxlen) {
> > -                                             strcpy(buf, mountpoint);
> > -                                             return 0;
> > -                                     }
> > -                                     return -1;
> > -                             }
> > -                             token = strtok_r(NULL, ",", &saved_ptr);
> > -                     }
> > +             if (p[6] == '2') {
> > +                     /* save cgroup v2 path */
> > +                     strcpy(mountpoint, path);
> > +                     continue;
> >               }
> >
> > -             if (!strcmp(type, "cgroup2"))
> > -                     strcpy(path_v2, mountpoint);
> > +             /* now we have cgroup v1, check the options for subsystem */
> > +             p += 7;
> > +
> > +             p = strstr(p, subsys);
>
> not sure this is a real problem, but this would mixe up for
> cpu/cpuacct/cpuset no? we are using the function for perf_event
> subsys only, but it's globaly availble

Yeah, that's why I added the sanity check below. :)

>
> > +             if (p == NULL)
> > +                     continue;
> > +
> > +             /* sanity check: it should be separated by a space or a comma */
> > +             if (!strchr(" ,", p[-1]) || !strchr(" ,", p[strlen(subsys)]))
> > +                     continue;

Here.

Thanks,
Namhyung


> > +
> > +             strcpy(mountpoint, path);
> > +             break;
> >       }
> > +     free(line);
> >       fclose(fp);
> >
> > -     if (path_v2[0] && strlen(path_v2) < maxlen) {
> > -             strcpy(buf, path_v2);
> > +     if (mountpoint[0] && strlen(mountpoint) < maxlen) {
> > +             strcpy(buf, mountpoint);
> >               return 0;
> >       }
> >       return -1;
> > --
> > 2.29.2.684.gfbc64c5ab5-goog
> >
>

Re: [PATCH 2/3] tools/lib/fs: Diet cgroupfs_find_mountpoint()

[Jiri Olsa]

On Tue, Dec 29, 2020 at 02:27:36PM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Mon, Dec 28, 2020 at 5:31 PM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Wed, Dec 16, 2020 at 06:05:55PM +0900, Namhyung Kim wrote:
> >
> > SNIP
> >
> > > +             *p++ = '\0';
> > >
> > > -                     while (token != NULL) {
> > > -                             if (subsys && !strcmp(token, subsys)) {
> > > -                                     /* found */
> > > -                                     fclose(fp);
> > > +             /* check filesystem type */
> > > +             if (strncmp(p, "cgroup", 6))
> > > +                     continue;
> > >
> > > -                                     if (strlen(mountpoint) < maxlen) {
> > > -                                             strcpy(buf, mountpoint);
> > > -                                             return 0;
> > > -                                     }
> > > -                                     return -1;
> > > -                             }
> > > -                             token = strtok_r(NULL, ",", &saved_ptr);
> > > -                     }
> > > +             if (p[6] == '2') {
> > > +                     /* save cgroup v2 path */
> > > +                     strcpy(mountpoint, path);
> > > +                     continue;
> > >               }
> > >
> > > -             if (!strcmp(type, "cgroup2"))
> > > -                     strcpy(path_v2, mountpoint);
> > > +             /* now we have cgroup v1, check the options for subsystem */
> > > +             p += 7;
> > > +
> > > +             p = strstr(p, subsys);
> >
> > not sure this is a real problem, but this would mixe up for
> > cpu/cpuacct/cpuset no? we are using the function for perf_event
> > subsys only, but it's globaly availble
> 
> Yeah, that's why I added the sanity check below. :)
> 
> >
> > > +             if (p == NULL)
> > > +                     continue;
> > > +
> > > +             /* sanity check: it should be separated by a space or a comma */
> > > +             if (!strchr(" ,", p[-1]) || !strchr(" ,", p[strlen(subsys)]))
> > > +                     continue;
> 
> Here.

aah right, sry ;-) looks ok

jirka

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

[Namhyung Kim]

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).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 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

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

[Arnaldo Carvalho de Melo]

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 <namhyung@kernel.org>
> ---
>  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

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

[Namhyung Kim]

Hi Arnaldo,

On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
<acme@kernel.org> 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

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

[Namhyung Kim]

On Wed, Jan 6, 2021 at 10:33 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Arnaldo,
>
> On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> 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

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

[Namhyung Kim]

Hi Arnaldo,

Can you share your thoughts on this?

On Fri, Jan 8, 2021 at 2:51 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Jan 6, 2021 at 10:33 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hi Arnaldo,
> >
> > On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
> > <acme@kernel.org> 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

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

[Arnaldo Carvalho de Melo]

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@kernel.org> wrote:
> >
> > Hi Arnaldo,
> >
> > On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
> > <acme@kernel.org> 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

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@kernel.org> 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@kernel.org> wrote:
> > >
> > > Hi Arnaldo,
> > >
> > > On Tue, Dec 29, 2020 at 8:51 PM Arnaldo Carvalho de Melo
> > > <acme@kernel.org> 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

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

[Arnaldo Carvalho de Melo]

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 <acme@kernel.org> 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@kernel.org> 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