Discussion:
[PATCH] operf: prefer _stext over _text as the kernel address range start
(too old to reply)
Michael Petlan
2016-03-31 17:10:57 UTC
Permalink
Raw Message
In newer kernels, _stext is the right pseudo-symbol for marking the
start address of the kernel address range. The older _text symbol is
rather architecture specific.

With this patch, the _stext symbol is preferred (if found). In case
_stext is not there (an older kernel), _text is accepted like before.
---
pe_profiling/operf.cpp | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/pe_profiling/operf.cpp b/pe_profiling/operf.cpp
index 5585b34..06a0ea3 100644
--- a/pe_profiling/operf.cpp
+++ b/pe_profiling/operf.cpp
@@ -87,8 +87,9 @@ bool track_new_forks;


#define DEFAULT_OPERF_OUTFILE "operf.data"
-#define KERN_ADDR_SPACE_START_SYMBOL "_text"
+#define KERN_ADDR_SPACE_START_SYMBOL "_stext"
#define KERN_ADDR_SPACE_END_SYMBOL "_etext"
+#define KERN_ADDR_SPACE_START_SYMBOL_OBSOLETE "_text"

static operf_record * operfRecord = NULL;
static char * app_name_SAVE = NULL;
@@ -1141,6 +1142,22 @@ static bool _process_kallsyms(void)
iss >> type;
iss >> name;

+ /* accept _text as the start symbol only in case there is no _stext
+ * because _stext has higher priority
+ */
+ if (start_addr_str.empty()) {
+ if (strncmp(name.c_str(), KERN_ADDR_SPACE_START_SYMBOL_OBSOLETE,
+ strlen(name.c_str())) == 0) {
+ /* found the symbol for the start of the kernel
+ * address space.
+ */
+ start_addr_str.assign(address_str);
+ }
+ }
+
+ /* if _stext is found, it will overwrite the _text if it has been
+ * already found
+ */
if (strncmp(name.c_str(), KERN_ADDR_SPACE_START_SYMBOL,
strlen(name.c_str())) == 0) {
/* found the symbol for the start of the kernel
--
1.8.3.1
William Cohen
2016-04-01 18:18:47 UTC
Permalink
Raw Message
Post by Michael Petlan
In newer kernels, _stext is the right pseudo-symbol for marking the
start address of the kernel address range. The older _text symbol is
rather architecture specific.
Hi Michaelm

There are kernel patches that are moving to using _stext, so operf preferring _stext makes sense. Could you add some info on which particular situation not having this patch presented a problem? Something that would allow testing to make sure that the patch works as expected.

How was the patch generated? "git am" didn't accept it when trying to apply it to the a branch of oprofile git repo. Could you resent a patch that applies?

-Will
Post by Michael Petlan
With this patch, the _stext symbol is preferred (if found). In case
_stext is not there (an older kernel), _text is accepted like before.
---
pe_profiling/operf.cpp | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/pe_profiling/operf.cpp b/pe_profiling/operf.cpp
index 5585b34..06a0ea3 100644
--- a/pe_profiling/operf.cpp
+++ b/pe_profiling/operf.cpp
@@ -87,8 +87,9 @@ bool track_new_forks;
#define DEFAULT_OPERF_OUTFILE "operf.data"
-#define KERN_ADDR_SPACE_START_SYMBOL "_text"
+#define KERN_ADDR_SPACE_START_SYMBOL "_stext"
#define KERN_ADDR_SPACE_END_SYMBOL "_etext"
+#define KERN_ADDR_SPACE_START_SYMBOL_OBSOLETE "_text"
static operf_record * operfRecord = NULL;
static char * app_name_SAVE = NULL;
@@ -1141,6 +1142,22 @@ static bool _process_kallsyms(void)
iss >> type;
iss >> name;
+ /* accept _text as the start symbol only in case there is no _stext
+ * because _stext has higher priority
+ */
+ if (start_addr_str.empty()) {
+ if (strncmp(name.c_str(), KERN_ADDR_SPACE_START_SYMBOL_OBSOLETE,
+ strlen(name.c_str())) == 0) {
+ /* found the symbol for the start of the kernel
+ * address space.
+ */
+ start_addr_str.assign(address_str);
+ }
+ }
+
+ /* if _stext is found, it will overwrite the _text if it has been
+ * already found
+ */
This logic seems a okay.
Post by Michael Petlan
if (strncmp(name.c_str(), KERN_ADDR_SPACE_START_SYMBOL,
strlen(name.c_str())) == 0) {
/* found the symbol for the start of the kernel
Michael Petlan
2016-04-18 11:34:20 UTC
Permalink
Raw Message
Post by Michael Petlan
In newer kernels, _stext is the right pseudo-symbol for marking the
start address of the kernel address range. The older _text symbol is
rather architecture specific.
Hi Michael,
There are kernel patches that are moving to using _stext, so operf preferring _stext makes sense. Could you add some info on which particular situation not having this patch presented a problem? Something that would allow testing to make sure that the patch works as expected.
Hi William,

based on previous testing and our IRC discussion, I understand that there
are two things:

1) The correct mark symbol for the start of text appears to be _stext anyway.
In case this is correct, it generally makes sense to apply the patch.

2) Usually, there are both _text and _stext and it applies that:

_text <= _stext < _etext

so sometimes, there is a reserved space between _text and _stext:

ffffffff81000000 T _text
ffffffff81000110 T secondary_startup_64
ffffffff810001b0 T start_cpu0
ffffffff810001c5 t bad_address
ffffffff810001c8 T _stext
ffffffff81001000 T hypercall_page
...

The problem occurs when there's no _text symbol and this situation happens
when the used kernel has been built with the following config:

CONFIG_KALLSYMS=y
# CONFIG_KALLSYMS_ALL is not set

It seems, that RHEL and Fedora always have both set to 'y', but I think we
cannot rely on that every distro is built in this way. So having a kernel
built with the mentioned configuration, OProfile without this patch cannot
use kallsyms and complains about kptr_restrict no matter what its value is.

----

With having both CONFIG_KALLSYMS* options set to 'y' there's no observable
problem without the patch. This explains why Will Schmidt didn't see any
change when he tested the patched operf.
How was the patch generated? "git am" didn't accept it when trying to apply it to the a branch of oprofile git repo. Could you resent a patch that applies?
I use the following steps:

git pull
git branch something
git checkout something
# change files here
git add
git commit
git format-patch master --thread --to .. --stdout | git imap-send

Let me know if you need anything else from me.

Regards,
Michael
-Will
Post by Michael Petlan
With this patch, the _stext symbol is preferred (if found). In case
_stext is not there (an older kernel), _text is accepted like before.
---
pe_profiling/operf.cpp | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/pe_profiling/operf.cpp b/pe_profiling/operf.cpp
index 5585b34..06a0ea3 100644
--- a/pe_profiling/operf.cpp
+++ b/pe_profiling/operf.cpp
@@ -87,8 +87,9 @@ bool track_new_forks;
#define DEFAULT_OPERF_OUTFILE "operf.data"
-#define KERN_ADDR_SPACE_START_SYMBOL "_text"
+#define KERN_ADDR_SPACE_START_SYMBOL "_stext"
#define KERN_ADDR_SPACE_END_SYMBOL "_etext"
+#define KERN_ADDR_SPACE_START_SYMBOL_OBSOLETE "_text"
static operf_record * operfRecord = NULL;
static char * app_name_SAVE = NULL;
@@ -1141,6 +1142,22 @@ static bool _process_kallsyms(void)
iss >> type;
iss >> name;
+ /* accept _text as the start symbol only in case there is no _stext
+ * because _stext has higher priority
+ */
+ if (start_addr_str.empty()) {
+ if (strncmp(name.c_str(), KERN_ADDR_SPACE_START_SYMBOL_OBSOLETE,
+ strlen(name.c_str())) == 0) {
+ /* found the symbol for the start of the kernel
+ * address space.
+ */
+ start_addr_str.assign(address_str);
+ }
+ }
+
+ /* if _stext is found, it will overwrite the _text if it has been
+ * already found
+ */
This logic seems a okay.
Post by Michael Petlan
if (strncmp(name.c_str(), KERN_ADDR_SPACE_START_SYMBOL,
strlen(name.c_str())) == 0) {
/* found the symbol for the start of the kernel
Loading...