Re: [PATCH v2 1/2] cpu: Offline state Framework.

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 28 Aug 2009 15:30:16 +0530 Gautham R Shenoy <ego@in.ibm.com> wrote:

> Provide an interface by which the system administrator can decide what state
> should the CPU go to when it is offlined.
> 
> To query the hotplug states, on needs to perform a read on the sysfs tunable:
> 	/sys/devices/system/cpu/cpu<number>/available_hotplug_states
> 
> To query or set the current state for a particular CPU, one needs to
> use the sysfs interface:
> 	/sys/devices/system/cpu/cpu<number>/current_state
> 
> This patch implements the architecture independent bits of the
> cpu-offline-state framework.
> 
> Architectures which want to expose the multiple offline-states to the
> userspace are expected to write a driver which can register
> with this framework.
> 
> Such a driver should:
> - Implement the callbacks defined in the structure struct cpu_offline_driver
>   which can be called into by this framework when the corresponding
>   sysfs interfaces are read or written into.
> 
> - Ensure that the following operation puts the CPU in the same state
>   as it did in the absence of the driver.
> 	echo 0 > /sys/devices/system/cpu/cpu<number>/online
> 
> This framework also serializes the writes to the "current_state"
> with respect to with the writes to the "online" sysfs tunable.
> 

It would be nice to document this new userspace interface somewhere.


> +struct cpu_offline_driver *cpu_offline_driver;
> +static DEFINE_MUTEX(cpu_offline_driver_lock);
> +
> +ssize_t show_available_states(struct sys_device *dev,
> +			struct sysdev_attribute *attr, char *buf)
> +{
> +	struct cpu *cpu = container_of(dev, struct cpu, sysdev);
> +	int cpu_num = cpu->sysdev.id;
> +	ssize_t ret;
> +
> +	mutex_lock(&cpu_offline_driver_lock);
> +	if (!cpu_offline_driver) {
> +		ret = -EEXIST;
> +		goto out_unlock;
> +	}
> +
> +	ret = cpu_offline_driver->read_available_states(cpu_num, buf);
> +
> +out_unlock:
> +	mutex_unlock(&cpu_offline_driver_lock);
> +
> +	return ret;
> +
> +}

The patch adds boatloads of global symbols which do not have names
which are appropriate for global symbols.

> +ssize_t show_current_state(struct sys_device *dev,
> +			struct sysdev_attribute *attr, char *buf)

Like that.

> +ssize_t store_current_state(struct sys_device *dev,
> +			struct sysdev_attribute *attr,
> +			const char *buf, size_t count)

And that.

> +
> +static SYSDEV_ATTR(available_hotplug_states, 0444, show_available_states,
> +								NULL);
> +static SYSDEV_ATTR(current_state, 0644, show_current_state,
> +						store_current_state);
> +
> +/* Should be called with cpu_offline_driver_lock held */
> +void cpu_offline_driver_add_cpu(struct sys_device *cpu_sys_dev)
> +{
> +	if (!cpu_offline_driver || !cpu_sys_dev)
> +		return;
> +
> +	sysdev_create_file(cpu_sys_dev, &attr_available_hotplug_states);
> +	sysdev_create_file(cpu_sys_dev, &attr_current_state);
> +}
> +
> +/* Should be called with cpu_offline_driver_lock held */
> +void cpu_offline_driver_remove_cpu(struct sys_device *cpu_sys_dev)
> +{
> +	if (!cpu_offline_driver || !cpu_sys_dev)
> +		return;
> +
> +	sysdev_remove_file(cpu_sys_dev, &attr_available_hotplug_states);
> +	sysdev_remove_file(cpu_sys_dev, &attr_current_state);
> +
> +}

Please don't just ignore possible error returns.

> +int register_cpu_offline_driver(struct cpu_offline_driver *arch_cpu_driver)
> +{
> +	int ret = 0;
> +	int cpu;
> +	mutex_lock(&cpu_offline_driver_lock);
> +

The blank line goes after end-of-locals and before start-of-code.

> +	if (cpu_offline_driver != NULL) {
> +		ret = -EEXIST;
> +		goto out_unlock;
> +	}
> +
> +	if (!(arch_cpu_driver->read_available_states &&
> +	      arch_cpu_driver->read_current_state &&
> +	      arch_cpu_driver->write_current_state)) {
> +		ret = -EINVAL;
> +		goto out_unlock;

This seems pretty pointless.  Just let the code oops - the developer
will notice fairly quickly.

> +	}
> +
> +	cpu_offline_driver = arch_cpu_driver;
> +
> +	for_each_possible_cpu(cpu)
> +		cpu_offline_driver_add_cpu(get_cpu_sysdev(cpu));
> +
> +out_unlock:
> +	mutex_unlock(&cpu_offline_driver_lock);
> +	return ret;
> +}
> +
> +void unregister_cpu_offline_driver(struct cpu_offline_driver *arch_cpu_driver)
> +{
> +	int cpu;
> +	mutex_lock(&cpu_offline_driver_lock);
> +
> +	if (!cpu_offline_driver) {
> +		WARN_ON(1);

	if (WARN_ON(!cpu_offline_driver)) {

> +		mutex_unlock(&cpu_offline_driver_lock);
> +		return;
> +	}
> +
> +	for_each_possible_cpu(cpu)
> +		cpu_offline_driver_remove_cpu(get_cpu_sysdev(cpu));
> +
> +	cpu_offline_driver = NULL;
> +	mutex_unlock(&cpu_offline_driver_lock);
> +}
> +
> +