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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
