Re: [RFC] QoS params patch

[Mark Gross <mgross@linux.intel.com>]

On Thu, Sep 27, 2007 at 11:24:40AM +0900, Paul Mundt wrote:
> 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 ;-)

fixed.

> 
> > +};
> > +
> > +#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.

I'm partial to the simplicity of not using an enum in this case.  We do
want it to be somewhat non-trivial to add qos classes.  

> 
> > +/* 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?

done.

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

done.

> 
> > +	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(...); ?

ok.

> 
> > +	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.

Not seeing the leak here.  this is an if block not a for loop.
 
> 
> > +
> > +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.
> 

/me turns on slab debugging.

> > +	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.
>

Re-worked.  I didn't need the lock to be held as long as I had it.
Thanks for catching this!

> > +		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. 

fixed.

> 
> > +	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.

fixed by the re-work.

--mgross