Re: [QUESTION] Performance deterioration caused by commit 85f726a35e504418

["Zanussi, Tom" <tom.zanussi@linux.intel.com>]

Hi Steve,

On 10/18/2021 9:51 PM, Steven Rostedt wrote:
> On Tue, 19 Oct 2021 10:39:47 +0800
> Yang Jihong <yangjihong1@huawei.com> wrote:
> 
>> Hi Steve,
>>
>> On 2021/10/18 21:37, Steven Rostedt wrote:
>>> On Mon, 18 Oct 2021 11:23:14 +0800
>>> Yang Jihong <yangjihong1@huawei.com> wrote:
>>>    
>>>> Hi Tom and Steven,
>>>>
>>>> commit 85f726a35e504418 use strncpy instead of memcpy when copying comm,
>>>> on ARM64 machine, this commit causes performance degradation.
>>>>
>>>> I test the number of instructions executed by invoking the
>>>> trace_sched_switch function once on an arm64 machine:
>>>> 1. Use memcpy, the number of instructions executed is 850.
>>>> 2. Use strncpy, the number of instructions executed 1100.
>>>> That is, use strncpy is almost 250 more instructions than memcpy.
>>>>
>>>> Has the impact on performance been considered in this commit? :)
>>>> What is the impact of revert the patch?
>>>>   
>>>
>>> It's a security issue. And like everything security, there's always going
>>> to be a performance impact. Look at the performance impact due to spectre
>>> and meltdown!
>>>
>>> That said, although memcpy() may not be used, we don't need strncpy.
>>> strncpy() will pad the rest of the string with nul bytes. But since the
>>> memory the string is being recorded into is already initialized (or can be
>>> if it isn't), we could use the faster strlcpy().
>>>
>>> Have you tried testing it by switching strncpy() with strlcpy()?
>>>    
>> I have tried testing it by switching strncpy() with strlcpy(), there is
>> no performance improvement, probably because the strlen function is
>> called in strlpy and the string is traversed each time.
> 
> Then there's not much we can do. Security trumps performance. Not to
> mention, the garbage in the comm after the '\0' causes the histograms to
> produce strange results.
> 
> Now for the saved_cmdlines, since it isn't exported directly to user space,
> that one may be put back to memcpy().
> 
> Tom, was there a reason to change saved_cmdlines(), as I'm not sure that is
> leaked. It looks like it is printed with the normal seq_printf() in
> saved_cmdlines_show().
> 

I don't think either of the changes in commit 85f726a35e504418 are directly related to the original problem [1] and therefore changing them back to memcpy or whatever shouldn't affect the histograms since that data is never used in keys.

Commit 85f726a35e504418 was basically a follow-on to commit 9f0bbf3115ca (tracing: Use strncpy instead of memcpy for string keys in hist triggers) and was added for completeness after examining other uses of memcpy in the tracing code (there's even a comment in there from you about possible performance hits from changing it ;-)

So anyway, as far as the histograms go, I think optimizing the two changes in 85f726a35e504418 while ignoring trailing garbage can be done without affecting the histogram correctness.

Tom

[1] https://lore.kernel.org/all/50c35ae1267d64eee975b8125e151e600071d4dc.1549309756.git.tom.zanussi@linux.intel.com/

> And it doesn't even look like the saved_cmdlines() is even initialized to
> zero, so it itself could leak memory if it was exposed.
> 
> -- Steve
>