Date:   Thu, 2 Mar 2023 14:44:28 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        James Clark <james.clark@arm.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/14] perf kvm: Use histograms list to replace cached
 list
Message-ID: <ZABFzPbfEarV0G5N@leoy-huanghe>
References: <20230228115125.144172-1-leo.yan@linaro.org>
 <20230228115125.144172-10-leo.yan@linaro.org>
 <CAM9d7cgXkPw_op7tj7eZE4xP68reO8cDmwpHOa+13C1CSeOOjg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAM9d7cgXkPw_op7tj7eZE4xP68reO8cDmwpHOa+13C1CSeOOjg@mail.gmail.com>
Precedence: bulk

On Tue, Feb 28, 2023 at 04:03:27PM -0800, Namhyung Kim wrote:

[...]

> > @@ -507,22 +530,26 @@ static struct kvm_event *find_create_kvm_event(struct perf_kvm_stat *kvm,
> >                                                struct perf_sample *sample)
> >  {
> >         struct kvm_event *event;
> > -       struct list_head *head;
> > +       struct rb_root_cached *root;
> > +       struct rb_node *nd;
> >
> >         BUG_ON(key->key == INVALID_KEY);
> >
> > -       head = &kvm->kvm_events_cache[kvm_events_hash_fn(key->key)];
> > -       list_for_each_entry(event, head, hash_entry) {
> > +       if (hists__has(&kvm_hists.hists, need_collapse))
> > +               root = &kvm_hists.hists.entries_collapsed;
> > +       else
> > +               root = kvm_hists.hists.entries_in;
> > +
> > +       for (nd = rb_first_cached(root); nd; nd = rb_next(nd)) {
> > +               struct hist_entry *he = rb_entry(nd, struct hist_entry,
> > +                                                rb_node_in);
> > +
> > +               event = container_of(he, struct kvm_event, he);
> >                 if (event->key.key == key->key && event->key.info == key->info)
> >                         return event;
> 
> This seems inefficient and even unnecessary.  You should find
> the event based on the return value of hist_entry__cmp() from
> the root and go down.
> 
> But I think that's what hists__add_entry_ops() does.  Maybe
> you may need to move the init logic (like init_stats) to the
> kvm_he_zalloc().

Indeed.  I will drop above code and rely on hists__add_entry_ops()
to check if have existed event entry.

Thanks a lot for suggestion!

Leo