On Mon, 26 Jun 2017, Vikas Shivappa wrote: > Add a mon_data directory for the root rdtgroup and all other rdtgroups. > The directory holds all of the monitored data for all domains and events > of all resources being monitored.
Again. This does two things at once. Move the existing code to a new file and add the monitoring stuff. Please split it apart. > +static bool __mon_event_count(u32 rmid, struct rmid_read *rr) > +{ > + u64 tval; > + > + tval = __rmid_read(rmid, rr->evtid); > + if (tval & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL)) { > + rr->val = tval; > + return false; > + } > + switch (rr->evtid) { > + case QOS_L3_OCCUP_EVENT_ID: > + rr->val += tval; > + return true; > + default: > + return false; I have no idea what that return code means. > + } > +} > + > +void mon_event_count(void *info) Some explanation why this is a void pointer and how that function is called (I assume it's via IPI) would be appreciated. > +{ > + struct rdtgroup *rdtgrp, *entry; > + struct rmid_read *rr = info; > + struct list_head *llist; *head; > + > + rdtgrp = rr->rgrp; > + > + if (!__mon_event_count(rdtgrp->rmid, rr)) > + return; > + > + /* > + * For Ctrl groups read data from child monitor groups. > + */ > + llist = &rdtgrp->crdtgrp_list; > + > + if (rdtgrp->type == RDTCTRL_GROUP) { > + list_for_each_entry(entry, llist, crdtgrp_list) { > + if (!__mon_event_count(entry->rmid, rr)) > + return; > + } > + } > +} > +static int get_rdt_resourceid(struct rdt_resource *r) > +{ > + if (r > (rdt_resources_all + RDT_NUM_RESOURCES - 1) || > + r < rdt_resources_all || > + ((r - rdt_resources_all) % sizeof(struct rdt_resource))) > + return -EINVAL; If that ever happens, then you have other problems than a wrong pointer. > + > + return ((r - rdt_resources_all) / sizeof(struct rdt_resource)); Moo. Can't you simply put an index field into struct rdt_resource, intialize it with the resource ID and use that? Thanks, tglx