Marc Zyngier <marc.zyngier@xxxxxxx> writes: > On 18/01/17 13:01, Punit Agrawal wrote: >> Mark Rutland <mark.rutland@xxxxxxx> writes: >> >>> On Wed, Jan 18, 2017 at 11:21:21AM +0000, Marc Zyngier wrote: >>>> On 10/01/17 11:38, Punit Agrawal wrote: >>>>> +#define VM_MASK GENMASK_ULL(31, 0) >>>>> +#define EVENT_MASK GENMASK_ULL(32, 39) >>>>> +#define EVENT_SHIFT (32) >>>>> + >>>>> +#define to_pid(cfg) ((cfg) & VM_MASK) >>>>> +#define to_event(cfg) (((cfg) & EVENT_MASK) >> EVENT_SHIFT) >>>>> + >>>>> +PMU_FORMAT_ATTR(vm, "config:0-31"); >>>>> +PMU_FORMAT_ATTR(event, "config:32-39"); >>>> >>>> I'm a bit confused by these. Can't you get the PID of the VM you're >>>> tracing directly from perf, without having to encode things? >> >> With perf attached to a PID, the event gets scheduled out when the task >> is context switched. As the PID of the controlling process was used, >> none of the vCPU events were counted. >> >>> And if you >>>> can't, surely this should be a function of the size of pid_t? >> >> Agreed. I'll update above if we decide to carry on with this >> approach. More below... >> >>>> >>>> Mark, can you shine some light here? >>> >>> AFAICT, this is not necessary. >>> >>> The perf_event_open() syscall takes a PID separately from the >>> perf_event_attr. i.e. we should be able to do: >>> >>> // monitor a particular vCPU >>> perf_event_open(attr, vcpupid, -1, -1, 0) >>> >>> ... or .. >>> >>> // monitor a particular vCPU on a pCPU >>> perf_event_open(attr, vcpupid, cpu, -1, 0) >>> >>> ... or ... >>> >>> // monitor all vCPUs on a pCPU >>> perf_event_open(attr, -1, cpu, -1, 0) >>> >>> ... so this shouldn't be necessary. AFAICT, this is a SW PMU, so there >>> should be no issue with using the perf_sw_context. >> >> I might have missed it but none of the modes of invoking perf_event_open >> allow monitoring a set of process, i.e., all vcpus belonging to a >> particular VM, which was one of the aims and a feature I was carrying >> over from the previous version. If we do not care about this... >> >>> >>> If this is a bodge to avoid opening a perf_event per vCPU thread, then I >>> completely disagree with the approach. This would be better handled in >>> userspace by discovering the set of threads and opening events for >>> each. >> >> ... then requiring userspace to invoke perf_event_open perf vCPU will >> simplify this patch. >> >> Marc, any objections? > > Not so far, but I'm curious to find out how you determine which thread > is a vcpu, let alone a given vcpu. I should've clarified in my reply that I wasn't looking to support the third instance from Mark's examples above - "monitor all vCPUs on a pCPU". I think it'll be quite expensive to figure out which threads from a given pool are vCPUs. For the other instances, we only need to find the vCPU for a given pid. Userspace will hand us a pid that needs to be checked against vCPUs to establish that it is a valid vCPU pid (here I was looking to use kvm_vcpu->pid and kvm->pid introduced in Patch 2). This will happen when setting up the event and the vCPU can be cached for later use. > > Thanks, > > M. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
