Re: [PATCH v2 2/9] drivers: base: support cpu cache information interface to userspace via sysfs

[Stephen Boyd <sboyd@codeaurora.org>]

On 07/30/14 09:23, Sudeep Holla wrote:
> Hi Stephen,
>
> Thanks for reviewing this.
>
> On 30/07/14 00:09, Stephen Boyd wrote:
>> On 07/25/14 09:44, Sudeep Holla wrote:
>
>>> +
>>> +             shared_cpu_map: logical cpu mask containing the list
>>> of cpus sharing
>>> +                             the cache
>>> +
>>> +             size: the total cache size in kB
>>> +
>>> +             type:
>>> +                     - instruction: cache that only holds instructions
>>> +                     - data: cache that only caches data
>>> +                     - unified: cache that holds both data and
>>> instructions
>>> +
>>> +             ways_of_associativity: degree of freedom in placing a
>>> particular block
>>> +                                     of memory in the cache
>>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>>> new file mode 100644
>>> index 000000000000..983728a919ec
>>> --- /dev/null
>>> +++ b/drivers/base/cacheinfo.c
>>> @@ -0,0 +1,539 @@
>> [...]
>>> +
>>> +static int detect_cache_attributes(unsigned int cpu)
>>
>> Unused if sysfs is disabled? Actually it looks like everything except
>> the weak functions are unused in such a case.
>>
>
>> I see that ia64 has this attributes file, but in that case only two
>> attributes exist (write through and write back) and only one value is
>> ever shown. When we have multiple attributes we'll have multiple lines
>> to parse here. What if we left attributes around for the ia64 case
>> (possibly even hiding that entirely within that architecture specific
>> code) and then have files like "allocation_policy" and "storage_method"
>> that correspond to whether its read/write allocation and write through
>> or write back? The goal being to make only one value exist in any sysfs
>> attribute.
>>
>
> I like your idea, but is it hard rule to have only one value in any
> sysfs attribute ? Though one concern I have is if different cache designs
> make have different features and like to express that, 'attributes' is a
> unified place to do that similar to cpu features in /proc/cpuinfo.

'attributes' seems too generic. Pretty much anything is an attribute.

>
> Anyways if we decide to split it, how about write_policy instead of
> storage_method ?

Sounds good.

>
>>> +     buf[n] = '\0';
>>> +     return n;
>>> +}
>>> +
>>> +static umode_t
>>> +cache_default_attrs_is_visible(struct kobject *kobj,
>>> +                            struct attribute *attr, int unused)
>>> +{
>>> +     struct device *dev = kobj_to_dev(kobj);
>>> +     struct device_attribute *dev_attr;
>>> +     umode_t mode = attr->mode;
>>> +     char *buf;
>>> +
>>> +     dev_attr = container_of(attr, struct device_attribute, attr);
>>> +     buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>>> +     if (!buf)
>>> +             return 0;
>>> +
>>> +     /* create attributes that provides meaningful value */
>>> +     if (dev_attr->show && dev_attr->show(dev, dev_attr, buf) < 0)
>>> +             mode = 0;
>>> +
>>> +     kfree(buf);
>>
>> This is sort of sad. We have to allocate a whole page and call the show
>> function to figure out if the attribute is visible? Why don't we
>> actually look at what the attribute is and check for the structure
>> members we care about? It looks like there are only a few combinations.
>>
>
> Yes I thought about that, as even I didn't like that allocation. But if
> we want the private attributes also use the same is_visible callback, we
> can't check member directly as we don't know the details of the
> individual element.
>
> Even if we have compare elements we need to compare the attribute and
> then the value for each element in the structure, requiring changes if
> elements are added/removed. I am fine either way, just explaining why
> it's done so.

Does any other sysfs attribute group do this? If it was desired I would
think someone else would have done this already, or we wouldn't have
even had an is_visible in the first place as this generic code would
replace it.

>
>
>>> +     case CPU_ONLINE:
>>> +     case CPU_ONLINE_FROZEN:
>>> +             rc = detect_cache_attributes(cpu);
>>> +             if (!rc)
>>> +                     rc = cache_add_dev(cpu);
>>> +             break;
>>> +     case CPU_DEAD:
>>> +     case CPU_DEAD_FROZEN:
>>> +             cache_remove_dev(cpu);
>>> +             if (per_cpu_cacheinfo(cpu))
>>> +                     free_cache_attributes(cpu);
>>> +             break;
>>> +     }
>>> +     return notifier_from_errno(rc);
>>> +}
>>
>> Hm... adding/detecting/destroying this stuff every time a CPU is
>> logically hotplugged seems like a waste of time and energy. Why can't we
>> only do this work when the CPU is actually physically removed? The path
>> for that is via the subsys_interface and it would make it easier on
>> programs that want to learn about cache info as long as the CPU is
>> present in the system even if it isn't online at the time of reading.
>>
>
> I agree, but the main reason I retained it as most of the existing
> architectures implement this way and I didn't want tho change that
> behaviour.

Would anything bad happen if we loosened the behavior so that the
directory is always present as long as the CPU is present? I doubt it.
Seems like a low risk change.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation