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