Messages in this thread | | | Date | Tue, 05 Aug 2014 19:15:47 +0100 | From | Sudeep Holla <> | Subject | Re: [PATCH v2 2/9] drivers: base: support cpu cache information interface to userspace via sysfs |
| |
Hi Stephen,
On 31/07/14 20:46, Stephen Boyd wrote: > 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 sysfs has dummy implementations, probably I can remove #ifdef
>> >>> 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. >
Yes I agree and hence I compared it to /proc/cpuinfo. As I said I am fine with new single value sysfs, but my main concern is the extendability. If we don't for-see any changes in near future, then we can go with new files as you suggested.
>> >> Anyways if we decide to split it, how about write_policy instead of >> storage_method ? > > Sounds good. >
Thanks.
>> >>>> + 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. >
I saw this first in PPC cacheinfo. Not sure who else have done that.
>> >> >>>> + 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. >
Yes, but before I change, I would like to see people are fine with that. I don't want to move existing implementations into this generic one and cause breakage.
Regards, Sudeep
| |