[AMD Public Use]
Hi Guenter,
-----Original Message-----
From: linux-hwmon-owner@xxxxxxxxxxxxxxx <linux-hwmon-owner@xxxxxxxxxxxxxxx> On Behalf Of Guenter Roeck
Sent: Tuesday, September 8, 2020 10:42 PM
To: Naveen Krishna Ch <naveenkrishna.ch@xxxxxxxxx>
Cc: linux-hwmon@xxxxxxxxxxxxxxx; Guenter Roeck <groeck7@xxxxxxxxx>
Subject: Re: FW: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs
[CAUTION: External Email]
On 9/8/20 9:46 AM, Naveen Krishna Ch wrote:
> Hi Guenter
>
> On Tue, 8 Sep 2020 at 22:04, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>>
>> On Tue, Sep 08, 2020 at 09:40:40PM +0530, Naveen Krishna Ch wrote:
>>> Hi Guenter
>>>
>>> On Sat, 5 Sep 2020 at 22:28, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>>>>
>>>> On 9/5/20 9:41 AM, Naveen Krishna Ch wrote:
>>>>> Hi Guenter,
>>>>>
>>>>>> On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote:
>>>>>>> Use seq_printf to capture the core and socket energies under
>>>>>>> debugfs path in '/sys/kernel/debug/amd_energy/'
>>>>>>> file cenergy_dump: To print out the core energy counters file
>>>>>>> senergy_dump: To print out the socket energy counters
>>>>>>>
>>>>>>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
>>>>>>
>>>>>> Isn't this a duplicate of other functionality available in the kernel ?
>>>>>> I'd have to look it up, but I am quite sure that energy values are already available. Besides that, what is the point of duplicating the hwmon attributes ?
>>>>>
>>>>> Idea is to reduce the latency in gathering all the counters (Rome
>>>>> has
>>>>> 128 cores on 2 sockets) from the user space.
>>>>> If there is a better way to get this done, please let me know. I
>>>>> shall implement and resubmit.
>>>>>
>>>>
>>>> Isn't turbostat supposed to be able to do that ?
>>> Apologies, I was not aware of turbostat, took a look at the
>>> turbostat code now, this is an elaborate tool which is dependent on
>>> msr.ko. At present, this tool provides a lot of information in
>>> sequence. There is no duplication of the code.
>>>
>>> We need this functionality for the following reasons.
>>> 1. Reduced latency in gathering the energy counters of all cores and
>>> packages 2. Possible to provide an API to the user space to
>>> integrate into other tools/frameworks
>>>
>>> Please let me know your opinion.
>>
>> debugfs should only used for debugging and not to create a backdoor API.
>> What you are looking for is a more efficient API than the hwmon API
> Yes and when i looked around i found this debugfs implementation in
> k10temp driver, providing the svi and thm information. So, I have
> implemented accordingly. Should have discussed this earlier.
>
That is purely for debugging, needed because AMD doesn't publish technical documents describing the register values. If it is used to argue for a backdoor API, I'll take it out. Actually, I'll submit a patch to do just that.
[naveenk:] Didn't mean to use it for an argument. Merely trying to give a reason why I chose to implement it this way. I agree, debugfs is not for this purpose.
>> to access sensor data. There is an available API to do that: iio.
>> You have two options: Register the driver with iio directly, or
>> better implement a generic hwmon->iio bridge in the hwmon core. I
>> have wanted to do the latter forever, but never got around to impementing it.
> I've had some experience with iio drivers in the past, i will look
> into this. In the meanwhile, can we keep this implementation here ?
[naveenk:] I explored the IIO subsystem on your suggestions. IIO seems to be supporting INT, Fractions(INT_PLUS_MICRO, INT_PLUS_NANO, FRACTIONAL) and IIO_VAL_INT_MULTIPLE for the reads. The energy counter values we are reporting are 64bits, there is no return multi support for the fractions at present.
Is it acceptable to add IIO_VAL_U64_MULTIPLE support first, any thoughts ? If this is acceptable, i will create a bridge hwmon->iio to update those values from the existing driver.
>
No.
Guenter
>
>>
>> Guenter
>>
>>>>
>>>> Guenter
>>>>
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>>> ---
>>>>>>> drivers/hwmon/amd_energy.c | 110
>>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>> 1 file changed, 110 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/hwmon/amd_energy.c
>>>>>>> b/drivers/hwmon/amd_energy.c index c294bea56c02..2184bd4510ed
>>>>>>> 100644
>>>>>>> --- a/drivers/hwmon/amd_energy.c
>>>>>>> +++ b/drivers/hwmon/amd_energy.c
>>>>>>> @@ -8,6 +8,7 @@
>>>>>>> #include <linux/bits.h>
>>>>>>> #include <linux/cpu.h>
>>>>>>> #include <linux/cpumask.h>
>>>>>>> +#include <linux/debugfs.h>
>>>>>>> #include <linux/delay.h>
>>>>>>> #include <linux/device.h>
>>>>>>> #include <linux/hwmon.h>
>>>>>>> @@ -20,6 +21,7 @@
>>>>>>> #include <linux/platform_device.h> #include <linux/sched.h>
>>>>>>> #include <linux/slab.h>
>>>>>>> +#include <linux/smp.h>
>>>>>>> #include <linux/topology.h>
>>>>>>> #include <linux/types.h>
>>>>>>>
>>>>>>> @@ -57,6 +59,8 @@ struct amd_energy_data {
>>>>>>> int nr_socks;
>>>>>>> int core_id;
>>>>>>> char (*label)[10];
>>>>>>> + u64 *cdump;
>>>>>>> + u64 *sdump;
>>>>>>> };
>>>>>>>
>>>>>>> static int amd_energy_read_labels(struct device *dev, @@ -329,6
>>>>>>> +333,108 @@ static int create_accumulate_status_file(struct
>>>>>>> +amd_energy_data *data)
>>>>>>> &accumulate_attr.attr); }
>>>>>>>
>>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>> +static void dump_on_each_cpu(void *info) {
>>>>>>> + struct amd_energy_data *data = info;
>>>>>>> + int cpu = smp_processor_id();
>>>>>>> +
>>>>>>> + amd_add_delta(data, cpu, cpu, (long *)&data->cdump[cpu],
>>>>>>> + ENERGY_CORE_MSR); }
>>>>>>> +
>>>>>>> +static int cenergy_dump_show(struct seq_file *s, void *unused) {
>>>>>>> + struct amd_energy_data *data = s->private;
>>>>>>> + struct cpumask *cpus_mask;
>>>>>>> + int i;
>>>>>>> +
>>>>>>> + cpus_mask = kmalloc(sizeof(*cpus_mask), GFP_KERNEL);
>>>>>>> + memset(data->cdump, 0, (data->nr_cpus) * sizeof(u64));
>>>>>>> + cpumask_clear(cpus_mask);
>>>>>>> + for (i = 0; i < data->nr_cpus; i++) {
>>>>>>> + if (cpu_online(i))
>>>>>>> + cpumask_set_cpu(i, cpus_mask);
>>>>>>> + }
>>>>>>> +
>>>>>>> + on_each_cpu_mask(cpus_mask, dump_on_each_cpu, data, true);
>>>>>>> +
>>>>>>> + for (i = 0; i < data->nr_cpus; i++) {
>>>>>>> + if (!(i & 3))
>>>>>>> + seq_printf(s, "Core %3d: ", i);
>>>>>>> +
>>>>>>> + seq_printf(s, "%16llu ", data->cdump[i]);
>>>>>>> + if ((i & 3) == 3)
>>>>>>> + seq_puts(s, "\n");
>>>>>>> + }
>>>>>>> + seq_puts(s, "\n");
>>>>>>> +
>>>>>>> + kfree(cpus_mask);
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +DEFINE_SHOW_ATTRIBUTE(cenergy_dump);
>>>>>>> +
>>>>>>> +static int senergy_dump_show(struct seq_file *s, void *unused) {
>>>>>>> + struct amd_energy_data *data = s->private;
>>>>>>> + int i, cpu;
>>>>>>> +
>>>>>>> + for (i = 0; i < data->nr_socks; i++) {
>>>>>>> + cpu = cpumask_first_and(cpu_online_mask,
>>>>>>> + cpumask_of_node(i));
>>>>>>> + amd_add_delta(data, data->nr_cpus + i, cpu,
>>>>>>> + (long *)&data->sdump[i], ENERGY_PKG_MSR);
>>>>>>> + seq_printf(s, "Socket %1d: %16llu\n",
>>>>>>> + i, data->sdump[i]);
>>>>>>> + }
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +DEFINE_SHOW_ATTRIBUTE(senergy_dump);
>>>>>>> +
>>>>>>> +static void dump_debugfs_cleanup(void *ddir) {
>>>>>>> + debugfs_remove_recursive(ddir); }
>>>>>>> +
>>>>>>> +static int create_dump_file(struct device *dev,
>>>>>>> + struct amd_energy_data *data) {
>>>>>>> + struct dentry *debugfs;
>>>>>>> + char name[] = "amd_energy";
>>>>>>> +
>>>>>>> + data->cdump = devm_kcalloc(dev, data->nr_cpus,
>>>>>>> + sizeof(u64), GFP_KERNEL);
>>>>>>> + if (!data->cdump)
>>>>>>> + return -ENOMEM;
>>>>>>> +
>>>>>>> + data->sdump = devm_kcalloc(dev, data->nr_socks,
>>>>>>> + sizeof(u64), GFP_KERNEL);
>>>>>>> + if (!data->sdump)
>>>>>>> + return -ENOMEM;
>>>>>>> +
>>>>>>> + debugfs = debugfs_create_dir(name, NULL);
>>>>>>> + if (debugfs) {
>>>>>>> + debugfs_create_file("cenergy_dump", 0440,
>>>>>>> + debugfs, data, &cenergy_dump_fops);
>>>>>>> + debugfs_create_file("senergy_dump", 0440,
>>>>>>> + debugfs, data, &senergy_dump_fops);
>>>>>>> + devm_add_action_or_reset(data->hwmon_dev,
>>>>>>> + dump_debugfs_cleanup, debugfs);
>>>>>>> + }
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +#else
>>>>>>> +
>>>>>>> +static int create_dump_file(struct device *dev,
>>>>>>> + struct amd_energy_data *data) {
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +#endif //CONFIG_DEBUG_FS
>>>>>>> +
>>>>>>> static int amd_energy_probe(struct platform_device *pdev) {
>>>>>>> struct amd_energy_data *data; @@ -376,6 +482,10 @@ static
>>>>>>> int amd_energy_probe(struct platform_device *pdev)
>>>>>>> if (ret)
>>>>>>> return ret;
>>>>>>>
>>>>>>> + ret = create_dump_file(dev, data);
>>>>>>> + if (ret)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>> Naveen
>>>>>
>>>>
>>>
>>>
>>> --
>>> Shine bright,
>>> (: Nav :)
>
>
>
> --
> Shine bright,
> (: Nav :)
>
