RE: FW: [PATCH 5/6] hwmon: amd_energy: dump energy counters via debugfs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[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 :)
>




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux