Re: [PATCH] operf: prefer _stext over _text as the kernel address range start

On Fri, 1 Apr 2016, William Cohen wrote:
> On 03/31/2016 01:10 PM, Michael Petlan wrote:
>> 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
>>
>> 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.
>
>>          if (strncmp(name.c_str(), KERN_ADDR_SPACE_START_SYMBOL,
>>                  strlen(name.c_str())) == 0) {
>>              /* found the symbol for the start of the kernel
>
>

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z