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

[Jean Pihet <jean.pihet@newoldbits.com>]

Rafael,

2011/8/13 Rafael J. Wysocki <rjw@sisk.pl>:
> Hi,
>
> Well, it looks like I should have reviewed this one more carefully.
>
> On Thursday, August 11, 2011, jean.pihet@newoldbits.com wrote:
>> From: Jean Pihet <j-pihet@ti.com>
>>
>> Implement the per-device PM QoS constraints by creating a device
>> PM QoS API, which calls the PM QoS constraints management core code.
>>
>> The per-device latency constraints data strctures are stored
>> in the device dev_pm_info struct.
>>
>> The device PM code calls the init and destroy of the per-device constraints
>> data struct in order to support the dynamic insertion and removal of the
>> devices in the system.
>>
>> 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.
>>
>> Signed-off-by: Jean Pihet <j-pihet@ti.com>
>> ---
...
>> +/*#define DEBUG*/
>
> Please remove this.
Ok

>
>> +
>> +#include <linux/pm_qos.h>
>> +#include <linux/sched.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/slab.h>
>> +#include <linux/time.h>
>> +#include <linux/fs.h>
>> +#include <linux/device.h>
>> +#include <linux/string.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>
> Are you sure all of the headers are necessary?
No. I can check that.

>
>> +
>> +
>> +static void dev_pm_qos_constraints_allocate(struct device *dev);
>> +
>> +int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
>> +{
>> +     return req->dev != 0;
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_request_active);
>
> That should be a static inline in a header.
Ok

>
>> +
>> +/**
>> + * dev_pm_qos_add_request - inserts new qos request into the list
>> + * @req: pointer to a preallocated handle
>> + * @dev: target device for the constraint
>> + * @value: defines the qos request
>> + *
>> + * This function inserts a new entry in the device constraints list of
>> + * requested qos performance characteristics. It recomputes the aggregate
>> + * QoS expectations of parameters and initializes the dev_pm_qos_request
>> + * handle.  Caller needs to save this handle for later use in updates and
>> + * removal.
>> + */
>> +void dev_pm_qos_add_request(struct dev_pm_qos_request *req, struct device *dev,
>> +                         s32 value)
>
> I'd use a different ordering of arguments, namely (dev, req, value).
Ok

>
>> +{
>> +     if (!req) /*guard against callers passing in null */
>> +             return;
>
> Why not to check for !dev too?
Ok that makes sense

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

>
>> +     pm_qos_update_target(dev->power.constraints,
>> +                          &req->node, PM_QOS_ADD_REQ, value);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
>> +
>> +/**
>> + * dev_pm_qos_update_request - modifies an existing qos request
>> + * @req : handle to list element holding a dev_pm_qos request to use
>> + * @value: defines the qos request
>> + *
>> + * Updates an existing dev PM qos request along with updating the
>> + * target value.
>> + *
>> + * Attempts are made to make this code callable on hot code paths.
>> + */
>> +void dev_pm_qos_update_request(struct dev_pm_qos_request *req,
>> +                        s32 new_value)
>> +{
>> +     if (!req) /*guard against callers passing in null */
>> +             return;
>> +
>> +     if (!dev_pm_qos_request_active(req)) {
>> +             WARN(1, KERN_ERR "dev_pm_qos_update_request() called for "
>> +                     "unknown object\n");
>> +             return;
>> +     }
>> +
>> +     /* Silently return if the device has been removed */
>> +     if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
>> +             return;
>> +
>> +     if (new_value != req->node.prio)
>> +             pm_qos_update_target(
>> +                     req->dev->power.constraints,
>> +                     &req->node, PM_QOS_UPDATE_REQ, new_value);
>
> I'm not sure what prevents dev_pm_qos_constraints_destroy() from removing
> the list from under us while we're executing this?
Cf. my comments at the end of this message for the data structs
alloc/free and the locking.

>
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_update_request);
>> +
>> +/**
>> + * dev_pm_qos_remove_request - modifies an existing qos request
>> + * @req: handle to request list element
>> + *
>> + * Will remove pm qos request from the list of constraints and
>> + * recompute the current target value. Call this on slow code paths.
>> + */
>> +void dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
>> +{
>> +     if (!req) /*guard against callers passing in null */
>> +             return;
>> +
>> +     if (!dev_pm_qos_request_active(req)) {
>> +             WARN(1, KERN_ERR "dev_pm_qos_remove_request() called for "
>> +                     "unknown object\n");
>> +             return;
>> +     }
>> +
>> +     /* Silently return if the device has been removed */
>> +     if (req->dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
>> +             return;
>> +
>> +     pm_qos_update_target(req->dev->power.constraints,
>> +                          &req->node, PM_QOS_REMOVE_REQ,
>> +                          PM_QOS_DEFAULT_VALUE);
>
> Same here.
>
>> +     memset(req, 0, sizeof(*req));
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
>> +
>> +/**
>> + * dev_pm_qos_add_notifier - sets notification entry for changes to target value
>> + * of per-device PM QoS constraints
>> + *
>> + * @dev: target device for the constraint
>> + * @notifier: notifier block managed by caller.
>> + *
>> + * Will register the notifier into a notification chain that gets called
>> + * upon changes to the target value for the device.
>> + */
>> +int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier)
>> +{
>> +     int retval = 0;
>> +
>> +     /* Silently return if the device has been removed */
>> +     if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
>> +             return retval;
>> +
>> +     retval = blocking_notifier_chain_register(
>> +                     dev->power.constraints->notifiers,
>> +                     notifier);
>
> That may be racing with dev_pm_qos_constraints_destroy() too.
>
>> +
>> +     return retval;
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier);
>> +
>> +/**
>> + * dev_pm_qos_remove_notifier - deletes notification for changes to target value
>> + * of per-device PM QoS constraints
>> + *
>> + * @dev: target device for the constraint
>> + * @notifier: notifier block to be removed.
>> + *
>> + * Will remove the notifier from the notification chain that gets called
>> + * upon changes to the target value.
>> + */
>> +int dev_pm_qos_remove_notifier(struct device *dev,
>> +                            struct notifier_block *notifier)
>> +{
>> +     int retval = 0;
>> +
>> +     /* Silently return if the device has been removed */
>> +     if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
>> +             return retval;
>> +
>> +     retval = blocking_notifier_chain_unregister(
>> +                     dev->power.constraints->notifiers,
>> +                     notifier);
>
> Same here.
>
>> +
>> +     return retval;
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_qos_remove_notifier);
>> +
>> +/* Called at the first call to add_request, for constraint data allocation */
>> +static void dev_pm_qos_constraints_allocate(struct device *dev)
>> +{
>> +     struct pm_qos_constraints *c;
>> +     struct blocking_notifier_head *n;
>> +
>> +     c = kzalloc(sizeof(*c), GFP_KERNEL);
>> +     if (!c)
>> +             return;
>> +
>> +     n = kzalloc(sizeof(*n), GFP_KERNEL);
>> +     if (!n) {
>> +             kfree(c);
>> +             return;
>> +     }
>> +     BLOCKING_INIT_NOTIFIER_HEAD(n);
>> +
>> +     dev->power.constraints = c;
>> +     plist_head_init(&dev->power.constraints->list, &dev->power.lock);
>> +     dev->power.constraints->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
>> +     dev->power.constraints->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
>> +     dev->power.constraints->type = PM_QOS_MIN;
>> +     dev->power.constraints->notifiers = n;
>> +     dev->power.constraints_state = DEV_PM_QOS_ALLOCATED;
>> +}
>> +
>> +/* Called from the device PM subsystem at device insertion */
>> +void dev_pm_qos_constraints_init(struct device *dev)
>> +{
>> +     dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT;
>
> Hmm.  Is it insufficient to check if "constraints" is not NULL?
>
>> +}
>> +
>> +/* Called from the device PM subsystem at device removal */
>> +void dev_pm_qos_constraints_destroy(struct device *dev)
>> +{
>> +     struct dev_pm_qos_request *req, *tmp;
>> +     enum dev_pm_qos_state constraints_state = dev->power.constraints_state;
>> +
>> +     dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE;
>
> I'm not sure what the purpose of this is.  I'd simply check if "constraints"
> is not NULL and I'd flush the list if not.
>
> Moreover, that has to go under a lock so that it doesn't race with
> the other functions above.  I think you can reuse power.lock for that.
>

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.

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.

I will rework this tomorrow and re-submit asap when it is ready.
Is that OK?

>
> Thanks,
> Rafael
>

Thanks,
Jean