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