| Message ID | 20200905143230.195049-6-nchatrad@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RFC: hwmon: few improvements to amd_energy driver | expand |
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@amd.com> 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 ? 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; > } > >
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@amd.com> > > 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. > > 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
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@amd.com> >> >> 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 ? 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 >
Hi Guenter On Sat, 5 Sep 2020 at 22:28, Guenter Roeck <linux@roeck-us.net> 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@amd.com> > >> > >> 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. > > 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 > > >
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@roeck-us.net> 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@amd.com> > > >> > > >> 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 :)
Hi Guenter On Tue, 8 Sep 2020 at 22:04, Guenter Roeck <linux@roeck-us.net> 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@roeck-us.net> 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@amd.com> > > > >> > > > >> 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 :)
On 9/8/20 9:46 AM, Naveen Krishna Ch wrote: > Hi Guenter > > On Tue, 8 Sep 2020 at 22:04, Guenter Roeck <linux@roeck-us.net> 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@roeck-us.net> 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@amd.com> >>>>>> >>>>>> 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. >> 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 ? > 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 :) >
[AMD Public Use] Hi Guenter, -----Original Message----- From: linux-hwmon-owner@vger.kernel.org <linux-hwmon-owner@vger.kernel.org> On Behalf Of Guenter Roeck Sent: Tuesday, September 8, 2020 10:42 PM To: Naveen Krishna Ch <naveenkrishna.ch@gmail.com> Cc: linux-hwmon@vger.kernel.org; Guenter Roeck <groeck7@gmail.com> 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@roeck-us.net> 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@roeck-us.net> 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@amd.com> >>>>>> >>>>>> 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 :) >
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; }
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@amd.com> --- drivers/hwmon/amd_energy.c | 110 +++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+)