Messages in this thread | | | Date | Wed, 11 Jul 2018 06:59:28 +0100 | From | Mark Rutland <> | Subject | Re: [RFC PATCH] perf/core: don't sample kernel regs upon skid |
| |
On Mon, Jul 09, 2018 at 06:42:29PM -0400, Boris Ostrovsky wrote: > On 07/02/2018 12:02 PM, Mark Rutland wrote: > > On Mon, Jul 02, 2018 at 05:46:55PM +0200, Peter Zijlstra wrote: > >> On Mon, Jul 02, 2018 at 04:12:50PM +0100, Mark Rutland wrote: > >>> +static struct pt_regs *perf_get_sample_regs(struct perf_event *event, > >>> + struct pt_regs *regs) > >>> +{ > >>> + /* > >>> + * Due to interrupt latency (AKA "skid"), we may enter the kernel > >>> + * before taking an overflow, even if the PMU is only counting user > >>> + * events. > >>> + * > >>> + * If we're not counting kernel events, always use the user regs when > >>> + * sampling. > >>> + * > >>> + * TODO: what do we do about sampling a guest's registers? The IP is > >>> + * special-cased, but for the rest of the regs they'll get the > >>> + * user/kernel regs depending on whether exclude_kernel is set, which > >>> + * is nonsensical. > >>> + * > >>> + * We can't get at the full set of regs in all cases (e.g. Xen's PV PMU > >>> + * can't provide the GPRs), so should we just zero the GPRs when in a > >>> + * guest? Or skip outputting the regs in perf_output_sample? > >> Seems daft Xen cannot provide registers; why is that? Boris? > > The xen_pmu_regs structure simply doesn't have them, so I assume there's > > no API to get them. > > > > Given we don't currently sample the guest regs, I'd be tempted to just > > zero them for now, or skip the sample at output time (if that doesn't > > break some other case). > > (Was out on vacation, couldn't respond earlier) > > Yes, PV guests only get a limited set of registers passed to the handler > by the hypervisor. GPRs are not part of this set.
Is that also true for Dom0?
> I think we need do > > diff --git a/arch/x86/xen/pmu.c b/arch/x86/xen/pmu.c > index 7d00d4a..95997e6 100644 > --- a/arch/x86/xen/pmu.c > +++ b/arch/x86/xen/pmu.c > @@ -478,7 +478,7 @@ static void xen_convert_regs(const struct > xen_pmu_regs *xen_regs, > irqreturn_t xen_pmu_irq_handler(int irq, void *dev_id) > { > int err, ret = IRQ_NONE; > - struct pt_regs regs; > + struct pt_regs regs = {0}; > const struct xen_pmu_data *xenpmu_data = get_xenpmu_data(); > uint8_t xenpmu_flags = get_xenpmu_flags(); > > > Do you want me to submit a separate patch or can you make this part of > yours?
I think this is going to become a series rather than a single patch, but I can have a go. I need to get my head around how the various cases interact with each other.
Thanks, Mark.
| |