Date: Wed, 20 Apr 2016 13:15:56 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Andy Lutomirski <luto@amacapital.net>
Cc: Dmitry Safonov <dsafonov@virtuozzo.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, X86 ML <x86@kernel.org>,
        Robert Richter <rric@kernel.org>, oprofile-list@lists.sf.net,
        Dmitry Safonov <0x7f454c46@gmail.com>
Subject: Re: [PATCH 1/4] x86/events: down with test_thread_flag(TIF_IA32)
Message-ID: <20160420111556.GZ3408@twins.programming.kicks-ass.net>
References: <1460657414-12530-1-git-send-email-dsafonov@virtuozzo.com>
 <1460657414-12530-2-git-send-email-dsafonov@virtuozzo.com>
 <CALCETrUJDdrH_brFuMGxEKcDUZbmxSevRjHNSCg-R_9vZ47kVg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALCETrUJDdrH_brFuMGxEKcDUZbmxSevRjHNSCg-R_9vZ47kVg@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2012-12-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2012
Lines: 51

On Thu, Apr 14, 2016 at 11:32:07AM -0700, Andy Lutomirski wrote:
> On Thu, Apr 14, 2016 at 11:10 AM, Dmitry Safonov <dsafonov@virtuozzo.com> wrote:
> > We can use user_64bit_mode(regs) here instead of thread flag
> > because we have full register frame.
> >
> > Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
> > ---
> >  arch/x86/events/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > index 041e442a3e28..91d101a9a6e9 100644
> > --- a/arch/x86/events/core.c
> > +++ b/arch/x86/events/core.c
> > @@ -2269,7 +2269,7 @@ perf_callchain_user32(struct pt_regs *regs, struct perf_callchain_entry *entry)
> >         struct stack_frame_ia32 frame;
> >         const void __user *fp;
> >
> > -       if (!test_thread_flag(TIF_IA32))
> > +       if (user_64bit_mode(regs))
> >                 return 0;

Urgh, so why didn't I get Cc'ed to these patches to begin with?

> 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())

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.