Re: [PATCH v7 REPOST 8/9] arm: add sysfs cpu_capacity attribute

[Juri Lelli]

Hi,

apologies for the delay in replying, but I'm attending Linux Plumbers
this week.

On 30/10/16 20:45, Russell King - ARM Linux wrote:
> On Mon, Oct 17, 2016 at 04:46:49PM +0100, Juri Lelli wrote:
> > +#ifdef CONFIG_PROC_SYSCTL
> > +#include <asm/cpu.h>
> > +#include <linux/string.h>
> 
> Include files at the top of the file please.  No need to ifdef them.
> They're sorted alphabetically, so new additions should be alphabetical.
> (That's a general rule - if something is already alphabetical, do not
> make it non-alphabetical.)
> 
> > +static ssize_t show_cpu_capacity(struct device *dev,
> > +				 struct device_attribute *attr,
> > +				 char *buf)
> > +{
> > +	struct cpu *cpu = container_of(dev, struct cpu, dev);
> > +	ssize_t rc;
> > +	int cpunum = cpu->dev.id;
> > +	unsigned long capacity = arch_scale_cpu_capacity(NULL, cpunum);
> > +
> > +	rc = sprintf(buf, "%lu\n", capacity);
> > +
> > +	return rc;
> 
> Way too many lines for such a simple function.  This can be simplified
> to just:
> 
> 	struct cpu *cpu = container_of(dev, struct cpu, dev);
> 
> 	return sprintf(buf, "%lu\n", arch_scale_cpu_capacity(NULL, cpu->dev.id);
> 
> If you don't like the last line ending on column 79, then feel free to
> break it across two lines after the format string.
> 
> > +}
> > +
> > +static ssize_t store_cpu_capacity(struct device *dev,
> > +				  struct device_attribute *attr,
> > +				  const char *buf,
> > +				  size_t count)
> > +{
> > +	struct cpu *cpu = container_of(dev, struct cpu, dev);
> > +	int this_cpu = cpu->dev.id, i;
> > +	unsigned long new_capacity;
> > +	ssize_t ret;
> > +
> > +	if (count) {
> > +		char *p = (char *) buf;
> > +
> > +		ret = kstrtoul(p, 0, &new_capacity);
> 
> Unnecessary cast - kstrtoul takes a const char pointer, and in any case
> it's really bad form to cast away the "const-ness" of any pointer.  So,
> just:
> 
> 	if (count) {
> 		ret = kstrtoul(buf, 0, &new_capacity);
> 
> should work just fine.
> 
> > +		if (ret)
> > +			return ret;
> > +		if (new_capacity > SCHED_CAPACITY_SCALE)
> > +			return -EINVAL;
> > +
> > +		mutex_lock(&cpu_scale_mutex);
> > +		for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
> > +			set_capacity_scale(i, new_capacity);
> > +		mutex_unlock(&cpu_scale_mutex);
> > +	}
> > +
> > +	return count;
> > +}
> > +
> > +static DEVICE_ATTR(cpu_capacity,
> > +		   0644,
> > +		   show_cpu_capacity,
> > +		   store_cpu_capacity);
> 
> There's a move to use the named DEVICE_ATTR_RW() for this kind of thing,
> it'll want the functions named xxx_show() and xxx_store().  I see
> there's some recent patches to do this conversion across the kernel, so
> this should probably be done before submission.
> 
> > +
> > +static int register_cpu_capacity_sysctl(void)
> > +{
> > +	int i;
> > +	struct device *cpu;
> > +
> > +	for_each_possible_cpu(i) {
> > +		cpu = get_cpu_device(i);
> > +		if (!cpu) {
> > +			pr_err("%s: too early to get CPU%d device!\n",
> > +			       __func__, i);
> > +			continue;
> > +		}
> > +		device_create_file(cpu, &dev_attr_cpu_capacity);
> > +	}
> > +
> > +	return 0;
> > +}
> > +late_initcall(register_cpu_capacity_sysctl);
> 
> Hmm, this is really weird.  topology_init() in arch/arm/kernel/setup.c
> is where these devices get created, and they're created at
> subsys_initcall() time.  By that point, the list of possible CPUs has
> to be static, it's not going to change.  I don't see why this has to be
> done at late_initcall() - and since topology.c will be linked after
> setup.c, I don't see why it shouldn't be at subsys_initcall() level to
> follow on after topology_init().
> 

I should have addressed your comments with the updated version below. If
it looks good to you I'll superseed the old version with this new one.

Best,

- Juri

--->8---