utils: Fix an issue not finding tracefs on some kernels #1762

gichoel · 2023-07-16T13:34:44Z

When parsing /proc/self/mountinfo on some kernels, the number of fields is different and tracefs cannot be obtained.

changed method to check if paths of "/sys/kernel/tracing (tracefs)" and "/sys/kernel/debug/tracing (debugfs)" exist

honggyukim

Thanks for the PR. But as you mentioned, statfs is deprecated so I'm not sure if it's good to use the interface here.

I thought we could just check whether /sys/kernel/tracing/tracing_on or /sys/kernel/debug/tracing/tracing_on exists or not.

@namhyung Do you think we can use statfs check here?

utils/tracefs.c

honggyukim · 2023-07-17T13:15:15Z

utils/tracefs.c

-		pr_dbg2("Use %s as TRACING_DIR\n", TRACING_DIR);
+	/* check debugfs path is mounted */
+	statfs(DEBUGFS_DIR_PATH, &fs);
+	if (fs.f_type == DEBUGFS_MAGIC) {


This check actually fails because /sys/kernel/debug is debugfs, but /sys/kernel/debug/tracing is mounted as tracefs on top of the debugfs.

There might be a chance that old kernel uses /sys/kernel/debug/tracing as debugfs so you might be better to check both DEBUGFS_MAGIC and TRACEFS_MAGIC.

utils/tracefs.c

honggyukim · 2023-07-17T13:28:55Z

And please reword the commit message to make it begin with a verb instead of past tense. i.e. utils: Fixed ... -> utils: Fix ....

Please read other commit message to get more references by running git log and you will see all the other commit messages begin with a verb.

gichoel · 2023-07-17T14:35:04Z

Hi @honggyukim

of course, statfs was marked as deprecated as I mentioned, so I wasn't sure if it was right to use it, but I used it for the following reasons.

statvfs is an alternative to statfs, but it did not provide magic numbers for checking tracefs and debugfs.
Rather than just checking the path, I wanted to do an additional check to make sure this physical path is indeed the path we are looking for.
In Kernel version 5.19.0 running on Ubuntu 22.04, statfs was still running.

Thank you for always reviewing my code and comments and pointing out anything I've missed. I'm learning a lot thanks!

I will re-apply and commit push the feedback you suggested. 😃

honggyukim

Rather than just checking the path, I wanted to do an additional check to make sure this physical path is indeed the path we are looking for.

I think existence check is enough for /sys/kernel/{debug/}tracing/tracing_on with access call. Doing so, you can remove statfs and be free from using deprecated call.

honggyukim · 2023-07-18T13:30:05Z

utils/tracefs.c

+		xasprintf(&TRACING_DIR, "%s", TRACEFS_DIR_PATH);
+		goto found;
+	}
+	else if (!statfs("/sys/kernel/debug", &fs) && fs.f_type == DEBUGFS_MAGIC) {


Why not testing /sys/kernel/debug/tracing here? i.e. OLD_TRACEFS_DIR_PATH.

utils/tracefs.c

namhyung · 2023-07-22T01:21:03Z

Please don't remove the original logic and use it as a fallback when the quick check for the known path fails.

I'm not sure why statfs is deprecated but I guess it won't go away anytime soon. Especially if there's no better way to verify the file system type. At least linux perf tool is using statfs to find some filesystem mount locations.

Also it's not true that having debugfs at /sys/kernel/debug guarantees tracefs at /sys/kernel/debug/tracing. Actually you don't need to check /sys/kernel/debug, just check if /sys/kernel/debug/tracing is tracefs type.

honggyukim · 2023-07-22T02:06:10Z

Please don't remove the original logic and use it as a fallback when the quick check for the known path fails.

Do you want to keep the parsing logic from /proc/self/mountinfo? I think it can be error-prone and we've already found broken base because /proc/self/mountinfo sometimes has different number of fields.

I'm not sure why statfs is deprecated but I guess it won't go away anytime soon. Especially if there's no better way to verify the file system type. At least linux perf tool is using statfs to find some filesystem mount locations.

Then I'm fine using statfs here.

Also it's not true that having debugfs at /sys/kernel/debug guarantees tracefs at /sys/kernel/debug/tracing.

The new tracefs location seems to be fixed at /sys/kernel/tracing. I see that from the commit at torvalds/linux@cc31004. Steven mentioned the location as "a consistent place".

As you mentioned I'm not sure about the tracefs location at /sys/kernel/debug/tracing, but I think we can blindly try it, then go to a fallback routine if it fails.

Actually you don't need to check /sys/kernel/debug, just check if /sys/kernel/debug/tracing is tracefs type.

I also think so.

gichoel · 2023-07-22T12:26:25Z

Hi @namhyung and @honggyukim.

I agree with honggyukim, it's better to use /proc/mounts because /proc/self/mountinfo can cause errors due to the different number of fields.

So I thought it would be better to remove the existing logic using /proc/self/mountinfo and use /proc/mounts and statfs() to trace a path of tracefs.

I still don't understand why you don't remove the existing logic, can you explain why?

namhyung · 2023-07-23T11:07:45Z

OK, I agree to use /proc/mounts and I was confused the existing logic already used it. I thought you just wanted to check the well-known locations like /sys/kernel/tracing without checking /proc/mounts.

namhyung

The commit message is too long, please break long lines not to exceed 72 characters.

namhyung · 2023-07-26T13:05:23Z

utils/tracefs.c

+				else if (!strcmp(ent->mnt_fsname, "debugfs")) {
+					xasprintf(&TRACING_DIR, "%s/tracing", ent->mnt_dir);
+					found = true;
+				}


I think this 'else if' part should be removed because there's no guarantee for tracefs even if it found debugfs.

I'll accept the feedback and upload the changed code.

namhyung · 2023-07-26T13:05:59Z

utils/tracefs.c

+			}
+
+			endmntent(fp);
+		} while (0);


Why did you use do ... while (0) block?

First, I wanted to go down without executing the while() { ... } and endmntent() below when the result of the setmntent() function is NULL.

Second, I didn't want to keep the if ~ else statements too long. If the if ~ else statement is too long or deep it becomes hard to read. so I wanted to use do ~ while(0) which is used by the Linux kernel to avoid this.

I thought this was best, but if there is a better alternative, I'm open to it.

I think you can split the check for TRACEFS_DIR_PATH into a separate commit and focus on the mntent change here. Then it can be like this.

fp = setmntent("/proc/mounts", "r"); if (fp == NULL) return false; while ((ent = getmntent(fp)) != NULL) { if (strcmp(ent->mnt_fsname, "tracefs")) continue; xasprintf(&TRACING_DIR, "%s", ent->mnt_dir); found = true; break; } endmntent(fp); return found;

namhyung · 2023-07-26T13:07:21Z

utils/tracefs.c

-	size_t len;
-
-	if (TRACING_DIR)
-		return false;


You removed this caching logic..

I'll revert the caching logic.

Hmm.. I'm not sure why the original code returns false.

I couldn't figure out why the original code was returning false either.

The original code works fine, but I think it's semantically correct for the caching logic to return true.

namhyung

I'm ok with the change roughly but want to have the commits in the reverse order. The first commit removed the original logic and added a new code to quickly check the well-known path. That's +1 and -1 in terms of the functionality. And the second commit added /proc parsing back, changing -1 to 0.

I'd like to have a commit that fixes /proc parsing first so that we don't lose any functionality in the meantime (like +1 to +1, no change). Then add the the quick check in the second commit (+1 to +2). Even though the net result is the same (+1), this way we can gaurantee it only adds and never loses. :)

namhyung · 2023-07-31T09:59:52Z

utils/tracefs.c

 	bool found = false;

 	if (TRACING_DIR)
 		return true;

 	if (!statfs(TRACEFS_DIR_PATH, &fs) && fs.f_type == TRACEFS_MAGIC) {
 		xasprintf(&TRACING_DIR, "%s", TRACEFS_DIR_PATH);
-		pr_dbg2("TRACING_DIR : %s\n", TRACING_DIR);


You can keep this or not introduce in the first place.

utils/tracefs.c

honggyukim

Thanks for the update. I have some comments.

In addition, the link in the below commit message is broken.

However, we can get faster performance by checking the
default tracefs path given in https://github.com/torvalds/linux/commit/cc31004
before checking the previously added logic.

I think it wouldn't make faster performance because this path is not in hot spot. It's just to simplify the logic.

utils/tracefs.c

honggyukim · 2023-08-07T13:41:18Z

I've merged a few commits into master so please rebase and update this PR onto the current master.

This logic change is to address issue namhyung#1753, which was causing issue on some kernels. The previous logic used '/proc/self/mountinfo', but in some cases the number of fields was different, causing a parse error. This has been fixed by using getmntent() and setmntent() to parse '/proc/mounts' structurally. Signed-off-by: Gichoel Choi <gichoel3101@gmail.com>

gichoel · 2023-08-07T14:20:02Z

@honggyukim, I replaced the broken commit link and re-uploaded the commit with the feedback.

honggyukim

Thanks for the update. I have two last comments. One is the inlined comment below.

The other is that I would like to ask you to use URL from the official kernel repo instead of github one.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2455f0e

In addition, the link can be clicked in github, but not from the commit message itself because it's shown as just torvalds/linux@2455f0e, which is not a complete URL.

Instead of it, please add the line below and just above Signed-off-by line as follows.

Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2455f0e
Signed-off-by: Gichoel Choi <gichoel3101@gmail.com>

utils/tracefs.c

namhyung · 2023-08-09T06:39:59Z

The other is that I would like to ask you to use URL from the official kernel repo instead of github one. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2455f0e

Actually 7 digit is not enough for kernel commits due to possible collision, kernel community recommends 12. And there's a short form of the URL like: https://git.kernel.org/torvalds/c/2455f0e124d3

honggyukim · 2023-08-09T23:59:51Z

utils/tracefs.c

+	if ((!statfs(TRACEFS_DIR_PATH, &fs) && fs.f_type == TRACEFS_MAGIC) ||
+	    (!statfs(OLD_TRACEFS_DIR_PATH, &fs) && fs.f_type == TRACEFS_MAGIC)) {
+		xasprintf(&TRACING_DIR, "%s", TRACEFS_DIR_PATH);
+		return true;
+	}


Hmm.. Does it really handle when only /sys/kernel/debug/tracing is available? This logic sets TRACING_DIR to /sys/kernel/tracing in both cases.

I've incorporated feedback on what I missed.

It's not that important but the expression "incorporated feedback" sounds awkward. You can just say that "I've updated it based on your feedback".

honggyukim · 2023-08-10T11:37:24Z

utils/tracefs.c

+	if (!statfs(TRACEFS_DIR_PATH, &fs) && fs.f_type == TRACEFS_MAGIC) {
+		xasprintf(&TRACING_DIR, "%s", TRACEFS_DIR_PATH);
+		found = true;
+	}
+	else if (!statfs(OLD_TRACEFS_DIR_PATH, &fs) && fs.f_type == TRACEFS_MAGIC) {
+		xasprintf(&TRACING_DIR, "%s", OLD_TRACEFS_DIR_PATH);
+		found = true;
+	}

-	fp = fopen(PROC_MOUNTINFO, "r");
+	if (found)
+		return true;


I would rather write this part as follows.

if (!statfs(TRACEFS_DIR_PATH, &fs) && fs.f_type == TRACEFS_MAGIC) { xasprintf(&TRACING_DIR, "%s", TRACEFS_DIR_PATH); return true; } if (!statfs(OLD_TRACEFS_DIR_PATH, &fs) && fs.f_type == TRACEFS_MAGIC) { xasprintf(&TRACING_DIR, "%s", OLD_TRACEFS_DIR_PATH); return true; }

This is to reduce one more branch from if (found).

The logic we added previously was to use '/proc/mounts' to find the tracefs path. However, the logic can be simplified even further by using the canonical ftrace path, as per commit from torvalds/linux@2455f0e. Link: https://git.kernel.org/torvalds/c/2455f0e124d3 Signed-off-by: Gichoel Choi <gichoel3101@gmail.com>

honggyukim

LGTM! Thanks very much for your patience.

gichoel · 2023-08-10T13:55:50Z

Thank you @namhyung and @honggyukim for the great feedback each time.

I learned a lot from this PR and issue :)

namhyung

LGTM

The currnet find_tracing_dir logic parses /proc/self/mounts to find tracefs, but some cases show the number of fields in /proc/self/mounts differs and it makes a parsing failure. Gichoel fixes this issue with two steps as follows. 1. Try known paths at /sys/kernel/tracing and /sys/kernel/debug/tracing 2. Use setmntent/getmntent to identify tracefs from /proc/mounts. Signed-off-by: Honggyu Kim <honggyu.kp@gmail.com>

gichoel force-pushed the find-tracefs-issue branch from e835911 to edb43bd Compare July 16, 2023 14:12

honggyukim reviewed Jul 17, 2023

View reviewed changes

honggyukim added the kernel label Jul 17, 2023

gichoel force-pushed the find-tracefs-issue branch from edb43bd to d2728d2 Compare July 17, 2023 15:38

honggyukim reviewed Jul 18, 2023

View reviewed changes

gichoel changed the title ~~utils: Fixed an issue not finding tracefs on some kernels~~ utils: Fix an issue not finding tracefs on some kernels Jul 23, 2023

gichoel force-pushed the find-tracefs-issue branch from d2728d2 to afef79f Compare July 25, 2023 16:03

namhyung reviewed Jul 26, 2023

View reviewed changes

gichoel force-pushed the find-tracefs-issue branch 3 times, most recently from bc8c281 to 0743e95 Compare July 30, 2023 07:11

namhyung reviewed Jul 31, 2023

View reviewed changes

gichoel force-pushed the find-tracefs-issue branch 2 times, most recently from 98835ae to c3c5df3 Compare August 6, 2023 14:06

honggyukim reviewed Aug 7, 2023

View reviewed changes

utils/tracefs.c Outdated Show resolved Hide resolved

gichoel force-pushed the find-tracefs-issue branch from c3c5df3 to d39158a Compare August 7, 2023 14:18

honggyukim reviewed Aug 8, 2023

View reviewed changes

utils/tracefs.c Outdated Show resolved Hide resolved

gichoel force-pushed the find-tracefs-issue branch from d39158a to c9b7691 Compare August 9, 2023 15:01

honggyukim reviewed Aug 9, 2023

View reviewed changes

gichoel force-pushed the find-tracefs-issue branch from c9b7691 to 44f1a26 Compare August 10, 2023 10:39

honggyukim reviewed Aug 10, 2023

View reviewed changes

gichoel force-pushed the find-tracefs-issue branch from 44f1a26 to a689290 Compare August 10, 2023 12:04

honggyukim approved these changes Aug 10, 2023

View reviewed changes

namhyung approved these changes Aug 11, 2023

View reviewed changes

honggyukim merged commit afeda9f into namhyung:master Aug 11, 2023
3 checks passed

honggyukim mentioned this pull request Aug 15, 2023

Incorrect help message when uftrace fails to trace kernel functions #1471

Closed

Porkepix mentioned this pull request Jan 8, 2024

uftrace 0.15 Homebrew/homebrew-core#159317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

utils: Fix an issue not finding tracefs on some kernels #1762

utils: Fix an issue not finding tracefs on some kernels #1762

gichoel commented Jul 16, 2023 •

edited

honggyukim left a comment

honggyukim Jul 17, 2023

honggyukim commented Jul 17, 2023

gichoel commented Jul 17, 2023

honggyukim left a comment

honggyukim Jul 18, 2023

namhyung commented Jul 22, 2023

honggyukim commented Jul 22, 2023

gichoel commented Jul 22, 2023

namhyung commented Jul 23, 2023

namhyung left a comment

namhyung Jul 26, 2023

gichoel Jul 26, 2023

namhyung Jul 26, 2023

gichoel Jul 26, 2023

namhyung Jul 29, 2023

namhyung Jul 26, 2023

gichoel Jul 26, 2023

namhyung Jul 29, 2023

gichoel Jul 30, 2023

namhyung left a comment

namhyung Jul 31, 2023

honggyukim left a comment

honggyukim commented Aug 7, 2023

gichoel commented Aug 7, 2023

honggyukim left a comment

namhyung commented Aug 9, 2023

honggyukim Aug 9, 2023

gichoel Aug 10, 2023

honggyukim Aug 10, 2023

honggyukim Aug 10, 2023 •

edited

honggyukim left a comment

gichoel commented Aug 10, 2023

namhyung left a comment

utils: Fix an issue not finding tracefs on some kernels #1762

utils: Fix an issue not finding tracefs on some kernels #1762

Conversation

gichoel commented Jul 16, 2023 • edited

honggyukim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honggyukim commented Jul 17, 2023

gichoel commented Jul 17, 2023

honggyukim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namhyung commented Jul 22, 2023

honggyukim commented Jul 22, 2023

gichoel commented Jul 22, 2023

namhyung commented Jul 23, 2023

namhyung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namhyung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honggyukim left a comment

Choose a reason for hiding this comment

honggyukim commented Aug 7, 2023

gichoel commented Aug 7, 2023

honggyukim left a comment

Choose a reason for hiding this comment

namhyung commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honggyukim Aug 10, 2023 • edited

Choose a reason for hiding this comment

honggyukim left a comment

Choose a reason for hiding this comment

gichoel commented Aug 10, 2023

namhyung left a comment

Choose a reason for hiding this comment

gichoel commented Jul 16, 2023 •

edited

honggyukim Aug 10, 2023 •

edited