Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1383277 > unrolled thread

Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to change compatible mode

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to  change compatible mode Peter Zijlstra <peterz@infradead.org> - 2016-04-20 13:10 +0200
    Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to  change compatible mode Andy Lutomirski <luto@amacapital.net> - 2016-04-20 17:50 +0200
      Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to  change compatible mode Peter Zijlstra <peterz@infradead.org> - 2016-04-20 21:10 +0200
        Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to  change compatible mode Andy Lutomirski <luto@amacapital.net> - 2016-04-21 21:50 +0200
          Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to  change compatible mode Peter Zijlstra <peterz@infradead.org> - 2016-04-21 22:20 +0200
            Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to  change compatible mode Andy Lutomirski <luto@amacapital.net> - 2016-04-22 01:30 +0200
              Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to  change compatible mode Andy Lutomirski <luto@amacapital.net> - 2016-04-22 01:50 +0200
              Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to  change compatible mode Peter Zijlstra <peterz@infradead.org> - 2016-04-25 17:20 +0200
                Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to  change compatible mode Andy Lutomirski <luto@amacapital.net> - 2016-04-25 19:00 +0200

#1383277 — Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to change compatible mode

On Thu, Apr 14, 2016 at 11:27:35AM -0700, Andy Lutomirski wrote:
> On Wed, Apr 13, 2016 at 9:55 AM, Dmitry Safonov <dsafonov@virtuozzo.com> wrote:
> > On 04/08/2016 11:44 PM, Andy Lutomirski wrote:
> >>
> >> Feel free to ask for help on some of these details.  user_64bit_mode
> >> will be helpful too.
> >
> > Hello again,
> >
> > here are some questions on  TIF_IA32 removal:
> > - in function intel_pmu_pebs_fixup_ip: there is need to
> > know if process was it native/compat mode for instruction
> > interpreter for IP + one instruction fixup. There are
> > registers, but they are from PEBS, which does not contain
> > segment descriptors (even for PEBSv3). Other values
> > are from interrupt regs (look at setup_pebs_sample_data).
> > So, I guess, we may use user_64bit_mode on interrupt
> > register set, which will be racy with changing task's mode,
> > but quite ok?
> 
> Here's my understanding:
> 
> We don't actually know the mode, and there's no way we could get it
> exactly.  User code could have changed the mode between when the PEBS
> event was written and when we got the interrupt, and there's no way
> for us to tell.
> 
> The regs passed to the interrupt aren't particularly helpful -- if we
> get the overflow event from kernel mode, the regs will be kernel regs,
> not user regs.
> 
> What we can do is to the the regs returned by perf_get_regs_user,
> which I imagine perf is already doing.  Peter, is this the case?

*confused*, how is perf_get_regs_user() connected to the PEBS fixup?

Ah, you want to use perf_get_regs_user() instead of task_pt_regs()
because of how an NMI during interrupt entry would mess up the
task_pt_regs() contents.

At that point you can use regs_user->abi, right?

> If necessary, starting in 4.6, I could make the regs->cs part of
> perf_get_regs_user be correct no matter what -- the only funny cases
> left are NMI-in-system-call-prologue (there can't be intervening
> interrupts any more other than MCE, and I don't think we really care
> if we report correct PEBS results if we take a machine check in the
> middle).
> 
> > - the same with LBR branching: I may got cs value for
> > user_64bit_mode or all registers set from intel_pmu_handle_irq
> > and pass it through intel_pmu_lbr_read => intel_pmu_lbr_filter
> > to branch_type for instruction decoder, which may
> > missinterpret opcode for the same racy-mode-switching app.
> > Is it also fine?
> 
> Same thing, I think.

Yep, whatever works for PEBS should also work for the LBR case. Both can
handle an occasional failed decode. Esp. if userspace is doing daft
things like changing the mode, you get to keep whatever pieces result
from that.

[toc] | [next] | [standalone]

#1383515

On Wed, Apr 20, 2016 at 4:04 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Apr 14, 2016 at 11:27:35AM -0700, Andy Lutomirski wrote:
>> On Wed, Apr 13, 2016 at 9:55 AM, Dmitry Safonov <dsafonov@virtuozzo.com> wrote:
>> > On 04/08/2016 11:44 PM, Andy Lutomirski wrote:
>> >>
>> >> Feel free to ask for help on some of these details.  user_64bit_mode
>> >> will be helpful too.
>> >
>> > Hello again,
>> >
>> > here are some questions on  TIF_IA32 removal:
>> > - in function intel_pmu_pebs_fixup_ip: there is need to
>> > know if process was it native/compat mode for instruction
>> > interpreter for IP + one instruction fixup. There are
>> > registers, but they are from PEBS, which does not contain
>> > segment descriptors (even for PEBSv3). Other values
>> > are from interrupt regs (look at setup_pebs_sample_data).
>> > So, I guess, we may use user_64bit_mode on interrupt
>> > register set, which will be racy with changing task's mode,
>> > but quite ok?
>>
>> Here's my understanding:
>>
>> We don't actually know the mode, and there's no way we could get it
>> exactly.  User code could have changed the mode between when the PEBS
>> event was written and when we got the interrupt, and there's no way
>> for us to tell.
>>
>> The regs passed to the interrupt aren't particularly helpful -- if we
>> get the overflow event from kernel mode, the regs will be kernel regs,
>> not user regs.
>>
>> What we can do is to the the regs returned by perf_get_regs_user,
>> which I imagine perf is already doing.  Peter, is this the case?
>
> *confused*, how is perf_get_regs_user() connected to the PEBS fixup?
>
> Ah, you want to use perf_get_regs_user() instead of task_pt_regs()
> because of how an NMI during interrupt entry would mess up the
> task_pt_regs() contents.
>
> At that point you can use regs_user->abi, right?

Yes, exactly.

Do LBR, PEBS, and similar report user regs or do they merely want to
know the instruction format?  If the latter, I could whip up a tiny
function to do just that (like perf_get_regs_user but just for ABI --
it would be simpler).

[merging some emails]

>> Peter, I got lost in the code that calls this.  Are regs coming from
>> the overflow interrupt's regs, current_pt_regs(), or
>> perf_get_regs_user?
>
> So get_perf_callchain() will get regs from:
>
>  - interrupt/NMI regs
>  - perf_arch_fetch_caller_regs()
>
> And when user && !user_mode(), we'll use:
>
>  - task_pt_regs() (which arguably should maybe be perf_get_regs_user())

Could you point me to this bit of the code?

>
> to call perf_callchain_user(), which then, ands up calling
> perf_callchain_user32() which is expected to NO-OP for 64bit userspace.
>
>> If it's the perf_get_regs_user, then this should be okay, but passing
>> in the ABI field directly would be even nicer.  If they're coming from
>> the overflow interrupt's regs or current_pt_regs(), could we change
>> that?
>>
>> It might also be nice to make sure that we call perf_get_regs_user
>> exactly once per overflow interrupt -- i.e. we could push it into the
>> main code rather than the regs sampling code.
>
> The risk there is that we might not need the user regs at all to handle
> the overflow thingy, so doing it unconditionally would be unwanted.

One call to perf_get_user_regs per interrupt shouldn't be too bad --
certainly much better then one per PEBS record.  One call to get user
ABI per overflow would be even less bad, but at that point, folding it
in to the PEBS code wouldn't be so bad either.

If I'm understanding this right (a big, big if), if we get a PEBS
overflow while running in user mode, we'll dump out the user regs (and
call perf_get_regs_user) and all the PEBS entries (subject to
exclude_kernel and with all the decoding magic).  So, in that case, we
call perf_get_user_regs.

If we get a PEBS overflow while running in kernel mode, we'll report
the kernel regs (if !exclude_kernel) and report the PEBS data as well.
If any of those records are in user mode, then, ideally, we'd invoke
perf_get_regs_user or similar *once* to get the ABI.  Although, if we
can get the user ABI efficiently enough, then maybe we don't care if
we call it once per PEBS record.

On x86, the only weird cases are NMIs or MCEs that land in the
syscall, syscall32, and sysenter prologues (easy to handle fully
correctly if we care because the IP that we interrupted tells us the
ABI) and the bullshit SYSENTER+TF thing.  Even the latter isn't so
hard to get right.

--Andy

[toc] | [prev] | [next] | [standalone]

#1383649

On Wed, Apr 20, 2016 at 08:40:23AM -0700, Andy Lutomirski wrote:
> Do LBR, PEBS, and similar report user regs or do they merely want to
> know the instruction format?  If the latter, I could whip up a tiny
> function to do just that (like perf_get_regs_user but just for ABI --
> it would be simpler).

Just the instruction format, nothing else.

> >> Peter, I got lost in the code that calls this.  Are regs coming from
> >> the overflow interrupt's regs, current_pt_regs(), or
> >> perf_get_regs_user?
> >
> > So get_perf_callchain() will get regs from:
> >
> >  - interrupt/NMI regs
> >  - perf_arch_fetch_caller_regs()
> >
> > And when user && !user_mode(), we'll use:
> >
> >  - task_pt_regs() (which arguably should maybe be perf_get_regs_user())
> 
> Could you point me to this bit of the code?

kernel/events/callchain.c:198

> > to call perf_callchain_user(), which then, ands up calling
> > perf_callchain_user32() which is expected to NO-OP for 64bit userspace.
> >
> >> If it's the perf_get_regs_user, then this should be okay, but passing
> >> in the ABI field directly would be even nicer.  If they're coming from
> >> the overflow interrupt's regs or current_pt_regs(), could we change
> >> that?
> >>
> >> It might also be nice to make sure that we call perf_get_regs_user
> >> exactly once per overflow interrupt -- i.e. we could push it into the
> >> main code rather than the regs sampling code.
> >
> > The risk there is that we might not need the user regs at all to handle
> > the overflow thingy, so doing it unconditionally would be unwanted.
> 
> One call to perf_get_user_regs per interrupt shouldn't be too bad --
> certainly much better then one per PEBS record.  One call to get user
> ABI per overflow would be even less bad, but at that point, folding it
> in to the PEBS code wouldn't be so bad either.

Right; although note that the whole fixup_ip() thing requires a single
record per interrupt (for we need the LBR state for each record in order
to rewind).

Also, HSW+ PEBS doesn't do the fixup anymore.

> If I'm understanding this right (a big, big if), if we get a PEBS
> overflow while running in user mode, we'll dump out the user regs (and
> call perf_get_regs_user) and all the PEBS entries (subject to
> exclude_kernel and with all the decoding magic).  So, in that case, we
> call perf_get_user_regs.

We only dump user regs if PERF_SAMPLE_REGS_USER, and in case we hit
userspace userspace with the interrupt we use the interrupt regs; see
perf_sample_regs_user().

> If we get a PEBS overflow while running in kernel mode, we'll report
> the kernel regs (if !exclude_kernel) and report the PEBS data as well.
> If any of those records are in user mode, then, ideally, we'd invoke
> perf_get_regs_user or similar *once* to get the ABI.  Although, if we
> can get the user ABI efficiently enough, then maybe we don't care if
> we call it once per PEBS record.

Right, if we interrupt kernel mode, we'll call perf_get_regs_user() if
PERF_SAMPLE_REGS_USER (| PERF_SAMPLE_STACK_USER).

The problem here is that the overflow stuff is designed for a single
'event' per interrupt, so passing it data for multiple events is
somewhat icky.

[toc] | [prev] | [next] | [standalone]

#1384536

On Wed, Apr 20, 2016 at 12:05 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Apr 20, 2016 at 08:40:23AM -0700, Andy Lutomirski wrote:
>> Do LBR, PEBS, and similar report user regs or do they merely want to
>> know the instruction format?  If the latter, I could whip up a tiny
>> function to do just that (like perf_get_regs_user but just for ABI --
>> it would be simpler).
>
> Just the instruction format, nothing else.
>
>> >> Peter, I got lost in the code that calls this.  Are regs coming from
>> >> the overflow interrupt's regs, current_pt_regs(), or
>> >> perf_get_regs_user?
>> >
>> > So get_perf_callchain() will get regs from:
>> >
>> >  - interrupt/NMI regs
>> >  - perf_arch_fetch_caller_regs()
>> >
>> > And when user && !user_mode(), we'll use:
>> >
>> >  - task_pt_regs() (which arguably should maybe be perf_get_regs_user())
>>
>> Could you point me to this bit of the code?
>
> kernel/events/callchain.c:198

But that only applies to the callchain code, right?  AFAICS the PEBS
code is invoked through the x86_pmu NMI handler and always gets the
IRQ regs.  Except for this case:

static inline void intel_pmu_drain_pebs_buffer(void)
{
    struct pt_regs regs;

    x86_pmu.drain_pebs(&regs);
}

which seems a bit confused.

I don't suppose we could arrange to pass something consistent into the
PEBS handlers...

Or is the PEBS code being called from the callchain code somehow?

I haven't dug in to the LBR code much.

>>
>> One call to perf_get_user_regs per interrupt shouldn't be too bad --
>> certainly much better then one per PEBS record.  One call to get user
>> ABI per overflow would be even less bad, but at that point, folding it
>> in to the PEBS code wouldn't be so bad either.
>
> Right; although note that the whole fixup_ip() thing requires a single
> record per interrupt (for we need the LBR state for each record in order
> to rewind).

So do earlier PEBS events not get rewound?  Or so we just program the
thing to only ever give us one event at a time?

>
> Also, HSW+ PEBS doesn't do the fixup anymore.
>
>> If I'm understanding this right (a big, big if), if we get a PEBS
>> overflow while running in user mode, we'll dump out the user regs (and
>> call perf_get_regs_user) and all the PEBS entries (subject to
>> exclude_kernel and with all the decoding magic).  So, in that case, we
>> call perf_get_user_regs.
>
> We only dump user regs if PERF_SAMPLE_REGS_USER, and in case we hit
> userspace userspace with the interrupt we use the interrupt regs; see
> perf_sample_regs_user().
>
>> If we get a PEBS overflow while running in kernel mode, we'll report
>> the kernel regs (if !exclude_kernel) and report the PEBS data as well.
>> If any of those records are in user mode, then, ideally, we'd invoke
>> perf_get_regs_user or similar *once* to get the ABI.  Although, if we
>> can get the user ABI efficiently enough, then maybe we don't care if
>> we call it once per PEBS record.
>
> Right, if we interrupt kernel mode, we'll call perf_get_regs_user() if
> PERF_SAMPLE_REGS_USER (| PERF_SAMPLE_STACK_USER).

But not in get_perf_callchain.  So we'll show the correct user *regs*
but not the current user callchain under some conditions, AFAICS.

>
> The problem here is that the overflow stuff is designed for a single
> 'event' per interrupt, so passing it data for multiple events is
> somewhat icky.

It also seems that there's a certain amount of confusion as to exactly
what "regs" means in various contexts.  Or at least I'm confused by
it.

--Andy

[toc] | [prev] | [next] | [standalone]

#1384552

On Thu, Apr 21, 2016 at 12:39:42PM -0700, Andy Lutomirski wrote:
> On Wed, Apr 20, 2016 at 12:05 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Wed, Apr 20, 2016 at 08:40:23AM -0700, Andy Lutomirski wrote:

> >> >> Peter, I got lost in the code that calls this.  Are regs coming from
> >> >> the overflow interrupt's regs, current_pt_regs(), or
> >> >> perf_get_regs_user?
> >> >
> >> > So get_perf_callchain() will get regs from:
> >> >
> >> >  - interrupt/NMI regs
> >> >  - perf_arch_fetch_caller_regs()
> >> >
> >> > And when user && !user_mode(), we'll use:
> >> >
> >> >  - task_pt_regs() (which arguably should maybe be perf_get_regs_user())
> >>
> >> Could you point me to this bit of the code?
> >
> > kernel/events/callchain.c:198
> 
> But that only applies to the callchain code, right? 

Yes, which is what I thought you were after..

> AFAICS the PEBS
> code is invoked through the x86_pmu NMI handler and always gets the
> IRQ regs.  Except for this case:
> 
> static inline void intel_pmu_drain_pebs_buffer(void)
> {
>     struct pt_regs regs;
> 
>     x86_pmu.drain_pebs(&regs);
> }
> 
> which seems a bit confused.

Yes, so that only gets used with 'large' pebs, which requires no other
flags than PERF_FRERERUNNING_FLAGS, which precludes the regs set from
being used.

Could definitely use a comment.

> I don't suppose we could arrange to pass something consistent into the
> PEBS handlers...
> 
> Or is the PEBS code being called from the callchain code somehow?

No. I think we were/are slightly talking past one another.

> >> One call to perf_get_user_regs per interrupt shouldn't be too bad --
> >> certainly much better then one per PEBS record.  One call to get user
> >> ABI per overflow would be even less bad, but at that point, folding it
> >> in to the PEBS code wouldn't be so bad either.
> >
> > Right; although note that the whole fixup_ip() thing requires a single
> > record per interrupt (for we need the LBR state for each record in order
> > to rewind).
> 
> So do earlier PEBS events not get rewound?  Or so we just program the
> thing to only ever give us one event at a time?

The latter; we program PEBS such that it can hold but a single record
and thereby assure we get an interrupt for each record.

> > The problem here is that the overflow stuff is designed for a single
> > 'event' per interrupt, so passing it data for multiple events is
> > somewhat icky.
> 
> It also seems that there's a certain amount of confusion as to exactly
> what "regs" means in various contexts.  Or at least I'm confused by
> it.

Yes, there's too much regs.

Typically 'regs' is the 'interrrupt'/'event' regs, that is the register
set at eventing time. For sampling hardware PMUs this is NMI/IRQ like
things, for software events this ends up being
perf_arch_fetch_caller_regs().

Then there's PERF_SAMPLE_REGS_USER|PERF_SAMPLE_STACK_USER, which, for
each event with it set, use perf_get_regs_user() to dump the thing into
our ringbuffer as part of the event record.

And then there's the callchain code, which first unwinds kernel space if
the 'interrupt'/'event' reg set points into the kernel, and then uses
task_pt_regs() (which I think we agree should be perf_get_regs_user())
to obtain the user regs to continue with the user stack unwind.

Finally there's PERF_SAMPLE_REGS_INTR, which dumps whatever
'interrupt/event' regs we get into the ringbuffer sample record.


Did that help? Or did I confuse you moar?

[toc] | [prev] | [next] | [standalone]

#1384636

On Thu, Apr 21, 2016 at 1:12 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Apr 21, 2016 at 12:39:42PM -0700, Andy Lutomirski wrote:
>> On Wed, Apr 20, 2016 at 12:05 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Wed, Apr 20, 2016 at 08:40:23AM -0700, Andy Lutomirski wrote:
>
>> >> >> Peter, I got lost in the code that calls this.  Are regs coming from
>> >> >> the overflow interrupt's regs, current_pt_regs(), or
>> >> >> perf_get_regs_user?
>> >> >
>> >> > So get_perf_callchain() will get regs from:
>> >> >
>> >> >  - interrupt/NMI regs
>> >> >  - perf_arch_fetch_caller_regs()
>> >> >
>> >> > And when user && !user_mode(), we'll use:
>> >> >
>> >> >  - task_pt_regs() (which arguably should maybe be perf_get_regs_user())
>> >>
>> >> Could you point me to this bit of the code?
>> >
>> > kernel/events/callchain.c:198
>>
>> But that only applies to the callchain code, right?
>
> Yes, which is what I thought you were after..
>
>> AFAICS the PEBS
>> code is invoked through the x86_pmu NMI handler and always gets the
>> IRQ regs.  Except for this case:
>>
>> static inline void intel_pmu_drain_pebs_buffer(void)
>> {
>>     struct pt_regs regs;
>>
>>     x86_pmu.drain_pebs(&regs);
>> }
>>
>> which seems a bit confused.
>
> Yes, so that only gets used with 'large' pebs, which requires no other
> flags than PERF_FRERERUNNING_FLAGS, which precludes the regs set from
> being used.
>
> Could definitely use a comment.
>
>> I don't suppose we could arrange to pass something consistent into the
>> PEBS handlers...
>>
>> Or is the PEBS code being called from the callchain code somehow?
>
> No. I think we were/are slightly talking past one another.
>
>> >> One call to perf_get_user_regs per interrupt shouldn't be too bad --
>> >> certainly much better then one per PEBS record.  One call to get user
>> >> ABI per overflow would be even less bad, but at that point, folding it
>> >> in to the PEBS code wouldn't be so bad either.
>> >
>> > Right; although note that the whole fixup_ip() thing requires a single
>> > record per interrupt (for we need the LBR state for each record in order
>> > to rewind).
>>
>> So do earlier PEBS events not get rewound?  Or so we just program the
>> thing to only ever give us one event at a time?
>
> The latter; we program PEBS such that it can hold but a single record
> and thereby assure we get an interrupt for each record.
>
>> > The problem here is that the overflow stuff is designed for a single
>> > 'event' per interrupt, so passing it data for multiple events is
>> > somewhat icky.
>>
>> It also seems that there's a certain amount of confusion as to exactly
>> what "regs" means in various contexts.  Or at least I'm confused by
>> it.
>
> Yes, there's too much regs.
>
> Typically 'regs' is the 'interrrupt'/'event' regs, that is the register
> set at eventing time. For sampling hardware PMUs this is NMI/IRQ like
> things, for software events this ends up being
> perf_arch_fetch_caller_regs().
>
> Then there's PERF_SAMPLE_REGS_USER|PERF_SAMPLE_STACK_USER, which, for
> each event with it set, use perf_get_regs_user() to dump the thing into
> our ringbuffer as part of the event record.
>
> And then there's the callchain code, which first unwinds kernel space if
> the 'interrupt'/'event' reg set points into the kernel, and then uses
> task_pt_regs() (which I think we agree should be perf_get_regs_user())
> to obtain the user regs to continue with the user stack unwind.
>
> Finally there's PERF_SAMPLE_REGS_INTR, which dumps whatever
> 'interrupt/event' regs we get into the ringbuffer sample record.
>
>
> Did that help? Or did I confuse you moar?
>

I think I'm starting to get it.  What if we rearrange slightly, like this:

perf_sample_data already has a struct perf_regs in it.  We could add a
flags field to the first chunk of perf_sample_data:

u64 sample_flags;

perf_sample_data_init sets sample_flags to zero.

Now we rename perf_sample_regs_user to __perf_sample_regs_user and
make it non-static.  We also teach it to set do data->sample_flags |=
PERF_SAMPLE_FLAGS_HAS_REGS_USER.  We add:

static void perf_fetch_regs_user(struct perf_sample_data *data, struct
pt_regs *interrupt_regs)
{
  if (data->sample_flags & PERF_SAMPLE_FLAGS_HAS_REGS_USER)
    return;

  __perf_sample_regs_user(&data->regs_user, interrupt_regs,
&data->regs_user_copy);
}

(Hmm.  This only really works well if we can guarantee that
interrupt_regs remains valid for the life of the perf_sample_data
object.  Could we perhaps move the interrupt_regs pointer *into*
perf_sample_data and stop passing it all over the place?)

We change all the callers of perf_sample_regs_user to use
perf_fetch_regs_user instead.

Now we teach the PEBS fixup code to call perf_fetch_regs_user if it
sees a user IP.  Then it can use regs_user->abi instead of TIF_IA32,
and my original goal of nuking TIF_IA32 can proceed apace.  (Keep in
mind that, if the interrupt refers to user mode, this is very fast.
If the interrupt refers to kernel mode, it's slower, but that's
comparatively rare and it's the case where we actually care.)

There might be one or two other tweaks needed, but I think this should
mostly do the trick.

What do you think?  If you like it, I can probably find some time to
give it a shot, but I don't guarantee that I won't miss some subtlety
in its interaction with the rest of the event output code.

On a vaguely related note, why is the big prebs-to-pt_regs copy
conditional on (sample_type & PERF_SAMPLE_REGS_INTR)?  I bet it would
be faster to make it unconditional, because you could avoid copying
over the entire pt_regs struct if PERF_SAMPLE_REGS_INTR isn't set.

--Andy


-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1384640

On Thu, Apr 21, 2016 at 4:27 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Apr 21, 2016 at 1:12 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> On Thu, Apr 21, 2016 at 12:39:42PM -0700, Andy Lutomirski wrote:
>>> On Wed, Apr 20, 2016 at 12:05 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>>> > On Wed, Apr 20, 2016 at 08:40:23AM -0700, Andy Lutomirski wrote:
>>
>>> >> >> Peter, I got lost in the code that calls this.  Are regs coming from
>>> >> >> the overflow interrupt's regs, current_pt_regs(), or
>>> >> >> perf_get_regs_user?
>>> >> >
>>> >> > So get_perf_callchain() will get regs from:
>>> >> >
>>> >> >  - interrupt/NMI regs
>>> >> >  - perf_arch_fetch_caller_regs()
>>> >> >
>>> >> > And when user && !user_mode(), we'll use:
>>> >> >
>>> >> >  - task_pt_regs() (which arguably should maybe be perf_get_regs_user())
>>> >>
>>> >> Could you point me to this bit of the code?
>>> >
>>> > kernel/events/callchain.c:198
>>>
>>> But that only applies to the callchain code, right?
>>
>> Yes, which is what I thought you were after..
>>
>>> AFAICS the PEBS
>>> code is invoked through the x86_pmu NMI handler and always gets the
>>> IRQ regs.  Except for this case:
>>>
>>> static inline void intel_pmu_drain_pebs_buffer(void)
>>> {
>>>     struct pt_regs regs;
>>>
>>>     x86_pmu.drain_pebs(&regs);
>>> }
>>>
>>> which seems a bit confused.
>>
>> Yes, so that only gets used with 'large' pebs, which requires no other
>> flags than PERF_FRERERUNNING_FLAGS, which precludes the regs set from
>> being used.
>>
>> Could definitely use a comment.
>>
>>> I don't suppose we could arrange to pass something consistent into the
>>> PEBS handlers...
>>>
>>> Or is the PEBS code being called from the callchain code somehow?
>>
>> No. I think we were/are slightly talking past one another.
>>
>>> >> One call to perf_get_user_regs per interrupt shouldn't be too bad --
>>> >> certainly much better then one per PEBS record.  One call to get user
>>> >> ABI per overflow would be even less bad, but at that point, folding it
>>> >> in to the PEBS code wouldn't be so bad either.
>>> >
>>> > Right; although note that the whole fixup_ip() thing requires a single
>>> > record per interrupt (for we need the LBR state for each record in order
>>> > to rewind).
>>>
>>> So do earlier PEBS events not get rewound?  Or so we just program the
>>> thing to only ever give us one event at a time?
>>
>> The latter; we program PEBS such that it can hold but a single record
>> and thereby assure we get an interrupt for each record.
>>
>>> > The problem here is that the overflow stuff is designed for a single
>>> > 'event' per interrupt, so passing it data for multiple events is
>>> > somewhat icky.
>>>
>>> It also seems that there's a certain amount of confusion as to exactly
>>> what "regs" means in various contexts.  Or at least I'm confused by
>>> it.
>>
>> Yes, there's too much regs.
>>
>> Typically 'regs' is the 'interrrupt'/'event' regs, that is the register
>> set at eventing time. For sampling hardware PMUs this is NMI/IRQ like
>> things, for software events this ends up being
>> perf_arch_fetch_caller_regs().
>>
>> Then there's PERF_SAMPLE_REGS_USER|PERF_SAMPLE_STACK_USER, which, for
>> each event with it set, use perf_get_regs_user() to dump the thing into
>> our ringbuffer as part of the event record.
>>
>> And then there's the callchain code, which first unwinds kernel space if
>> the 'interrupt'/'event' reg set points into the kernel, and then uses
>> task_pt_regs() (which I think we agree should be perf_get_regs_user())
>> to obtain the user regs to continue with the user stack unwind.
>>
>> Finally there's PERF_SAMPLE_REGS_INTR, which dumps whatever
>> 'interrupt/event' regs we get into the ringbuffer sample record.
>>
>>
>> Did that help? Or did I confuse you moar?
>>
>
> I think I'm starting to get it.  What if we rearrange slightly, like this:
>

I started fiddling to see what's involved, then I got to this:

    if (sample_type & PERF_SAMPLE_REGS_INTR) {
        u64 abi = data->regs_intr.abi;
        /*
         * If there are no regs to dump, notice it through
         * first u64 being zero (PERF_SAMPLE_REGS_ABI_NONE).
         */
        perf_output_put(handle, abi);

        if (abi) {
            u64 mask = event->attr.sample_regs_intr;

            perf_output_sample_regs(handle,
                        data->regs_intr.regs,
                        mask);
        }
    }

regs_intr.abi comes from perf_regs_abi(current), which, on x86_64 or
arm64, may indicate 32-bit regs, but the actual regs are always
64-bit.  Am I just confused or is this a bug?

--Andy

[toc] | [prev] | [next] | [standalone]

#1386570

On Thu, Apr 21, 2016 at 04:27:19PM -0700, Andy Lutomirski wrote:
> > Did that help? Or did I confuse you moar?
> >
> 
> I think I'm starting to get it.  What if we rearrange slightly, like this:
> 
> perf_sample_data already has a struct perf_regs in it.  We could add a
> flags field to the first chunk of perf_sample_data:
> 
> u64 sample_flags;

I actually considered that for another problem. Didn't like it then, but
seeing how I still haven't figured out a better way and you're now
proposing this too, maybe...

Part of the problem is that this will completely exhaust that first
cacheline :/

> perf_sample_data_init sets sample_flags to zero.

And while we're on struct perf_sample_data, that thing has gotten
insanely large. We carry it on-stack!

It should be fairly easy to take regs_user_copy out and use a per-cpu
array of them things for this I think, see below.

> Now we rename perf_sample_regs_user to __perf_sample_regs_user and
> make it non-static.  We also teach it to set do data->sample_flags |=
> PERF_SAMPLE_FLAGS_HAS_REGS_USER.  We add:
> 
> static void perf_fetch_regs_user(struct perf_sample_data *data, struct
> pt_regs *interrupt_regs)
> {
>   if (data->sample_flags & PERF_SAMPLE_FLAGS_HAS_REGS_USER)
>     return;
> 
>   __perf_sample_regs_user(&data->regs_user, interrupt_regs,
> &data->regs_user_copy);
> }

I meant to change perf_prepare_sample() to do:

	u64 sample_type = event->attr.sample_type & ~data.sample_type;

or something similar, such that we can override/avoid some of the work
there.

> (Hmm.  This only really works well if we can guarantee that
> interrupt_regs remains valid for the life of the perf_sample_data
> object.  Could we perhaps move the interrupt_regs pointer *into*
> perf_sample_data and stop passing it all over the place?)

So the problem with that is that we'll now overflow the one cacheline,
and the last time I really looked at this that made samples that much
slower.

It might be time to re-evaluate this stuff, since pretty much everything
will eventually write into perf_sample_data::ip etc.. which is the
second line anyway.

Also, looking at it, we actually have a pointer in there for this,
perf_sample_data::regs_intr::regs, but its at the very tail of this
monster, 4 cachelines off the normal path.

> We change all the callers of perf_sample_regs_user to use
> perf_fetch_regs_user instead.

There's only the one site currently, but yeah.

> What do you think?  If you like it, I can probably find some time to
> give it a shot, but I don't guarantee that I won't miss some subtlety
> in its interaction with the rest of the event output code.

Sure give it a go, I'll stomp on it to fix the pebs-time issue (we need
to skip perf_prepare_sample's PERF_SAMPLE_TIME branch for that).

> On a vaguely related note, why is the big prebs-to-pt_regs copy
> conditional on (sample_type & PERF_SAMPLE_REGS_INTR)?  I bet it would
> be faster to make it unconditional, because you could avoid copying
> over the entire pt_regs struct if PERF_SAMPLE_REGS_INTR isn't set.

Hmm, yes.. that code did move about a bit, not sure what it looked like
originally.

In any case, That fully copy is overkill in the simple case as well, I
think that could get away with only copying cs,flags.


Compile tested only..

---
Subject: perf: Replace perf_sample_data::regs_user_copy with per-cpu storage

struct perf_sample_data is immense, and we carry it on stack, shrink it
some.

struct perf_sample_data {
        /* size: 384, cachelines: 6, members: 19 */
}

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/perf_event.h |  2 --
 kernel/events/core.c       | 23 +++++++++++++++++------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 85749ae8cb5f..dd2cab6c5bbb 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -795,8 +795,6 @@ struct perf_sample_data {
 	 * on arch details.
 	 */
 	struct perf_regs		regs_user;
-	struct pt_regs			regs_user_copy;
-
 	struct perf_regs		regs_intr;
 	u64				stack_user_size;
 } ____cacheline_aligned;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index eabeb2aec00f..72754607d2cd 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5146,15 +5146,27 @@ perf_output_sample_regs(struct perf_output_handle *handle,
 	}
 }
 
-static void perf_sample_regs_user(struct perf_regs *regs_user,
-				  struct pt_regs *regs,
-				  struct pt_regs *regs_user_copy)
+static DEFINE_PER_CPU(struct pt_regs, __regs_user[4]);
+
+static struct pt_regs *regs_user_ptr(void)
+{
+	if (in_nmi())
+		return this_cpu_ptr(&__regs_user[0]);
+	if (in_interrupt())
+		return this_cpu_ptr(&__regs_user[1]);
+	if (in_serving_softirq())
+		return this_cpu_ptr(&__regs_user[2]);
+	return this_cpu_ptr(&__regs_user[3]);
+}
+
+static void
+perf_sample_regs_user(struct perf_regs *regs_user, struct pt_regs *regs)
 {
 	if (user_mode(regs)) {
 		regs_user->abi = perf_reg_abi(current);
 		regs_user->regs = regs;
 	} else if (current->mm) {
-		perf_get_regs_user(regs_user, regs, regs_user_copy);
+		perf_get_regs_user(regs_user, regs, regs_user_ptr());
 	} else {
 		regs_user->abi = PERF_SAMPLE_REGS_ABI_NONE;
 		regs_user->regs = NULL;
@@ -5638,8 +5650,7 @@ void perf_prepare_sample(struct perf_event_header *header,
 	}
 
 	if (sample_type & (PERF_SAMPLE_REGS_USER | PERF_SAMPLE_STACK_USER))
-		perf_sample_regs_user(&data->regs_user, regs,
-				      &data->regs_user_copy);
+		perf_sample_regs_user(&data->regs_user, regs);
 
 	if (sample_type & PERF_SAMPLE_REGS_USER) {
 		/* regs dump ABI info */

[toc] | [prev] | [next] | [standalone]

#1386673

On Mon, Apr 25, 2016 at 8:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Apr 21, 2016 at 04:27:19PM -0700, Andy Lutomirski wrote:
>> > Did that help? Or did I confuse you moar?
>> >
>>
>> I think I'm starting to get it.  What if we rearrange slightly, like this:
>>
>> perf_sample_data already has a struct perf_regs in it.  We could add a
>> flags field to the first chunk of perf_sample_data:
>>
>> u64 sample_flags;
>
> I actually considered that for another problem. Didn't like it then, but
> seeing how I still haven't figured out a better way and you're now
> proposing this too, maybe...
>
> Part of the problem is that this will completely exhaust that first
> cacheline :/

What do you mean?  You have a whole 63 bits left :)

Another option would be to initialize regs_user.regs to
PERF_REGS_NOT_YET_FILLED (#defined to ~0 or whatever).  That would
involve a *write* to an otherwise possibly unused cacheline, which is
less than ideal but is probably considerably less bad than reading the
cacheline.

>
>> perf_sample_data_init sets sample_flags to zero.
>
> And while we're on struct perf_sample_data, that thing has gotten
> insanely large. We carry it on-stack!
>
> It should be fairly easy to take regs_user_copy out and use a per-cpu
> array of them things for this I think, see below.
>
>> Now we rename perf_sample_regs_user to __perf_sample_regs_user and
>> make it non-static.  We also teach it to set do data->sample_flags |=
>> PERF_SAMPLE_FLAGS_HAS_REGS_USER.  We add:
>>
>> static void perf_fetch_regs_user(struct perf_sample_data *data, struct
>> pt_regs *interrupt_regs)
>> {
>>   if (data->sample_flags & PERF_SAMPLE_FLAGS_HAS_REGS_USER)
>>     return;
>>
>>   __perf_sample_regs_user(&data->regs_user, interrupt_regs,
>> &data->regs_user_copy);
>> }
>
> I meant to change perf_prepare_sample() to do:
>
>         u64 sample_type = event->attr.sample_type & ~data.sample_type;
>
> or something similar, such that we can override/avoid some of the work
> there.

I'm not sure I follow, but that's okay.

>
>> (Hmm.  This only really works well if we can guarantee that
>> interrupt_regs remains valid for the life of the perf_sample_data
>> object.  Could we perhaps move the interrupt_regs pointer *into*
>> perf_sample_data and stop passing it all over the place?)
>
> So the problem with that is that we'll now overflow the one cacheline,
> and the last time I really looked at this that made samples that much
> slower.
>
> It might be time to re-evaluate this stuff, since pretty much everything
> will eventually write into perf_sample_data::ip etc.. which is the
> second line anyway.
>
> Also, looking at it, we actually have a pointer in there for this,
> perf_sample_data::regs_intr::regs, but its at the very tail of this
> monster, 4 cachelines off the normal path.
>
>> We change all the callers of perf_sample_regs_user to use
>> perf_fetch_regs_user instead.
>
> There's only the one site currently, but yeah.
>
>> What do you think?  If you like it, I can probably find some time to
>> give it a shot, but I don't guarantee that I won't miss some subtlety
>> in its interaction with the rest of the event output code.
>
> Sure give it a go, I'll stomp on it to fix the pebs-time issue (we need
> to skip perf_prepare_sample's PERF_SAMPLE_TIME branch for that).

Will do.  No promises about the time frame -- my queue overfloweth
right now.  But I do have a draft patch or two that I should be able
to dust off a bit over the next few days.

>
>> On a vaguely related note, why is the big prebs-to-pt_regs copy
>> conditional on (sample_type & PERF_SAMPLE_REGS_INTR)?  I bet it would
>> be faster to make it unconditional, because you could avoid copying
>> over the entire pt_regs struct if PERF_SAMPLE_REGS_INTR isn't set.
>
> Hmm, yes.. that code did move about a bit, not sure what it looked like
> originally.
>
> In any case, That fully copy is overkill in the simple case as well, I
> think that could get away with only copying cs,flags.
>

I'd be more comfortable with it if we always either populated all or
none of it or otherwise made sure that unpopulated regs never leaked
out into a sample.

>
> Compile tested only..
>
> ---
> Subject: perf: Replace perf_sample_data::regs_user_copy with per-cpu storage
>
> struct perf_sample_data is immense, and we carry it on stack, shrink it
> some.
>
> struct perf_sample_data {
>         /* size: 384, cachelines: 6, members: 19 */
> }
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/perf_event.h |  2 --
>  kernel/events/core.c       | 23 +++++++++++++++++------
>  2 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 85749ae8cb5f..dd2cab6c5bbb 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -795,8 +795,6 @@ struct perf_sample_data {
>          * on arch details.
>          */
>         struct perf_regs                regs_user;
> -       struct pt_regs                  regs_user_copy;
> -
>         struct perf_regs                regs_intr;
>         u64                             stack_user_size;
>  } ____cacheline_aligned;
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index eabeb2aec00f..72754607d2cd 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5146,15 +5146,27 @@ perf_output_sample_regs(struct perf_output_handle *handle,
>         }
>  }
>
> -static void perf_sample_regs_user(struct perf_regs *regs_user,
> -                                 struct pt_regs *regs,
> -                                 struct pt_regs *regs_user_copy)
> +static DEFINE_PER_CPU(struct pt_regs, __regs_user[4]);
> +
> +static struct pt_regs *regs_user_ptr(void)
> +{
> +       if (in_nmi())
> +               return this_cpu_ptr(&__regs_user[0]);
> +       if (in_interrupt())
> +               return this_cpu_ptr(&__regs_user[1]);
> +       if (in_serving_softirq())
> +               return this_cpu_ptr(&__regs_user[2]);
> +       return this_cpu_ptr(&__regs_user[3]);
> +}
> +

There's already something very similar in
kernel/trace/trace_event_perf.c and core.c
(perf_swevent_get_recursion_context()) that explicitly counts
recursion.  Could they maybe be merged?  I.e. there could just be a
per-cpu pile of pt_regs structs and a simple allocator for them?  E.g.
perf_sample_data_init could increment some counter and
perf_sample_data_free (which doesn't currently exist) could decrement
the counter.

I don't personally mind keeping one of these on the stack -- it's not
*that* big.

But maybe there's a much better solution.  There is only ever one set
of user regs at a time.  If perf events nest, then the user regs are
exactly the same.  I wonder if this means that there could be a single
percpu copy of this mess.  It might not be quite that simple, because
an NMI could hit in the middle of populating the thing, though.
Grumble.

> +static void
> +perf_sample_regs_user(struct perf_regs *regs_user, struct pt_regs *regs)
>  {
>         if (user_mode(regs)) {
>                 regs_user->abi = perf_reg_abi(current);
>                 regs_user->regs = regs;
>         } else if (current->mm) {
> -               perf_get_regs_user(regs_user, regs, regs_user_copy);
> +               perf_get_regs_user(regs_user, regs, regs_user_ptr());
>         } else {
>                 regs_user->abi = PERF_SAMPLE_REGS_ABI_NONE;
>                 regs_user->regs = NULL;
> @@ -5638,8 +5650,7 @@ void perf_prepare_sample(struct perf_event_header *header,
>         }
>
>         if (sample_type & (PERF_SAMPLE_REGS_USER | PERF_SAMPLE_STACK_USER))
> -               perf_sample_regs_user(&data->regs_user, regs,
> -                                     &data->regs_user_copy);
> +               perf_sample_regs_user(&data->regs_user, regs);

The rest looks reasonable.

--Andy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web