Hi,
On Sunday, August 14, 2011, Jean Pihet wrote:
...
> >> +
> >> + if (dev_pm_qos_request_active(req)) {
> >> + WARN(1, KERN_ERR "dev_pm_qos_add_request() called for
> >> already "
> >> + "added request\n");
> >> + return;
> >> + }
> >> + req->dev = dev;
> >> +
> >> + /* Allocate the constraints struct on the first call to add_request
> >> */
> >> + if (req->dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
> >> + dev_pm_qos_constraints_allocate(dev);
> >
> > Why not to do
> >
> > + if (!req->dev->power.constraints)
> > + dev_pm_qos_constraints_allocate(dev);
>
> Cf. my comments at the end of this message for the data structs
> alloc/free and the locking.
>
> >
> >> +
> >> + /* Silently return if the device has been removed */
> >> + if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> >> + return;
> >> +
> >
> > Hmm. What will happen if two callers run dev_pm_qos_add_request()
> > concurrently for the same device?
> update_target is using the power.lock to protect the constraints lists
> changes.
I was referring to the scenario in particular:
Suppose that there are no constraits for dev initially.
A -> calls dev_pm_qos_add_request(dev)
B -> calls dev_pm_qos_add_request(dev)
A -> sees power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT
and calls dev_pm_qos_constraints_allocate(dev)
B -> sees power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT
and calls dev_pm_qos_constraints_allocate(dev)
As a result, the structure allocated by A is leaked.
...
>
> Your remarks are inline with the concerns I had about the adata
> structs alloc/free and the locking.
>
> 1) data structs alloc/free
> As described in the changelog:
> >> To minimize the data usage by the per-device constraints, the data struct
> >> is only allocated at the first call to dev_pm_qos_add_request.
> >> The data is later free'd when the device is removed from the system.
> A basic state machine is needed in order to allocate the data at the
> first call to add_request and free it when the device is removed.
> Another option is to allocate the data when the device is added to the
> system and free it when the device is removed. That would make the
> code simpler but it is using a lot of memory for unneeded data.
That's fine. You simply need to be more careful about the possible
race conditions when the constraints objects are created and destroyed.
> 2) Race conditions
> I will add a lock to isolate the API callers from
> dev_pm_qos_constraints_destroy().
> The power.lock is already used by update_target so another lock is
> needed to protect the data allocation/free.
OK
> I will rework this tomorrow and re-submit asap when it is ready.
> Is that OK?
Yes, it is.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
