Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fc5793b2-e4f7-8639-3bbb-c934cd909c5c@kernel.org>
Date:   Wed, 2 Mar 2022 09:00:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH] tracing/histogram: Fix sorting on old "cpu" value
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>,
        LKML <linux-kernel@vger.kernel.org>
Cc:     Ingo Molnar <mingo@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Tom Zanussi <zanussi@kernel.org>
References: <20220301225728.100f17af@gandalf.local.home>
From:   Daniel Bristot de Oliveira <bristot@kernel.org>
In-Reply-To: <20220301225728.100f17af@gandalf.local.home>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/2/22 04:57, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> When trying to add a histogram against an event with the "cpu" field, it
> was impossible due to "cpu" being a keyword to key off of the running CPU.
> So to fix this, it was changed to "common_cpu" to match the other generic
> fields (like "common_pid"). But since some scripts used "cpu" for keying
> off of the CPU (for events that did not have "cpu" as a field, which is
> most of them), a backward compatibility trick was added such that if "cpu"
> was used as a key, and the event did not have "cpu" as a field name, then
> it would fallback and switch over to "common_cpu".
> 
> This fix has a couple of subtle bugs. One was that when switching over to
> "common_cpu", it did not change the field name, it just set a flag. But
> the code still found a "cpu" field. The "cpu" field is used for filtering
> and is returned when the event does not have a "cpu" field.
> 
> This was found by:
> 
>   # cd /sys/kernel/tracing
>   # echo hist:key=cpu,pid:sort=cpu > events/sched/sched_wakeup/trigger
>   # cat events/sched/sched_wakeup/hist
> 
> Which showed the histogram unsorted:
> 
> { cpu:         19, pid:       1175 } hitcount:          1
> { cpu:          6, pid:        239 } hitcount:          2
> { cpu:         23, pid:       1186 } hitcount:         14
> { cpu:         12, pid:        249 } hitcount:          2
> { cpu:          3, pid:        994 } hitcount:          5
> 
> Instead of hard coding the "cpu" checks, take advantage of the fact that
> trace_event_field_field() returns a special field for "cpu" and "CPU" if
> the event does not have "cpu" as a field. This special field has the
> "filter_type" of "FILTER_CPU". Check that to test if the returned field is
> of the CPU type instead of doing the string compare.
> 
> Also, fix the sorting bug by testing for the hist_field flag of
> HIST_FIELD_FL_CPU when setting up the sort routine. Otherwise it will use
> the special CPU field to know what compare routine to use, and since that
> special field does not have a size, it returns tracing_map_cmp_none.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1e3bac71c505 ("tracing/histogram: Rename "cpu" to "common_cpu"")
> Reported-by: Daniel Bristot de Oliveira <bristot@kernel.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

It works!

Tested-by: Daniel Bristot de Oliveira <bristot@kernel.org>

-- Daniel