Messages in this thread | | | Date | Sun, 2 Jul 2017 14:43:56 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 14/21] x86/intel_rdt/cqm: Add mon_data |
| |
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
| |