On Thu, 8 Sep 2016, Fenghua Yu wrote:
> +struct resources {
Darn. The first look made me parse this as a redefinition of 'struct
resource' ... Can't you find an even more generic name for this?
> + struct cache_resource *l3;
> +};
> +
> +static int get_res_type(char **res, enum resource_type *res_type)
> +{
> + char *tok;
> +
> + tok = strsep(res, ":");
> + if (tok == NULL)
We still write: if (!tok) as anywhere else.
> +static int divide_resources(char *buf, char *resources[RESOURCE_NUM])
> +{
> + char *tok;
> + unsigned int resource_num = 0;
> + int ret = 0;
> + char *res;
> + char *res_block;
> + size_t size;
> + enum resource_type res_type;
Sigh.
> +
> + size = strlen(buf) + 1;
> + res = kzalloc(size, GFP_KERNEL);
> + if (!res) {
> + ret = -ENOSPC;
> + goto out;
> + }
> +
> + while ((tok = strsep(&buf, "\n")) != NULL) {
> + if (strlen(tok) == 0)
> + break;
> + if (resource_num++ >= 1) {
How gets that ever greater 1?
> + pr_info("More than one line of resource input!\n");
> + ret = -EINVAL;
> + goto out;
> + }
> + strcpy(res, tok);
> + }
> +
> + res_block = res;
> + ret = get_res_type(&res_block, &res_type);
> + if (ret) {
> + pr_info("Unknown resource type!");
> + goto out;
> + }
> +
> + if (res_block == NULL) {
> + pr_info("Invalid resource value!");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (res_type == RESOURCE_L3 && cat_enabled(CACHE_LEVEL3)) {
> + strcpy(resources[RESOURCE_L3], res_block);
> + } else {
> + pr_info("Invalid resource type!");
> + goto out;
> + }
> +
> + ret = 0;
You surely found the most convoluted solution for this. Whats wrong with:
data = get_res_type(res, &type);
if (IS_ERR(data)) {
ret = PTR_ERR(data);
goto out;
}
ret = 0;
switch (type) {
case RESOURCE_L3:
if (cat_enabled(CACHE_LEVEL3))
strcpy(resources[RESOURCE_L3], data);
break;
default:
ret = -EINVAL;
}
That's too simple to understand and too extensible for future resource
types, right?
> +out:
> + kfree(res);
> + return ret;
> +}
> +static int get_input_cbm(char *tok, struct cache_resource *l,
> + int input_domain_num, int level)
> +{
> + int ret;
> +
> + if (!cdp_enabled) {
> + if (tok == NULL)
> + return -EINVAL;
> +
> + ret = kstrtoul(tok, 16,
> + (unsigned long *)&l->cbm[input_domain_num]);
What is this type cast for? Can't you just parse the data into a local
unsigned long and then store it after validation?
> + if (ret)
> + return ret;
> +
> + if (!cbm_validate(l->cbm[input_domain_num], level))
> + return -EINVAL;
> + } else {
> + char *input_cbm1_str;
> +
> + input_cbm1_str = strsep(&tok, ",");
> + if (input_cbm1_str == NULL || tok == NULL)
> + return -EINVAL;
> +
> + ret = kstrtoul(input_cbm1_str, 16,
> + (unsigned long *)&l->cbm[input_domain_num]);
> + if (ret)
> + return ret;
> +
> + if (!cbm_validate(l->cbm[input_domain_num], level))
> + return -EINVAL;
> +
> + ret = kstrtoul(tok, 16,
> + (unsigned long *)&l->cbm2[input_domain_num]);
> + if (ret)
> + return ret;
> +
> + if (!cbm_validate(l->cbm2[input_domain_num], level))
> + return -EINVAL;
So you have 3 copies of the same sequence now. At other places you split
out the most tiniest stuff into a gazillion of helper functions ...
Just create a parser helper and call it for any of those types. So the
whole thing boils down to:
static int parse_cbm_token(char *tok, u64 *cbm, int level)
{
unsigned long data;
int ret;
ret = kstrtoul(tok, 16, &data);
if (ret)
return ret;
if (!cbm_validate(data, level))
return -EINVAL;
*cbm = data;
return 0;
}
static int parse_cbm(char *buf, struct cache_resource *cr, int domain,
int level)
{
char *cbm1 = buf;
int ret;
if (cdp_enabled)
cbm1 = strsep(&buf, ',');
if (!cbm1 || !buf)
return -EINVAL;
ret = parse_cbm_token(cbm1, &cr->cbm[domain]);
if (ret)
return ret;
if (cdp_enmabled)
return parse_cbm_token(buf, &cr->cbm2[domain]);
return 0;
}
Copy and paste is simpler than thinking, but the result is uglier and
harder to read.
> +static int get_cache_schema(char *buf, struct cache_resource *l, int level,
> + struct rdtgroup *rdtgrp)
> +{
> + char *tok, *tok_cache_id;
> + int ret;
> + int domain_num;
> + int input_domain_num;
> + int len;
> + unsigned int input_cache_id;
> + unsigned int cid;
> + unsigned int leaf;
> +
> + if (!cat_enabled(level) && strcmp(buf, ";")) {
> + pr_info("Disabled resource should have empty schema\n");
So an empty schema is a string which is != ";". Very interesting.
This enabled check here wants more thoughts. If a resource is disabled,
then the input line should be simply ignored. Otherwise you need to rewrite
scripts, config files just because you disabled a particular resource.
> + return -EINVAL;
> + }
> +}
> +
> +enum {
> + CURRENT_CLOSID,
> + REUSED_OWN_CLOSID,
> + REUSED_OTHER_CLOSID,
> + NEW_CLOSID,
> +};
Another random enum in the middle of the code and at a place where it is
completely disjunct from its usage.
> +
> +/*
> + * Check if the reference counts are all ones in rdtgrp's domain.
> + */
> +static bool one_refcnt(struct rdtgroup *rdtgrp, int domain)
A really self explaining function name - NOT!
> +/*
> + * Go through all shared domains. Check if there is an existing closid
> + * in all rdtgroups that matches l3 cbms in the shared
> + * domain. If find one, reuse the closid. Otherwise, allocate a new one.
> + */
> +static int get_rdtgroup_resources(struct resources *resources_set,
> + struct rdtgroup *rdtgrp)
> +{
> + struct cache_resource *l3;
> + bool l3_cbm_found;
> + struct list_head *l;
> + struct rdtgroup *r;
> + u64 cbm;
> + int rdt_closid[MAX_CACHE_DOMAINS];
> + int rdt_closid_type[MAX_CACHE_DOMAINS];
Have you ever checked what the stack foot print of this whole callchain is?
One of the callers has already a char array[1024] on the stack.....
> + int domain;
> + int closid;
> + int ret;
> +
> + l3 = resources_set->l3;
> + memcpy(rdt_closid, rdtgrp->resource.closid,
> + shared_domain_num * sizeof(int));
Can you please seperate stuff with new lines ocassionally to make it
readable?
> + for (domain = 0; domain < shared_domain_num; domain++) {
> + if (rdtgrp->resource.valid) {
> + /*
> + * If current rdtgrp is the only user of cbms in
> + * this domain, will replace the cbms with the input
> + * cbms and reuse its own closid.
> + */
> + if (one_refcnt(rdtgrp, domain)) {
> + closid = rdtgrp->resource.closid[domain];
> + rdt_closid[domain] = closid;
> + rdt_closid_type[domain] = REUSED_OWN_CLOSID;
> + continue;
> + }
> +
> + l3_cbm_found = true;
> +
> + if (cat_l3_enabled)
> + l3_cbm_found = cbm_found(l3, rdtgrp, domain,
> + CACHE_LEVEL3);
> +
> + /*
> + * If the cbms in this shared domain are already
> + * existing in current rdtgrp, record the closid
> + * and its type.
> + */
> + if (l3_cbm_found) {
> + closid = rdtgrp->resource.closid[domain];
> + rdt_closid[domain] = closid;
> + rdt_closid_type[domain] = CURRENT_CLOSID;
> + continue;
> + }
This is unreadable once more.
if (find_cbm(l3, rdtgrp, domain, CACHE_LEVEL3) {
closid = rdtgrp->resource.closid[domain];
rdt_closid[domain] = closid;
rdt_closid_type[domain] = CURRENT_CLOSID;
continue;
}
That requires that find_cbm() - which is a way more intuitive name than
cbm_found() - returns false when cat_l3_enabled is false. Which is trivial
and obvious ...
> + }
> +
> + /*
> + * If the cbms are not found in this rdtgrp, search other
> + * rdtgroups and see if there are matched cbms.
> + */
> + l3_cbm_found = cat_l3_enabled ? false : true;
What the heck?
l3_cbm_found = !cat_l3_enabled;
Is too simple obviously.
Aside of that silly conditional: If cat_l3_enables is false then
l3_cbm_found is true.
> + list_for_each(l, &rdtgroup_lists) {
> + r = list_entry(l, struct rdtgroup, rdtgroup_list);
> + if (r == rdtgrp || !r->resource.valid)
> + continue;
> +
> + if (cat_l3_enabled)
> + l3_cbm_found = cbm_found(l3, r, domain,
> + CACHE_LEVEL3);
And because this path is never taken when cat_l3_enabled is false.
> +
> + if (l3_cbm_found) {
We happily get the closid for something which is not enabled at all. What
is the logic here? I can't find any in this convoluted mess.
> + /* Get the closid that matches l3 cbms.*/
> + closid = r->resource.closid[domain];
> + rdt_closid[domain] = closid;
> + rdt_closid_type[domain] = REUSED_OTHER_CLOSID;
> + break;
> + }
> + }
> + if (!l3_cbm_found) {
> + /*
> + * If no existing closid is found, allocate
> + * a new one.
> + */
> + ret = closid_alloc(&closid, domain);
> + if (ret)
> + goto err;
> + rdt_closid[domain] = closid;
> + rdt_closid_type[domain] = NEW_CLOSID;
> + }
> + }
I really don't want to imagine how this might look like when you add L2
support and if you have code doing this please hide it in the poison
cabinet forever,
> + /*
> + * Now all closid are ready in rdt_closid. Update rdtgrp's closid.
> + */
> + for_each_cache_domain(domain, 0, shared_domain_num) {
> + /*
> + * Nothing is changed if the same closid and same cbms were
> + * found in this rdtgrp's domain.
> + */
> + if (rdt_closid_type[domain] == CURRENT_CLOSID)
> + continue;
> +
> + /*
> + * Put rdtgroup closid. No need to put the closid if we
> + * just change cbms and keep the closid (REUSED_OWN_CLOSID).
> + */
> + if (rdtgrp->resource.valid &&
> + rdt_closid_type[domain] != REUSED_OWN_CLOSID) {
> + /* Put old closid in this rdtgrp's domain if valid. */
> + closid = rdtgrp->resource.closid[domain];
> + closid_put(closid, domain);
> + }
> +
> + /*
> + * Replace the closid in this rdtgrp's domain with saved
> + * closid that was newly allocted (NEW_CLOSID), or found in
> + * another rdtgroup's domains (REUSED_CLOSID), or found in
> + * this rdtgrp (REUSED_OWN_CLOSID).
> + */
> + closid = rdt_closid[domain];
> + rdtgrp->resource.closid[domain] = closid;
> +
> + /*
> + * Get the reused other rdtgroup's closid. No need to get the
> + * closid newly allocated (NEW_CLOSID) because it's been
> + * already got in closid_alloc(). And no need to get the closid
> + * for resued own closid (REUSED_OWN_CLOSID).
> + */
> + if (rdt_closid_type[domain] == REUSED_OTHER_CLOSID)
> + closid_get(closid, domain);
> +
> + /*
> + * If the closid comes from a newly allocated closid
> + * (NEW_CLOSID), or found in this rdtgrp (REUSED_OWN_CLOSID),
> + * cbms for this closid will be updated in MSRs.
> + */
> + if (rdt_closid_type[domain] == NEW_CLOSID ||
> + rdt_closid_type[domain] == REUSED_OWN_CLOSID) {
> + /*
> + * Update cbm in cctable with the newly allocated
> + * closid.
> + */
> + if (cat_l3_enabled) {
> + int cpu;
> + struct cpumask *mask;
> + int dindex;
> + int l3_domain = shared_domain[domain].l3_domain;
> + int leaf = level_to_leaf(CACHE_LEVEL3);
> +
> + cbm = l3->cbm[l3_domain];
> + dindex = get_dcbm_table_index(closid);
> + l3_cctable[l3_domain][dindex].cbm = cbm;
> + if (cdp_enabled) {
> + int iindex;
> +
> + cbm = l3->cbm2[l3_domain];
> + iindex = get_icbm_table_index(closid);
> + l3_cctable[l3_domain][iindex].cbm = cbm;
> + }
> +
> + mask =
> + &cache_domains[leaf].shared_cpu_map[l3_domain];
> +
> + cpu = cpumask_first(mask);
> + smp_call_function_single(cpu, cbm_update_l3_msr,
> + &closid, 1);
Again, why don't ypu split that out into a seperate function instead of
having the forth indentation level and random line breaks?
> +static void init_cache_resource(struct cache_resource *l)
> +{
> + l->cbm = NULL;
> + l->cbm2 = NULL;
> + l->closid = NULL;
> + l->refcnt = NULL;
memset ?
> +}
> +
> +static void free_cache_resource(struct cache_resource *l)
> +{
> + kfree(l->cbm);
> + kfree(l->cbm2);
> + kfree(l->closid);
> + kfree(l->refcnt);
> +}
> +
> +static int alloc_cache_resource(struct cache_resource *l, int level)
> +{
> + int domain_num = get_domain_num(level);
> +
> + l->cbm = kcalloc(domain_num, sizeof(*l->cbm), GFP_KERNEL);
> + l->cbm2 = kcalloc(domain_num, sizeof(*l->cbm2), GFP_KERNEL);
> + l->closid = kcalloc(domain_num, sizeof(*l->closid), GFP_KERNEL);
> + l->refcnt = kcalloc(domain_num, sizeof(*l->refcnt), GFP_KERNEL);
> + if (l->cbm && l->cbm2 && l->closid && l->refcnt)
> + return 0;
> +
> + return -ENOMEM;
> +}
> +
> +/*
> + * This function digests schemata given in text buf. If the schemata are in
> + * right format and there is enough closid, input the schemata in rdtgrp
> + * and update resource cctables.
> + *
> + * Inputs:
> + * buf: string buffer containing schemata
> + * rdtgrp: current rdtgroup holding schemata.
> + *
> + * Return:
> + * 0 on success or error code.
> + */
> +static int get_resources(char *buf, struct rdtgroup *rdtgrp)
> +{
> + char *resources[RESOURCE_NUM];
> + struct cache_resource l3;
> + struct resources resources_set;
> + int ret;
> + char *resources_block;
> + int i;
> + int size = strlen(buf) + 1;
> +
> + resources_block = kcalloc(RESOURCE_NUM, size, GFP_KERNEL);
> + if (!resources_block)
> + return -ENOMEM;
> +
> + for (i = 0; i < RESOURCE_NUM; i++)
> + resources[i] = (char *)(resources_block + i * size);
This is a recurring scheme in your code. Allocating a runtime sized array
and initializing pointers.
Darn, instead of open coding this in several places can't you just make a
single function which does exactly that?
> + ret = divide_resources(buf, resources);
> + if (ret) {
> + kfree(resources_block);
> + return -EINVAL;
> + }
> +
> + init_cache_resource(&l3);
> +
> + if (cat_l3_enabled) {
> + ret = alloc_cache_resource(&l3, CACHE_LEVEL3);
> + if (ret)
> + goto out;
> +
> + ret = get_cache_schema(resources[RESOURCE_L3], &l3,
> + CACHE_LEVEL3, rdtgrp);
> + if (ret)
> + goto out;
> +
> + resources_set.l3 = &l3;
> + } else
> + resources_set.l3 = NULL;
> +
> + ret = get_rdtgroup_resources(&resources_set, rdtgrp);
> +
> +out:
> + kfree(resources_block);
> + free_cache_resource(&l3);
> +
> + return ret;
> +}
> +
> +static void gen_cache_prefix(char *buf, int level)
> +{
> + sprintf(buf, "L%1d:", level == CACHE_LEVEL3 ? 3 : 2);
> +}
> +
> +static int get_cache_id(int domain, int level)
> +{
> + return cache_domains[level_to_leaf(level)].shared_cache_id[domain];
> +}
> +
> +static void gen_cache_buf(char *buf, int level)
> +{
> + int domain;
> + char buf1[32];
> + int domain_num;
> + u64 val;
> +
> + gen_cache_prefix(buf, level);
> +
> + domain_num = get_domain_num(level);
> +
> + val = max_cbm(level);
> +
> + for (domain = 0; domain < domain_num; domain++) {
> + sprintf(buf1, "%d=%lx", get_cache_id(domain, level),
> + (unsigned long)val);
> + strcat(buf, buf1);
WTF?
char *p = buf;
p += sprintf(p, "....", ...);
p += sprintf(p, "....", ...);
p += sprintf(p, "....", ...);
Solves the same problem as this local buffer on the stack plus strcat().
> +/*
> + * Set up default schemata in a rdtgroup. All schemata in all resources are
> + * default values (all 1's) for all domains.
> + *
> + * Input: rdtgroup.
> + * Return: 0: successful
> + * non-0: error code
> + */
> +int get_default_resources(struct rdtgroup *rdtgrp)
> +{
> + char schema[1024];
And that number is pulled out of thin air or what?
> + int ret = 0;
> +
> + if (cat_enabled(CACHE_LEVEL3)) {
> + gen_cache_buf(schema, CACHE_LEVEL3);
> +
> + if (strlen(schema)) {
> + ret = get_resources(schema, rdtgrp);
> + if (ret)
> + return ret;
> + }
> + gen_cache_buf(rdtgrp->schema, CACHE_LEVEL3);
> + }
> +
> + return ret;
> +}
> +
> +ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> + char *buf, size_t nbytes, loff_t off)
> +{
> + int ret = 0;
> + struct rdtgroup *rdtgrp;
> + char *schema;
> +
> + rdtgrp = rdtgroup_kn_lock_live(of->kn);
> + if (!rdtgrp)
> + return -ENODEV;
> +
> + schema = kzalloc(sizeof(char) * strlen(buf) + 1, GFP_KERNEL);
> + if (!schema) {
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + memcpy(schema, buf, strlen(buf) + 1);
Open coding kstrdup() is indeed useful. and reevaluating strlen(buf) three
times in the same function is even more useful.
> +
> + ret = get_resources(buf, rdtgrp);
> + if (ret)
> + goto out;
> +
> + memcpy(rdtgrp->schema, schema, strlen(schema) + 1);
IIRC then the kernel has even strcpy() and strncpy for that matter.
Btw, what makes sure that strlen(schema) is < 1023 ?????
Thanks,
tglx
