linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Andy Lutomirski" <luto@kernel.org>
To: "Rob Herring" <robh@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Will Deacon" <will@kernel.org>,
	"Kan Liang" <kan.liang@linux.intel.com>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Arnaldo Carvalho de Melo" <acme@kernel.org>,
	"Alexander Shishkin" <alexander.shishkin@linux.intel.com>,
	"Jiri Olsa" <jolsa@redhat.com>,
	"Namhyung Kim" <namhyung@kernel.org>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Borislav Petkov" <bp@alien8.de>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	"Dave Hansen" <dave.hansen@linux.intel.com>,
	linux-perf-users@vger.kernel.org
Subject: Re: [RFC 2/3] perf/x86: Control RDPMC access from .enable() hook
Date: Thu, 26 Aug 2021 11:13:10 -0700	[thread overview]
Message-ID: <de97454b-9b4d-492f-a435-6a5e33889219@www.fastmail.com> (raw)
In-Reply-To: <CAL_JsqLRv9ugKJcn4dq_ps0JMt74Y7PKA=5yySYxvftdQWzzPA@mail.gmail.com>



On Thu, Aug 12, 2021, at 11:16 AM, Rob Herring wrote:
> On Thu, Aug 12, 2021 at 11:50 AM Andy Lutomirski <luto@kernel.org> wrote:
> >
> > On 7/28/21 4:02 PM, Rob Herring wrote:
> > > Rather than controlling RDPMC access behind the scenes from switch_mm(),
> > > move RDPMC access controls to the PMU .enable() hook. The .enable() hook
> > > is called whenever the perf CPU or task context changes which is when
> > > the RDPMC access may need to change.
> > >
> > > This is the first step in moving the RDPMC state tracking out of the mm
> > > context to the perf context.
> >
> > Is this series supposed to be a user-visible change or not?  I'm confused.
> 
> It should not be user-visible. Or at least not user-visible for what
> any user would notice. If an event is not part of the perf context on
> another thread sharing the mm, does that thread need rdpmc access? No
> access would be a user-visible change, but I struggle with how that's
> a useful scenario?
> 

This is what I mean by user-visible -- it changes semantics in a way that a user program could detect.  I'm not saying it's a problem, but I do think you need to document the new semantics.

> > If you intend to have an entire mm have access to RDPMC if an event is
> > mapped, then surely access needs to be context switched for the whole
> > mm.  If you intend to only have the thread to which the event is bound
> > have access, then the only reason I see to use IPIs is to revoke access
> > on munmap from the wrong thread.  But even that latter case could be
> > handled with a more targeted approach, not a broadcast to all of mm_cpumask.
> 
> Right, that's what patch 3 does. When we mmap/munmap an event, then
> the perf core invokes the callback only on the active contexts in
> which the event resides.
> 
> > Can you clarify what the overall intent is and what this particular
> > patch is trying to do?
> >
> > >
> > >       if (atomic_dec_and_test(&mm->context.perf_rdpmc_allowed))
> > > -             on_each_cpu_mask(mm_cpumask(mm), cr4_update_pce, NULL, 1);
> > > +             on_each_cpu_mask(mm_cpumask(mm), x86_pmu_set_user_access_ipi, NULL, 1);
> >
> > Here you do something for the whole mm.
> 
> It's just a rename of the callback though.
> 
> >
> > > -             on_each_cpu(cr4_update_pce, NULL, 1);
> > > +             on_each_cpu(x86_pmu_set_user_access_ipi, NULL, 1);
> >
> > Here too.
> 
> It's not just the whole mm here as changing sysfs rdpmc permission is
> a global state.

Whoops, missed that.

> 
> > >  void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
> > >                       struct task_struct *tsk)
> > >  {
> > > @@ -581,10 +556,8 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
> > >       this_cpu_write(cpu_tlbstate.loaded_mm, next);
> > >       this_cpu_write(cpu_tlbstate.loaded_mm_asid, new_asid);
> > >
> > > -     if (next != real_prev) {
> > > -             cr4_update_pce_mm(next);
> > > +     if (next != real_prev)
> > >               switch_ldt(real_prev, next);
> > > -     }
> > >  }
> >
> > But if you remove this, then what handles context switching?
> 
> perf. On a context switch, perf is going to context switch the events
> and set the access based on an event in the context being mmapped.
> Though I guess if rdpmc needs to be enabled without any events opened,
> this is not going to work. So maybe I need to keep the
> rdpmc_always_available_key and rdpmc_never_available_key cases here.

You seem to have a weird combination of per-thread and per-mm stuff going on in this patch, though.  Maybe it's all reasonable after patch 3 is applied, but this patch is very hard to review in its current state.

  reply	other threads:[~2021-08-26 18:13 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-28 23:02 [RFC 0/3] perf/x86: Rework RDPMC access handling Rob Herring
2021-07-28 23:02 ` [RFC 1/3] x86: perf: Move RDPMC event flag to a common definition Rob Herring
2021-07-28 23:02 ` [RFC 2/3] perf/x86: Control RDPMC access from .enable() hook Rob Herring
2021-08-12 16:50   ` Andy Lutomirski
2021-08-12 18:16     ` Rob Herring
2021-08-26 18:13       ` Andy Lutomirski [this message]
2021-08-26 19:09         ` Rob Herring
2021-08-27 21:10           ` Andy Lutomirski
2021-08-30  3:05             ` Vince Weaver
2021-08-30  8:51               ` Peter Zijlstra
2021-08-30 20:21                 ` Vince Weaver
2021-08-30 21:40                   ` Rob Herring
2021-08-30 20:58               ` Rob Herring
2021-07-28 23:02 ` [RFC 3/3] perf/x86: Call mmap event callbacks on event's CPU Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=de97454b-9b4d-492f-a435-6a5e33889219@www.fastmail.com \
    --to=luto@kernel.org \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=jolsa@redhat.com \
    --cc=kan.liang@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --cc=robh@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=will@kernel.org \
    --cc=x86@kernel.org \
    --subject='Re: [RFC 2/3] perf/x86: Control RDPMC access from .enable() hook' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).