Re: [libvirt] [PATCH v4 2/4] perf: introduce a static attr table and refactor virPerfEventEnable() for general purpose

Need to keep those commit messages a lot shorter... On 07/16/2016 04:15 AM, Qiaowei Ren wrote: Another patch which could use some splitting... Rather than go back and forth more on this, I'll generate a sequence of patches based on your code with some tweaks - most of which I'll suggest throughout here. If you git am those patches and git diff them to your current patches you can see the differences... But I'd also expect you could test and validate my adjustments work... Avoid forward decls... I think what would be best to do is move virPerfNew and virPerfFree to the bottom of the function after the #endif for "if defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)" That would be it's own separate patch... Then we can start modifying the code to add the new call... In my changes virPerfNew is at the bottom now so we won't need the forward decl. This should return a value, as follows: if (virPerfRdtAttrInit() < 0) virResetLastError(); For the #else code, we just return 0 - so it's a no-op. When the virPerfEventEnable is called an error is returned. The "real" code inside the #if getting a failure would need to be made "as failure proof" as the existing code which would have returned a failure from virPerfEventEnable and for the most part ignore it. Once we call virPerfEventEnable it'll find the attrType == 0 for the RDT events and cause a failure. Alternatively, we could fail virPerfNew if AttrInit fails, but that's a bit different than the existing code. static void virPerfRdtAttrInit(void) is the normal way of doing this. However, this is not a void function... If there's any errors here, then we're not failing and returning any error... This will go to error with some error message set, but then returns a void status. This goes to error with some error message set, but then returns a void status. Currently unnecessary since everything falls through into the error: label... Is this hunk no longer necessary? Also, existing "efields.cmt.scale" is an "int" while "scale" is an unsigned int. Can be read directly too: if (virStrToLong_i(buf, NULL, 10, &event->efields.cmt.scale) < 0) { So I assume I can make the 'assumption' that all 3 types will have the same attr_type, so rather than reading for each loop, you're reading just once... In any case, the for loop is unecessary - especially considering future patches where have more events. Just set the 3 attrType's directly: attrs[VIR_PERF_EVENT_CMT].attrType = event_type; attrs[VIR_PERF_EVENT_MBMT].attrType = event_type; attrs[VIR_PERF_EVENT_MBML].attrType = event_type; And we fall into the error label! So, it's more of a cleanup label... > + > + error: > + VIR_FREE(buf); > +} > + > +static virPerfEventPtr > +virPerfGetEvent(virPerfPtr perf, > + virPerfEventType type) > +{ > + if (!perf) > + return NULL; > + > + if (type >= VIR_PERF_EVENT_LAST) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Event '%d' is not supported"), > + type); > + return NULL; > } > > - event->fd = syscall(__NR_perf_event_open, &rdt_attr, pid, -1, -1, 0); > + return perf->events + type; > +} > + > +int > +virPerfEventEnable(virPerfPtr perf, > + virPerfEventType type, > + pid_t pid) > +{ > + struct perf_event_attr attr; > + virPerfEventPtr event = virPerfGetEvent(perf, type); > + virPerfEventAttrPtr event_attr = virPerfGetEventAttr(type); > + if (event == NULL || event_attr == NULL) > + return -1; > + > + if (event_attr->attrType == 0 && (type == VIR_PERF_EVENT_CMT || > + type == VIR_PERF_EVENT_MBMT || > + type == VIR_PERF_EVENT_MBML)) { > + virReportSystemError(errno, > + _("Unable to open perf event for %s"), > + virPerfEventTypeToString(event->type)); > + return -1; > + This is where the scale code should be put back: if (type == VIR_PERF_EVENT_CMT) { if (virFileReadAll("/sys/devices/intel_cqm/events/llc_occupancy.scale", 10, &buf) < 0) goto error; if (virStrToLong_i(buf, NULL, 10, &event->efields.cmt.scale) < 0) { virReportSystemError(errno, "%s", _("failed to get cmt scaling factor")); goto error; } } This should do nothing, in fact return 0... Subsequent callers will cause the failure... Otherwise, we set an error message without using it. So just have a "return 0;" Since nothing here affects virperf.c, it should be separate. I've take some liberties to adjust things slightly as you'll see John