Re: [PATCH 06/15] PM QoS: implement the per-device PM QoS constraints

["Rafael J. Wysocki" <rjw@sisk.pl>]

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