From: Stephen Boyd <sboyd@codeaurora.org>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
Heiko Carstens <heiko.carstens@de.ibm.com>,
Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>
Subject: Re: [PATCH v2 2/9] drivers: base: support cpu cache information interface to userspace via sysfs
Date: Thu, 31 Jul 2014 12:46:11 -0700 [thread overview]
Message-ID: <53DA9D03.60606@codeaurora.org> (raw)
In-Reply-To: <53D91BE8.8050008@arm.com>
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
next prev parent reply other threads:[~2014-07-31 19:46 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-25 17:30 [PATCH 0/9] drivers: cacheinfo support Sudeep Holla
2014-06-25 17:30 ` [PATCH 1/9] drivers: base: add new class "cpu" to group cpu devices Sudeep Holla
2014-06-25 17:30 ` [PATCH 2/9] drivers: base: support cpu cache information interface to userspace via sysfs Sudeep Holla
2014-06-25 22:23 ` Russell King - ARM Linux
2014-06-26 18:41 ` Sudeep Holla
2014-06-26 18:50 ` Russell King - ARM Linux
2014-06-26 19:03 ` Sudeep Holla
2014-07-10 0:09 ` Greg Kroah-Hartman
2014-07-10 13:37 ` Sudeep Holla
2014-06-25 17:30 ` [PATCH 3/9] ia64: move cacheinfo sysfs to generic cacheinfo infrastructure Sudeep Holla
2014-06-25 17:30 ` [PATCH 4/9] s390: " Sudeep Holla
2014-06-25 17:30 ` [PATCH 5/9] x86: " Sudeep Holla
2014-06-25 17:30 ` [PATCH 6/9] powerpc: " Sudeep Holla
2014-06-25 17:30 ` [PATCH 7/9] ARM64: kernel: add support for cpu cache information Sudeep Holla
2014-06-27 10:36 ` Mark Rutland
2014-06-27 11:22 ` Sudeep Holla
2014-06-27 11:34 ` Mark Rutland
2014-06-25 17:30 ` [PATCH 8/9] ARM: " Sudeep Holla
2014-06-25 22:33 ` Russell King - ARM Linux
2014-06-26 11:33 ` Sudeep Holla
2014-06-26 0:19 ` Stephen Boyd
2014-06-26 11:36 ` Sudeep Holla
2014-06-26 18:45 ` Stephen Boyd
2014-06-27 9:38 ` Sudeep Holla
2014-06-25 17:30 ` [PATCH 9/9] ARM: kernel: add outer cache support for cacheinfo implementation Sudeep Holla
2014-06-25 22:37 ` Russell King - ARM Linux
2014-06-26 13:02 ` Sudeep Holla
2014-07-25 16:44 ` [PATCH v2 0/9] drivers: cacheinfo support Sudeep Holla
2014-07-25 16:44 ` [PATCH v2 1/9] drivers: base: add new class "cpu" to group cpu devices Sudeep Holla
2014-07-25 19:09 ` Stephen Boyd
2014-07-28 13:37 ` Sudeep Holla
2014-07-25 16:44 ` [PATCH v2 2/9] drivers: base: support cpu cache information interface to userspace via sysfs Sudeep Holla
2014-07-29 23:09 ` Stephen Boyd
2014-07-30 16:23 ` Sudeep Holla
2014-07-31 19:46 ` Stephen Boyd [this message]
2014-08-05 18:15 ` Sudeep Holla
2014-07-25 16:44 ` [PATCH v2 3/9] ia64: move cacheinfo sysfs to generic cacheinfo infrastructure Sudeep Holla
2014-07-25 16:44 ` [PATCH v2 4/9] s390: " Sudeep Holla
2014-07-25 16:44 ` [PATCH v2 5/9] x86: " Sudeep Holla
2014-07-25 16:44 ` [PATCH v2 6/9] powerpc: " Sudeep Holla
2014-07-25 16:44 ` [PATCH v2 7/9] ARM64: kernel: add support for cpu cache information Sudeep Holla
2014-07-25 16:44 ` [PATCH v2 8/9] ARM: " Sudeep Holla
2014-07-25 16:44 ` [PATCH v2 9/9] ARM: kernel: add outer cache support for cacheinfo implementation Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 00/11] drivers: cacheinfo support Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 01/11] cpumask: factor out show_cpumap into separate helper function Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 02/11] topology: replace custom attribute macros with standard DEVICE_ATTR* Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 03/11] drivers: base: add new class "cpu" to group cpu devices Sudeep Holla
2014-08-21 11:20 ` David Herrmann
2014-08-21 12:30 ` Sudeep Holla
2014-08-21 12:37 ` David Herrmann
2014-08-21 14:54 ` Sudeep Holla
2014-08-22 9:12 ` Kay Sievers
2014-08-22 11:29 ` [PATCH] drivers: base: add cpu_device_create to support per-cpu devices Sudeep Holla
2014-08-22 11:37 ` David Herrmann
2014-08-22 11:41 ` David Herrmann
2014-08-22 12:33 ` Sudeep Holla
2014-08-26 16:54 ` Sudeep Holla
2014-08-26 17:08 ` David Herrmann
2014-08-22 12:17 ` Sudeep Holla
2014-09-02 17:22 ` Sudeep Holla
2014-09-02 17:26 ` Greg Kroah-Hartman
2014-09-02 17:40 ` Sudeep Holla
2014-09-02 17:55 ` Greg Kroah-Hartman
2014-08-21 10:59 ` [PATCH v3 04/11] drivers: base: support cpu cache information interface to userspace via sysfs Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 05/11] ia64: move cacheinfo sysfs to generic cacheinfo infrastructure Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 06/11] s390: " Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 07/11] x86: " Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 08/11] powerpc: " Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 09/11] ARM64: kernel: add support for cpu cache information Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 10/11] ARM: " Sudeep Holla
2014-08-21 10:59 ` [PATCH v3 11/11] ARM: kernel: add outer cache support for cacheinfo implementation Sudeep Holla
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53DA9D03.60606@codeaurora.org \
--to=sboyd@codeaurora.org \
--cc=Lorenzo.Pieralisi@arm.com \
--cc=gregkh@linuxfoundation.org \
--cc=heiko.carstens@de.ibm.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sudeep.holla@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).