Re: [RFC] QoS params patch

[Paul Mundt <lethal@linux-sh.org>]

On Wed, Sep 26, 2007 at 03:40:26PM -0700, Mark Gross wrote:
> +	struct list_head list;
> +	union {
> +		s32 value;
> +		s32 usec;
> +		s32 kbps;
> +		};
> +	char *name;

Your } is in a strange place. It looks like it wants to join its friends
closer to the left margin ;-)

> +};
> +
> +#define QOS_RESERVED 0
> +#define QOS_CPU_DMA_LATENCY 1
> +#define QOS_NETWORK_LATENCY 2
> +#define QOS_NETWORK_THROUGHPUT 3
> +
> +#define QOS_NUM_CLASSES 4
> +#define QOS_DEFAULT_VALUE -1

an enum would be better for this, especially as people are likely to add
new types, having to update QOS_NUM_CLASSES each time sucks.

> +/* static helper functions */
> +static s32 max_compare(s32 v1, s32 v2)
> +{
> +	if (v1 < v2)
> +		return v2;
> +	else
> +		return v1;
> +}
> +
> +static s32 min_compare(s32 v1, s32 v2)
> +{
> +	if (v1 < v2)
> +		return v1;
> +	else
> +		return v2;
> +}
> +
min()/max() instead?

> +/* assumes qos_lock is held */
> +static void update_target(int i)
> +{
'target' might be a more meaningful variable name.

> +	qos->qos_power_miscdev.fops = &qos_power_fops;
> +
> +	ret = misc_register(& qos->qos_power_miscdev);
> +	if (ret < 0 )
> +		printk(KERN_ERR "qos_power: misc_register returns %d.\n", ret);
> +
> +	return ret;
> +}
> +
Just return misc_register(...); ?

> +	if( i < QOS_NUM_CLASSES) {
> +		qos = &qos_array[i];
> +		qos->name = kstrdup(name, GFP_KERNEL);
> +		if(!qos->name)
> +			goto cleanup;
> +
> +		qos->default_value = default_value;
> +		qos->target_value = default_value;
> +		qos->comparitor = comparitor;
> +		srcu_init_notifier_head(&qos->notifiers);
> +		INIT_LIST_HEAD(&qos->requirements.list);
> +		if (register_new_qos_misc(qos) < 0 )
> +			goto cleanup;
> +	}
> +	return;
> +cleanup:
> +	kfree(qos->name);
> +}

This leaks. You'll have to scan down from i and clean up the kstrdup()
per qos_array element. Presently this will only free the first one to fail
registration.

> +
> +int qos_add_requirement(int i, char *name, s32 value)
> +{
> +	struct requirement_list * dep;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&qos_lock, flags);
> +	dep = kzalloc(sizeof(struct requirement_list), GFP_KERNEL);

kmalloc() under a spinlock. GFP_KERNEL implies __GFP_WAIT, which can
sleep. Slab debugging would have caught this, too.

> +	if (dep) {
> +		if (value == QOS_DEFAULT_VALUE)
> +			dep->value = qos_array[i].default_value;
> +		else
> +			dep->value = value;
> +		dep->name = kstrdup(name, GFP_KERNEL);

And here also, still under the spinlock. You can probably rework the
locking just to protect the list, if you really need it at all, it
doesn't seem to matter anywhere else here.

> +		if(!dep->name)
> +			goto cleanup;
> +
> +		list_add(&dep->list, &qos_array[i].requirements.list);
> +		update_target(i);
> +		spin_unlock_irqrestore(&qos_lock, flags);
> +
> +		return 0;
> +	}
> +	spin_unlock_irqrestore(&qos_lock, flags);
> +	
> +cleanup:
> +	if(dep)
> +		kfree(dep);

no if() needed. 

> +	return -ENOMEM;
> +}
> +EXPORT_SYMBOL_GPL(qos_add_requirement);

You also didn't spin_unlock_irqrestore() in the error path, so this bails
out with the lock held and IRQs disabled.