lkml.org 
[lkml]   [2016]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 10/18] x86/intel_rdt: Build structures for each resource based on cache topology
    On Fri, 14 Oct 2016, Fenghua Yu wrote:
    >
    > +static int get_cache_id(int cpu, int level)
    > +{
    > + struct cpu_cacheinfo *ci = get_cpu_cacheinfo(cpu);
    > + int i;
    > +
    > + for (i = 0; i < ci->num_leaves; i++)
    > + if (ci->info_list[i].level == level)
    > + return ci->info_list[i].id;

    Multi line statements need curly braces.

    > + return -1;
    > +}
    > +
    > +void rdt_cbm_update(void *arg)
    > +{
    > + struct msr_param *m = (struct msr_param *)arg;
    > + struct rdt_resource *r = m->res;
    > + struct rdt_domain *d;
    > + struct list_head *l;
    > + int i, cpu = smp_processor_id();
    > +
    > + list_for_each(l, &r->domains) {
    > + d = list_entry(l, struct rdt_domain, list);

    We have list_for_each_entry() ....

    > + if (cpumask_test_cpu(cpu, &d->cpu_mask))
    > + goto found;
    > + }
    > + pr_info_once("cpu %d not found in any domain for resource %s\n",
    > + cpu, r->name);

    If the list is empty, then 'd' is not initialized and dereferenced. If the
    list is not empty then you use the last domain in the list. Neither one is
    the right thing to do....

    > +
    > +found:
    > + for (i = m->low; i < m->high; i++)
    > + wrmsrl(r->msr_base + i, d->cbm[i]);
    > +}
    > +
    > +static void update_domain(int cpu, struct rdt_resource *r, int add)
    > +{
    > + struct list_head *l;
    > + struct rdt_domain *d;
    > + int i, cache_id;

    + struct rdt_domain *d;
    + struct list_head *l;
    + int i, cache_id;

    Can you see how this is simpler to read?

    > +
    > + cache_id = get_cache_id(cpu, r->cache_level);

    > + if (cache_id == -1) {
    > + pr_info_once("Could't find cache id for cpu %d\n", cpu);
    > + return;
    > + }

    Please seperate this by a empty line.

    > + list_for_each(l, &r->domains) {
    > + d = list_entry(l, struct rdt_domain, list);

    list_for_each_entry() once more.

    > + if (cache_id == d->id)
    > + goto found;
    > + if (cache_id < d->id)
    > + break;
    > + }
    > + if (!add) {
    > + pr_info_once("removed unknown cpu %d\n", cpu);

    _once? Why should this happen at all?

    > + return;
    > + }
    > + d = kzalloc(sizeof(*d), GFP_KERNEL);

    Shouldn't this be kzalloc_node() ?

    > + if (!d)
    > + return;
    > +
    > + d->id = cache_id;
    > + d->cbm = kmalloc_array(r->max_closid, sizeof(*d->cbm), GFP_KERNEL);
    > + if (!d->cbm) {
    > + pr_info("Failed to alloc CBM array for cpu %d\n", cpu);
    > + kfree(d);
    > + return;
    > + }
    > + cpumask_set_cpu(cpu, &d->cpu_mask);
    > + for (i = 0; i < r->max_closid; i++) {
    > + d->cbm[i] = r->max_cbm;
    > + wrmsrl(r->msr_base + i, d->cbm[i]);
    > + }
    > + list_add_tail(&d->list, l);
    > + r->num_domains++;
    > + return;
    > +
    > +found:
    > + if (add) {
    > + cpumask_set_cpu(cpu, &d->cpu_mask);

    Gah. Reusing this function for add and del is just a silly optimization
    which makes the code harder to read.

    All you share is the find thing. So you can split that out into a helper:

    static struct rdt_domain *rdt_find_domain(struct rdt_resource *r,
    unsigned int cpu, int id)
    {
    if (id < 0)
    return ERR_PTR(id);

    list_for_each_entry(d, &r->domains, list) {
    if (d->id == cache_id)
    return d;
    }
    return NULL;
    }

    and use it for seperate add/del functions.

    static void domain_add_cpu(unsigned int cpu, struct rdt_resource *r)
    {
    int id = get_cache_id(cpu, r->cache_level);
    struct rdt_domain *d;

    d = rdt_find_domain(r, cpu, id);
    if (IS_ERR(d)) {
    pr_warn("Print some useful information\n", r->cache_level, cpu, ....);
    return;
    }

    if (d) {
    cpumask_set_cpu(cpu, &d->cpu_mask);
    return;
    }

    /* Do the allocaction stuff */
    }

    static void domain_remove_cpu(unsigned int cpu, struct rdt_resource *r)
    {
    int id = get_cache_id(cpu, r->cache_level);
    struct rdt_domain *d;

    d = rdt_find_domain(r, cpu, id);
    if (IS_ERR_OR_NULL(d)) {
    pr_warn("Print some useful information\n", r->cache_level, cpu, ...);
    return;
    }

    cpumask_clear_cpu(cpu, &d->cpu_mask);
    ....
    }

    Hmm?

    > static int __init intel_rdt_late_init(void)
    > {
    > struct rdt_resource *r;
    > + int state;
    >
    > if (!get_rdt_resources())
    > return -ENODEV;
    >
    > + state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
    > + "AP_INTEL_RDT_ONLINE",

    Please use: "x86/rdt/cat:online:" or similar. We messed that up in a few
    places when adding the names, but we don't want to add more of this.

    > + intel_rdt_online_cpu, intel_rdt_offline_cpu);

    Thanks,

    tglx

    \
     
     \ /
      Last update: 2016-10-17 16:48    [W:3.658 / U:0.292 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site