Date:   Wed, 26 Oct 2016 16:06:27 +0000
From:   "Luck, Tony" <tony.luck@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     "Yu, Fenghua" <fenghua.yu@...el.com>,
        "Anvin, H Peter" <h.peter.anvin@...el.com>,
        Ingo Molnar <mingo@...e.hu>,
        Peter Zijlstra <peterz@...radead.org>,
        Stephane Eranian <eranian@...gle.com>,
        "Borislav Petkov" <bp@...e.de>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        Nilay Vaish <nilayvaish@...il.com>, Shaohua Li <shli@...com>,
        David Carrillo-Cisneros <davidcc@...gle.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "Prakhya, Sai Praneeth" <sai.praneeth.prakhya@...el.com>,
        Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v5 10/18] x86/intel_rdt: Build structures for each
 resource based on cache topology

Order is visible to users when we print entries in the schemata file, and validate input that they write (we require that they provide all masks in the same order as this list).

If we hot remove a socket, it disappears from the list, and from the schemata file. When we put in a replacement it reappears. I didn't want the user to see:
L3:0=fffff;2=fffff;3=fffff;1=fffff
after hot replace socket 1, hence the sort.

Sent from my iPhone

> On Oct 26, 2016, at 06:05, Thomas Gleixner <tglx@...utronix.de> wrote:
> 
>> On Sat, 22 Oct 2016, Fenghua Yu wrote:
>> +void rdt_cbm_update(void *arg)
>> +{
>> +    struct msr_param *m = (struct msr_param *)arg;
>> +    struct rdt_resource *r = m->res;
>> +    int i, cpu = smp_processor_id();
>> +    struct rdt_domain *d;
>> +
>> +    list_for_each_entry(d, &r->domains, list) {
> 
>> +static struct rdt_domain *rdt_find_domain(struct rdt_resource *r, int id,
>> +                      struct list_head **pos)
>> +{
>> +    struct rdt_domain *d;
>> +    struct list_head *l;
>> +
>> +    if (id < 0)
>> +        return ERR_PTR(id);
>> +
>> +    list_for_each(l, &r->domains) {
>> +        d = list_entry(l, struct rdt_domain, list);
> 
> So above you converted to list_for_each_entry(). Is there a sensible
> reason, aside of being sloppy, why is this still using list_for_each()?
> 
>> +        /* When id is found, return its domain. */
>> +        if (id == d->id)
>> +            return d;
>> +        /* Stop searching when finding id's position in sorted list. */
> 
> What is the reason that this needs to be in a sorted list?
> 
> I haven't found one so far. And if there is none, then this can use a hlist.
> 
>> +        if (id < d->id)
>> +            break;
>> +    }
>> +    /*
>> +     * No id is found in resource domains. Record the position
>> +     * that the new domain will be added. The posistion is not used
>> +     * when removing a domain.
> 
> This comment makes no sense. If you want to document that a caller does not
> require the @pos argument, then you really should make it optional and do
> 
>    if (pos)
>        *pos = l;
> 
> But before doing that blindly, you want to explain why sorting is required
> at all.
> 
>> +     */
>> +    *pos = l;
>> +
>> +    return NULL;
>> +}
>> +
>> +static void domain_add_cpu(int cpu, struct rdt_resource *r)
>> +{
>> +    int i, id = get_cache_id(cpu, r->cache_level);
>> +    struct list_head *add_pos = NULL;
>> +    struct rdt_domain *d;
>> +
>> +    d = rdt_find_domain(r, id, &add_pos);
>> +    if (IS_ERR(d)) {
>> +        pr_warn("Could't find cache id for cpu %d\n", cpu);
>> +        return;
>> +    }
>> +
>> +    if (d) {
>> +        cpumask_set_cpu(cpu, &d->cpu_mask);
>> +        return;
>> +    }
>> +
>> +    if (!add_pos) {
>> +        pr_warn("Couldn't add cpu %d in %s domain\n", cpu, r->name);
> 
> Errm, how can add_pos ever be NULL if you get here? Not at all AFAICT.
> 
>> +        return;
>> +    }
>> +
>> +    d = kzalloc_node(sizeof(*d), GFP_KERNEL, cpu_to_node(cpu));
>> +    if (!d)
>> +        return;
>> +
>> +    d->id = id;
> 
> Please move this after the allocation. This random code ordering just makes
> reading hard as one expects that d->id is a prerequisite for the
> allocation.
> 
>> +    d->cbm = kmalloc_array(r->num_closid, sizeof(*d->cbm), GFP_KERNEL);
>> +    if (!d->cbm) {
>> +        pr_warn("Failed to alloc CBM array for cpu %d\n", cpu);
>> +        kfree(d);
>> +        return;
>> +    }
> 
> New line please. Visually seperating logical code blocks enhances
> readability.
> 
>> +    for (i = 0; i < r->num_closid; i++) {
> 
> Thanks,
> 
>    tglx

Powered by blists - more mailing lists