Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201021211612.2026234-1-eranian@google.com> <20201022080003.GR2628@hirez.programming.kicks-ass.net>
In-Reply-To: <20201022080003.GR2628@hirez.programming.kicks-ass.net>
From:   Stephane Eranian <eranian@google.com>
Date:   Thu, 22 Oct 2020 10:24:34 -0700
Message-ID: <CABPqkBRjKXtpijMnPE-eho+tQtYmgRs2OQe_5u52HxOx-QRq_A@mail.gmail.com>
Subject: Re: [PATCH] perf/x86/intel: make anythread filter support conditional
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Andi Kleen <ak@linux.intel.com>,
        "Liang, Kan" <kan.liang@intel.com>, Jiri Olsa <jolsa@redhat.com>,
        mingo@elte.hu, Ian Rogers <irogers@google.com>,
        Namhyung Kim <namhyung@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Oct 22, 2020 at 1:00 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Oct 21, 2020 at 02:16:12PM -0700, Stephane Eranian wrote:
> > Starting with Arch Perfmon v5, the anythread filter on generic counters may be
> > deprecated. The current kernel was exporting the any filter without checking.
> > On Icelake, it means you could do cpu/event=0x3c,any/ even though the filter
> > does not exist. This patch corrects the problem by relying on the CPUID 0xa leaf
> > function to determine if anythread is supported or not as described in the
> > Intel SDM Vol3b 18.2.5.1 AnyThread Deprecation section.
> >
> > Signed-off-by: Stephane Eranian <eranian@google.com>
> > ---
> >  arch/x86/events/intel/core.c      | 20 ++++++++++++++++++++
> >  arch/x86/events/perf_event.h      |  1 +
> >  arch/x86/include/asm/perf_event.h |  4 +++-
> >  arch/x86/kvm/cpuid.c              |  4 +++-
> >  4 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> > index f1926e9f2143..65bf649048a6 100644
> > --- a/arch/x86/events/intel/core.c
> > +++ b/arch/x86/events/intel/core.c
> > @@ -4220,6 +4220,16 @@ static struct attribute *intel_arch3_formats_attr[] = {
> >       NULL,
> >  };
> >
> > +static struct attribute *intel_arch5_formats_attr[] = {
> > +     &format_attr_event.attr,
> > +     &format_attr_umask.attr,
> > +     &format_attr_edge.attr,
> > +     &format_attr_pc.attr,
> > +     &format_attr_inv.attr,
> > +     &format_attr_cmask.attr,
> > +     NULL,
> > +};
>
> Instead of adding yet another (which is an exact duplicate of the
> existing intel_arch_formats_attr BTW), can't we clean this up and use
> is_visible() as 'demanded' by GregKH and done by Jiri here:
>
>   3d5672735b23 ("perf/x86: Add is_visible attribute_group callback for base events")
>   b7c9b3927337 ("perf/x86/intel: Use ->is_visible callback for default group")
>   baa0c83363c7 ("perf/x86: Use the new pmu::update_attrs attribute group")
>
> And only have "any" visible for v3,v4

Sure, let me resubmit with these changes.