linux-kernel - Re: [PATCH v3 09/14] perf kvm: Use histograms list to replace cached list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZABFzPbfEarV0G5N@leoy-huanghe>
Date:   Thu, 2 Mar 2023 14:44:28 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        James Clark <james.clark@....com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 09/14] perf kvm: Use histograms list to replace cached
 list

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