* [PATCH v3 0/4] PM QoS: per-cpu PM QoS support
@ 2014-08-27 20:14 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-08-27 20:14 UTC (permalink / raw)
To: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
tglx, rjw
Cc: daniel.lezcano, Lina Iyer
Changes since v2:
[ http://marc.info/?l=linux-pm&m=140794570012619&w=2 ]
- Amend commit texts
This patchset enables drivers to set per-cpu PM QoS. PM QoS requests currently
apply to all cpus. However, drivers may be interested in only a few cpu(s) that
need to gaurantee the QoS requirement. The reason being, some requests may be
needed only to allow a quicker response to an IRQ, while requests may apply to
a certain cpu or a cluster of cpus, as dictated by an usecase. For example, in
ARM architectures, it may be benefical to set a QoS requirement to handle use
cases on big/Little cluster, while the other cluster can be in the deepest idle
state it can be.
An intended application of this feature could be the cpuidle governor. For
example, governor can now look at a CPU_DMA_LATENCY request only for that cpu
when deciding the allowable C-States. In a clustered environment, the governor
would look at the QoS request for a set of cpus before deciding the cluster's
low power mode.
With these requirements in mind, the patches do the following -
- Reorganize the PM QoS and dev-PM QoS frameworks to pass around data structures
rather than member variables. Do not want to pass plist around. It is much more
scalable, if we pass the pm_qos_request object rather than its member.
- Enhance PM QoS framework to support the per-cpu requests. This is done by
adding a per-cpu array for each constraint. This array holds the per-cpu QoS
value for each constraint. The intelligence behind the per-cpu is on the driver.
The framework just adds the request against the list held by the constraint
structure. Adding a request adds the request to the list of requests
against a constraint and also parses the array to see if this request superseeds
any existing QoS value for the cpus that this request has specified. The
workload of parsing the request is borne when updating the request.
- The driver that does an add request passes in the PM QoS request
structure, can now pass in an additional request type that indicates if the
request is to be applied to all cores (default and existing behavior) or applies
to a subset of the cpus using a cpumask.
- To get the request value of a constraint back, you could use the
pm_qos_request() or its new cpu variants pm_qos_request_for_cpu() and
pm_qos_request_for_cpumask(). The _for_cpu() allows returns an s32 value without
locking.
- The next feature builds up on the per-cpu PM QoS feature to provide the
ability to allow a driver to tag a QoS request against an IRQ instead of a cpu.
Drivers may be concerned with an IRQ handling latency because of CPU low
power states, for example. When adding a request, they could specify the request
type to tag an IRQ and provide the IRQ numbers. When using tools like
irq-balancer, the IRQ affinity may jump from one cpu to another and in such a
case, the request needs to be modified correctly. irq/manage.c provides such
notification whenever the affinity changes. However, the current mechanism
allows for only one notification callback.
- Change to the smp_affinity_notifications. Modify the exisiting
mechanism of a single callback to support a list of requests. Drivers
can register for change notifications.
- From PM QoS register for notification on behalf of a driver whenever
a request with IRQ is specified. When the notification arrives, modify
the cpu affinity of the QoS request to the cpus, that the IRQ is affine to.
- Remarks
I do not have the cpuidle governor changes here at this point. Also, there is
no change to userspace at this point, i.e, userspace cannot specify a QoS
request against a cpu or specify an IRQ to tag this request against.
Thanks,
Lina
Lina Iyer (4):
QoS: Modify data structures and function arguments for scalability.
QoS: Enhance framework to support per-cpu PM QoS request
irq: Allow multiple clients to register for irq affinity notification
QoS: Enable PM QoS requests to apply only on smp_affinity of an IRQ
Documentation/power/pm_qos_interface.txt | 18 +++
drivers/base/power/qos.c | 14 +--
drivers/infiniband/hw/qib/qib_iba7322.c | 4 +-
include/linux/interrupt.h | 12 +-
include/linux/irqdesc.h | 6 +-
include/linux/pm_qos.h | 25 +++-
kernel/irq/irqdesc.c | 1 +
kernel/irq/manage.c | 85 +++++++++-----
kernel/power/qos.c | 191 ++++++++++++++++++++++++++++++-
lib/cpu_rmap.c | 2 +-
10 files changed, 305 insertions(+), 53 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 0/4] PM QoS: per-cpu PM QoS support
@ 2014-08-27 20:14 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-08-27 20:14 UTC (permalink / raw)
To: linux-arm-kernel
Changes since v2:
[ http://marc.info/?l=linux-pm&m=140794570012619&w=2 ]
- Amend commit texts
This patchset enables drivers to set per-cpu PM QoS. PM QoS requests currently
apply to all cpus. However, drivers may be interested in only a few cpu(s) that
need to gaurantee the QoS requirement. The reason being, some requests may be
needed only to allow a quicker response to an IRQ, while requests may apply to
a certain cpu or a cluster of cpus, as dictated by an usecase. For example, in
ARM architectures, it may be benefical to set a QoS requirement to handle use
cases on big/Little cluster, while the other cluster can be in the deepest idle
state it can be.
An intended application of this feature could be the cpuidle governor. For
example, governor can now look at a CPU_DMA_LATENCY request only for that cpu
when deciding the allowable C-States. In a clustered environment, the governor
would look at the QoS request for a set of cpus before deciding the cluster's
low power mode.
With these requirements in mind, the patches do the following -
- Reorganize the PM QoS and dev-PM QoS frameworks to pass around data structures
rather than member variables. Do not want to pass plist around. It is much more
scalable, if we pass the pm_qos_request object rather than its member.
- Enhance PM QoS framework to support the per-cpu requests. This is done by
adding a per-cpu array for each constraint. This array holds the per-cpu QoS
value for each constraint. The intelligence behind the per-cpu is on the driver.
The framework just adds the request against the list held by the constraint
structure. Adding a request adds the request to the list of requests
against a constraint and also parses the array to see if this request superseeds
any existing QoS value for the cpus that this request has specified. The
workload of parsing the request is borne when updating the request.
- The driver that does an add request passes in the PM QoS request
structure, can now pass in an additional request type that indicates if the
request is to be applied to all cores (default and existing behavior) or applies
to a subset of the cpus using a cpumask.
- To get the request value of a constraint back, you could use the
pm_qos_request() or its new cpu variants pm_qos_request_for_cpu() and
pm_qos_request_for_cpumask(). The _for_cpu() allows returns an s32 value without
locking.
- The next feature builds up on the per-cpu PM QoS feature to provide the
ability to allow a driver to tag a QoS request against an IRQ instead of a cpu.
Drivers may be concerned with an IRQ handling latency because of CPU low
power states, for example. When adding a request, they could specify the request
type to tag an IRQ and provide the IRQ numbers. When using tools like
irq-balancer, the IRQ affinity may jump from one cpu to another and in such a
case, the request needs to be modified correctly. irq/manage.c provides such
notification whenever the affinity changes. However, the current mechanism
allows for only one notification callback.
- Change to the smp_affinity_notifications. Modify the exisiting
mechanism of a single callback to support a list of requests. Drivers
can register for change notifications.
- From PM QoS register for notification on behalf of a driver whenever
a request with IRQ is specified. When the notification arrives, modify
the cpu affinity of the QoS request to the cpus, that the IRQ is affine to.
- Remarks
I do not have the cpuidle governor changes here at this point. Also, there is
no change to userspace at this point, i.e, userspace cannot specify a QoS
request against a cpu or specify an IRQ to tag this request against.
Thanks,
Lina
Lina Iyer (4):
QoS: Modify data structures and function arguments for scalability.
QoS: Enhance framework to support per-cpu PM QoS request
irq: Allow multiple clients to register for irq affinity notification
QoS: Enable PM QoS requests to apply only on smp_affinity of an IRQ
Documentation/power/pm_qos_interface.txt | 18 +++
drivers/base/power/qos.c | 14 +--
drivers/infiniband/hw/qib/qib_iba7322.c | 4 +-
include/linux/interrupt.h | 12 +-
include/linux/irqdesc.h | 6 +-
include/linux/pm_qos.h | 25 +++-
kernel/irq/irqdesc.c | 1 +
kernel/irq/manage.c | 85 +++++++++-----
kernel/power/qos.c | 191 ++++++++++++++++++++++++++++++-
lib/cpu_rmap.c | 2 +-
10 files changed, 305 insertions(+), 53 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 1/4] QoS: Modify data structures and function arguments for scalability.
2014-08-27 20:14 ` Lina Iyer
@ 2014-08-27 20:14 ` Lina Iyer
-1 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-08-27 20:14 UTC (permalink / raw)
To: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
tglx, rjw
Cc: daniel.lezcano, Lina Iyer, Praveen Chidambaram
From: Praveen Chidambaram <pchidamb@codeaurora.org>
QoS add requests uses a handle to the priority list that is used
internally to save the request, but this does not extend well. Also,
dev_pm_qos structure definition seems to use a list object directly.
The 'derivative' relationship seems to be broken.
Use pm_qos_request objects instead of passing around the protected
priority list object.
Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 36b9eb4..67a66b1 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -143,7 +143,7 @@ static int apply_constraint(struct dev_pm_qos_request *req,
switch(req->type) {
case DEV_PM_QOS_RESUME_LATENCY:
ret = pm_qos_update_target(&qos->resume_latency,
- &req->data.pnode, action, value);
+ &req->data.lat, action, value);
if (ret) {
value = pm_qos_read_value(&qos->resume_latency);
blocking_notifier_call_chain(&dev_pm_notifiers,
@@ -153,7 +153,7 @@ static int apply_constraint(struct dev_pm_qos_request *req,
break;
case DEV_PM_QOS_LATENCY_TOLERANCE:
ret = pm_qos_update_target(&qos->latency_tolerance,
- &req->data.pnode, action, value);
+ &req->data.lat, action, value);
if (ret) {
value = pm_qos_read_value(&qos->latency_tolerance);
req->dev->power.set_latency_tolerance(req->dev, value);
@@ -254,7 +254,7 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
/* Flush the constraints lists for the device. */
c = &qos->resume_latency;
- plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
+ plist_for_each_entry_safe(req, tmp, &c->list, data.lat.node) {
/*
* Update constraints list and call the notification
* callbacks if needed
@@ -263,7 +263,7 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
memset(req, 0, sizeof(*req));
}
c = &qos->latency_tolerance;
- plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
+ plist_for_each_entry_safe(req, tmp, &c->list, data.lat.node) {
apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
memset(req, 0, sizeof(*req));
}
@@ -378,7 +378,7 @@ static int __dev_pm_qos_update_request(struct dev_pm_qos_request *req,
switch(req->type) {
case DEV_PM_QOS_RESUME_LATENCY:
case DEV_PM_QOS_LATENCY_TOLERANCE:
- curr_value = req->data.pnode.prio;
+ curr_value = req->data.lat.node.prio;
break;
case DEV_PM_QOS_FLAGS:
curr_value = req->data.flr.flags;
@@ -831,8 +831,8 @@ s32 dev_pm_qos_get_user_latency_tolerance(struct device *dev)
mutex_lock(&dev_pm_qos_mtx);
ret = IS_ERR_OR_NULL(dev->power.qos)
|| !dev->power.qos->latency_tolerance_req ?
- PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT :
- dev->power.qos->latency_tolerance_req->data.pnode.prio;
+ PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT :
+ dev->power.qos->latency_tolerance_req->data.lat.node.prio;
mutex_unlock(&dev_pm_qos_mtx);
return ret;
}
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 9ab4bf7..e1b763d 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -60,7 +60,7 @@ enum dev_pm_qos_req_type {
struct dev_pm_qos_request {
enum dev_pm_qos_req_type type;
union {
- struct plist_node pnode;
+ struct pm_qos_request lat;
struct pm_qos_flags_request flr;
} data;
struct device *dev;
@@ -112,7 +112,8 @@ static inline int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
return req->dev != NULL;
}
-int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
+int pm_qos_update_target(struct pm_qos_constraints *c,
+ struct pm_qos_request *req,
enum pm_qos_req_action action, int value);
bool pm_qos_update_flags(struct pm_qos_flags *pqf,
struct pm_qos_flags_request *req,
@@ -210,7 +211,7 @@ int dev_pm_qos_update_user_latency_tolerance(struct device *dev, s32 val);
static inline s32 dev_pm_qos_requested_resume_latency(struct device *dev)
{
- return dev->power.qos->resume_latency_req->data.pnode.prio;
+ return dev->power.qos->resume_latency_req->data.lat.node.prio;
}
static inline s32 dev_pm_qos_requested_flags(struct device *dev)
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 884b770..d0b9c0f 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -161,19 +161,21 @@ static inline void pm_qos_set_value(struct pm_qos_constraints *c, s32 value)
* pm_qos_update_target - manages the constraints list and calls the notifiers
* if needed
* @c: constraints data struct
- * @node: request to add to the list, to update or to remove
+ * @req: request to add to the list, to update or to remove
* @action: action to take on the constraints list
* @value: value of the request to add or update
*
* This function returns 1 if the aggregated constraint value has changed, 0
* otherwise.
*/
-int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
+int pm_qos_update_target(struct pm_qos_constraints *c,
+ struct pm_qos_request *req,
enum pm_qos_req_action action, int value)
{
unsigned long flags;
int prev_value, curr_value, new_value;
int ret;
+ struct plist_node *node = &req->node;
spin_lock_irqsave(&pm_qos_lock, flags);
prev_value = pm_qos_get_value(c);
@@ -310,7 +312,7 @@ static void __pm_qos_update_request(struct pm_qos_request *req,
if (new_value != req->node.prio)
pm_qos_update_target(
pm_qos_array[req->pm_qos_class]->constraints,
- &req->node, PM_QOS_UPDATE_REQ, new_value);
+ req, PM_QOS_UPDATE_REQ, new_value);
}
/**
@@ -355,7 +357,7 @@ void pm_qos_add_request(struct pm_qos_request *req,
INIT_DELAYED_WORK(&req->work, pm_qos_work_fn);
trace_pm_qos_add_request(pm_qos_class, value);
pm_qos_update_target(pm_qos_array[pm_qos_class]->constraints,
- &req->node, PM_QOS_ADD_REQ, value);
+ req, PM_QOS_ADD_REQ, value);
}
EXPORT_SYMBOL_GPL(pm_qos_add_request);
@@ -409,7 +411,7 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req, s32 new_value,
if (new_value != req->node.prio)
pm_qos_update_target(
pm_qos_array[req->pm_qos_class]->constraints,
- &req->node, PM_QOS_UPDATE_REQ, new_value);
+ req, PM_QOS_UPDATE_REQ, new_value);
schedule_delayed_work(&req->work, usecs_to_jiffies(timeout_us));
}
@@ -437,7 +439,7 @@ void pm_qos_remove_request(struct pm_qos_request *req)
trace_pm_qos_remove_request(req->pm_qos_class, PM_QOS_DEFAULT_VALUE);
pm_qos_update_target(pm_qos_array[req->pm_qos_class]->constraints,
- &req->node, PM_QOS_REMOVE_REQ,
+ req, PM_QOS_REMOVE_REQ,
PM_QOS_DEFAULT_VALUE);
memset(req, 0, sizeof(*req));
}
--
1.9.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 1/4] QoS: Modify data structures and function arguments for scalability.
@ 2014-08-27 20:14 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-08-27 20:14 UTC (permalink / raw)
To: linux-arm-kernel
From: Praveen Chidambaram <pchidamb@codeaurora.org>
QoS add requests uses a handle to the priority list that is used
internally to save the request, but this does not extend well. Also,
dev_pm_qos structure definition seems to use a list object directly.
The 'derivative' relationship seems to be broken.
Use pm_qos_request objects instead of passing around the protected
priority list object.
Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 36b9eb4..67a66b1 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -143,7 +143,7 @@ static int apply_constraint(struct dev_pm_qos_request *req,
switch(req->type) {
case DEV_PM_QOS_RESUME_LATENCY:
ret = pm_qos_update_target(&qos->resume_latency,
- &req->data.pnode, action, value);
+ &req->data.lat, action, value);
if (ret) {
value = pm_qos_read_value(&qos->resume_latency);
blocking_notifier_call_chain(&dev_pm_notifiers,
@@ -153,7 +153,7 @@ static int apply_constraint(struct dev_pm_qos_request *req,
break;
case DEV_PM_QOS_LATENCY_TOLERANCE:
ret = pm_qos_update_target(&qos->latency_tolerance,
- &req->data.pnode, action, value);
+ &req->data.lat, action, value);
if (ret) {
value = pm_qos_read_value(&qos->latency_tolerance);
req->dev->power.set_latency_tolerance(req->dev, value);
@@ -254,7 +254,7 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
/* Flush the constraints lists for the device. */
c = &qos->resume_latency;
- plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
+ plist_for_each_entry_safe(req, tmp, &c->list, data.lat.node) {
/*
* Update constraints list and call the notification
* callbacks if needed
@@ -263,7 +263,7 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
memset(req, 0, sizeof(*req));
}
c = &qos->latency_tolerance;
- plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
+ plist_for_each_entry_safe(req, tmp, &c->list, data.lat.node) {
apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
memset(req, 0, sizeof(*req));
}
@@ -378,7 +378,7 @@ static int __dev_pm_qos_update_request(struct dev_pm_qos_request *req,
switch(req->type) {
case DEV_PM_QOS_RESUME_LATENCY:
case DEV_PM_QOS_LATENCY_TOLERANCE:
- curr_value = req->data.pnode.prio;
+ curr_value = req->data.lat.node.prio;
break;
case DEV_PM_QOS_FLAGS:
curr_value = req->data.flr.flags;
@@ -831,8 +831,8 @@ s32 dev_pm_qos_get_user_latency_tolerance(struct device *dev)
mutex_lock(&dev_pm_qos_mtx);
ret = IS_ERR_OR_NULL(dev->power.qos)
|| !dev->power.qos->latency_tolerance_req ?
- PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT :
- dev->power.qos->latency_tolerance_req->data.pnode.prio;
+ PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT :
+ dev->power.qos->latency_tolerance_req->data.lat.node.prio;
mutex_unlock(&dev_pm_qos_mtx);
return ret;
}
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 9ab4bf7..e1b763d 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -60,7 +60,7 @@ enum dev_pm_qos_req_type {
struct dev_pm_qos_request {
enum dev_pm_qos_req_type type;
union {
- struct plist_node pnode;
+ struct pm_qos_request lat;
struct pm_qos_flags_request flr;
} data;
struct device *dev;
@@ -112,7 +112,8 @@ static inline int dev_pm_qos_request_active(struct dev_pm_qos_request *req)
return req->dev != NULL;
}
-int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
+int pm_qos_update_target(struct pm_qos_constraints *c,
+ struct pm_qos_request *req,
enum pm_qos_req_action action, int value);
bool pm_qos_update_flags(struct pm_qos_flags *pqf,
struct pm_qos_flags_request *req,
@@ -210,7 +211,7 @@ int dev_pm_qos_update_user_latency_tolerance(struct device *dev, s32 val);
static inline s32 dev_pm_qos_requested_resume_latency(struct device *dev)
{
- return dev->power.qos->resume_latency_req->data.pnode.prio;
+ return dev->power.qos->resume_latency_req->data.lat.node.prio;
}
static inline s32 dev_pm_qos_requested_flags(struct device *dev)
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 884b770..d0b9c0f 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -161,19 +161,21 @@ static inline void pm_qos_set_value(struct pm_qos_constraints *c, s32 value)
* pm_qos_update_target - manages the constraints list and calls the notifiers
* if needed
* @c: constraints data struct
- * @node: request to add to the list, to update or to remove
+ * @req: request to add to the list, to update or to remove
* @action: action to take on the constraints list
* @value: value of the request to add or update
*
* This function returns 1 if the aggregated constraint value has changed, 0
* otherwise.
*/
-int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
+int pm_qos_update_target(struct pm_qos_constraints *c,
+ struct pm_qos_request *req,
enum pm_qos_req_action action, int value)
{
unsigned long flags;
int prev_value, curr_value, new_value;
int ret;
+ struct plist_node *node = &req->node;
spin_lock_irqsave(&pm_qos_lock, flags);
prev_value = pm_qos_get_value(c);
@@ -310,7 +312,7 @@ static void __pm_qos_update_request(struct pm_qos_request *req,
if (new_value != req->node.prio)
pm_qos_update_target(
pm_qos_array[req->pm_qos_class]->constraints,
- &req->node, PM_QOS_UPDATE_REQ, new_value);
+ req, PM_QOS_UPDATE_REQ, new_value);
}
/**
@@ -355,7 +357,7 @@ void pm_qos_add_request(struct pm_qos_request *req,
INIT_DELAYED_WORK(&req->work, pm_qos_work_fn);
trace_pm_qos_add_request(pm_qos_class, value);
pm_qos_update_target(pm_qos_array[pm_qos_class]->constraints,
- &req->node, PM_QOS_ADD_REQ, value);
+ req, PM_QOS_ADD_REQ, value);
}
EXPORT_SYMBOL_GPL(pm_qos_add_request);
@@ -409,7 +411,7 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req, s32 new_value,
if (new_value != req->node.prio)
pm_qos_update_target(
pm_qos_array[req->pm_qos_class]->constraints,
- &req->node, PM_QOS_UPDATE_REQ, new_value);
+ req, PM_QOS_UPDATE_REQ, new_value);
schedule_delayed_work(&req->work, usecs_to_jiffies(timeout_us));
}
@@ -437,7 +439,7 @@ void pm_qos_remove_request(struct pm_qos_request *req)
trace_pm_qos_remove_request(req->pm_qos_class, PM_QOS_DEFAULT_VALUE);
pm_qos_update_target(pm_qos_array[req->pm_qos_class]->constraints,
- &req->node, PM_QOS_REMOVE_REQ,
+ req, PM_QOS_REMOVE_REQ,
PM_QOS_DEFAULT_VALUE);
memset(req, 0, sizeof(*req));
}
--
1.9.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 2/4] QoS: Enhance framework to support per-cpu PM QoS request
2014-08-27 20:14 ` Lina Iyer
@ 2014-08-27 20:14 ` Lina Iyer
-1 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-08-27 20:14 UTC (permalink / raw)
To: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
tglx, rjw
Cc: daniel.lezcano, Lina Iyer, Praveen Chidambaram
From: Praveen Chidambaram <pchidamb@codeaurora.org>
QoS request can be better optimized if the request can be set only for
the required cpus and not all cpus. This helps save power on other
cores, while still guaranteeing the quality of service on the desired
cores.
Add a new enumeration to specify the PM QoS request type. The enums help
specify what is the intended target cpu of the request.
Enhance the QoS constraints data structures to support target value for
each core. Requests specify if the QoS is applicable to all cores
(default) or to a selective subset of the cores or to a core(s).
Idle and interested drivers can request a PM QoS value for a constraint
across all cpus, or a specific cpu or a set of cpus. Separate APIs have
been added to request for individual cpu or a cpumask. The default
behaviour of PM QoS is maintained i.e, requests that do not specify a
type of the request will continue to be effected on all cores.
The userspace sysfs interface does not support setting cpumask of a PM
QoS request.
Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
[lina.iyer: rework, add commit text, split changes into multiple patches]
diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index a5da5c7..c129517 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -41,6 +41,15 @@ registered notifiers are called only if the target value is now different.
Clients of pm_qos need to save the returned handle for future use in other
pm_qos API functions.
+The handle is a pm_qos_request object. By default the request object sets the
+request type to PM_QOS_REQ_ALL_CORES, in which case, the PM QoS request
+applies to all cores. However, the driver can also specify a request type to
+be either of
+ PM_QOS_REQ_ALL_CORES,
+ PM_QOS_REQ_AFFINE_CORES,
+
+Specify the cpumask when type is set to PM_QOS_REQ_AFFINE_CORES.
+
void pm_qos_update_request(handle, new_target_value):
Will update the list element pointed to by the handle with the new target value
and recompute the new aggregated target, calling the notification tree if the
@@ -54,6 +63,13 @@ the request.
int pm_qos_request(param_class):
Returns the aggregated value for a given PM QoS class.
+int pm_qos_request_for_cpu(param_class, cpu):
+Returns the aggregated value for a given PM QoS class for the specified cpu.
+
+int pm_qos_request_for_cpumask(param_class, cpumask):
+Returns the aggregated value for a given PM QoS class for the specified
+cpumask.
+
int pm_qos_request_active(handle):
Returns if the request is still active, i.e. it has not been removed from a
PM QoS class constraints list.
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index e1b763d..a3aa5b5 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -9,6 +9,8 @@
#include <linux/miscdevice.h>
#include <linux/device.h>
#include <linux/workqueue.h>
+#include <linux/cpumask.h>
+#include <linux/interrupt.h>
enum {
PM_QOS_RESERVED = 0,
@@ -40,7 +42,15 @@ enum pm_qos_flags_status {
#define PM_QOS_FLAG_NO_POWER_OFF (1 << 0)
#define PM_QOS_FLAG_REMOTE_WAKEUP (1 << 1)
+enum pm_qos_req_type {
+ PM_QOS_REQ_ALL_CORES = 0,
+ PM_QOS_REQ_AFFINE_CORES,
+};
+
struct pm_qos_request {
+ enum pm_qos_req_type type;
+ struct cpumask cpus_affine;
+ /* Internal structure members */
struct plist_node node;
int pm_qos_class;
struct delayed_work work; /* for pm_qos_update_request_timeout */
@@ -80,6 +90,7 @@ enum pm_qos_type {
struct pm_qos_constraints {
struct plist_head list;
s32 target_value; /* Do not change to 64 bit */
+ s32 target_per_cpu[NR_CPUS];
s32 default_value;
s32 no_constraint_value;
enum pm_qos_type type;
@@ -127,6 +138,8 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req,
void pm_qos_remove_request(struct pm_qos_request *req);
int pm_qos_request(int pm_qos_class);
+int pm_qos_request_for_cpu(int pm_qos_class, int cpu);
+int pm_qos_request_for_cpumask(int pm_qos_class, struct cpumask *mask);
int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
int pm_qos_request_active(struct pm_qos_request *req);
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index d0b9c0f..27f84a2 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -65,6 +65,8 @@ static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
static struct pm_qos_constraints cpu_dma_constraints = {
.list = PLIST_HEAD_INIT(cpu_dma_constraints.list),
.target_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
+ .target_per_cpu = { [0 ... (NR_CPUS - 1)] =
+ PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE },
.default_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
.no_constraint_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
.type = PM_QOS_MIN,
@@ -79,6 +81,8 @@ static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
static struct pm_qos_constraints network_lat_constraints = {
.list = PLIST_HEAD_INIT(network_lat_constraints.list),
.target_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
+ .target_per_cpu = { [0 ... (NR_CPUS - 1)] =
+ PM_QOS_NETWORK_LAT_DEFAULT_VALUE },
.default_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
.no_constraint_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
.type = PM_QOS_MIN,
@@ -94,6 +98,8 @@ static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
static struct pm_qos_constraints network_tput_constraints = {
.list = PLIST_HEAD_INIT(network_tput_constraints.list),
.target_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
+ .target_per_cpu = { [0 ... (NR_CPUS - 1)] =
+ PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE },
.default_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
.no_constraint_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
.type = PM_QOS_MAX,
@@ -157,6 +163,43 @@ static inline void pm_qos_set_value(struct pm_qos_constraints *c, s32 value)
c->target_value = value;
}
+static inline void pm_qos_set_value_for_cpus(struct pm_qos_constraints *c)
+{
+ struct pm_qos_request *req = NULL;
+ int cpu;
+ s32 *qos_val;
+
+ qos_val = kcalloc(NR_CPUS, sizeof(*qos_val), GFP_KERNEL);
+ if (!qos_val) {
+ WARN("%s: No memory for PM QoS\n", __func__);
+ return;
+ }
+
+ for_each_possible_cpu(cpu)
+ qos_val[cpu] = c->default_value;
+
+ plist_for_each_entry(req, &c->list, node) {
+ for_each_cpu(cpu, &req->cpus_affine) {
+ switch (c->type) {
+ case PM_QOS_MIN:
+ if (qos_val[cpu] > req->node.prio)
+ qos_val[cpu] = req->node.prio;
+ break;
+ case PM_QOS_MAX:
+ if (req->node.prio > qos_val[cpu])
+ qos_val[cpu] = req->node.prio;
+ break;
+ default:
+ BUG();
+ break;
+ }
+ }
+ }
+
+ for_each_possible_cpu(cpu)
+ c->target_per_cpu[cpu] = qos_val[cpu];
+}
+
/**
* pm_qos_update_target - manages the constraints list and calls the notifiers
* if needed
@@ -206,6 +249,7 @@ int pm_qos_update_target(struct pm_qos_constraints *c,
curr_value = pm_qos_get_value(c);
pm_qos_set_value(c, curr_value);
+ pm_qos_set_value_for_cpus(c);
spin_unlock_irqrestore(&pm_qos_lock, flags);
@@ -298,6 +342,44 @@ int pm_qos_request(int pm_qos_class)
}
EXPORT_SYMBOL_GPL(pm_qos_request);
+int pm_qos_request_for_cpu(int pm_qos_class, int cpu)
+{
+ return pm_qos_array[pm_qos_class]->constraints->target_per_cpu[cpu];
+}
+EXPORT_SYMBOL(pm_qos_request_for_cpu);
+
+int pm_qos_request_for_cpumask(int pm_qos_class, struct cpumask *mask)
+{
+ unsigned long irqflags;
+ int cpu;
+ struct pm_qos_constraints *c = NULL;
+ int val;
+
+ spin_lock_irqsave(&pm_qos_lock, irqflags);
+ c = pm_qos_array[pm_qos_class]->constraints;
+ val = c->default_value;
+
+ for_each_cpu(cpu, mask) {
+ switch (c->type) {
+ case PM_QOS_MIN:
+ if (c->target_per_cpu[cpu] < val)
+ val = c->target_per_cpu[cpu];
+ break;
+ case PM_QOS_MAX:
+ if (c->target_per_cpu[cpu] > val)
+ val = c->target_per_cpu[cpu];
+ break;
+ default:
+ BUG();
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&pm_qos_lock, irqflags);
+
+ return val;
+}
+EXPORT_SYMBOL(pm_qos_request_for_cpumask);
+
int pm_qos_request_active(struct pm_qos_request *req)
{
return req->pm_qos_class != 0;
@@ -353,6 +435,24 @@ void pm_qos_add_request(struct pm_qos_request *req,
WARN(1, KERN_ERR "pm_qos_add_request() called for already added request\n");
return;
}
+
+ switch (req->type) {
+ case PM_QOS_REQ_AFFINE_CORES:
+ if (cpumask_empty(&req->cpus_affine)) {
+ req->type = PM_QOS_REQ_ALL_CORES;
+ cpumask_setall(&req->cpus_affine);
+ WARN(1, KERN_ERR "Affine cores not set for request with affinity flag\n");
+ }
+ break;
+
+ default:
+ WARN(1, KERN_ERR "Unknown request type %d\n", req->type);
+ /* fall through */
+ case PM_QOS_REQ_ALL_CORES:
+ cpumask_setall(&req->cpus_affine);
+ break;
+ }
+
req->pm_qos_class = pm_qos_class;
INIT_DELAYED_WORK(&req->work, pm_qos_work_fn);
trace_pm_qos_add_request(pm_qos_class, value);
@@ -426,6 +526,7 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req, s32 new_value,
*/
void pm_qos_remove_request(struct pm_qos_request *req)
{
+
if (!req) /*guard against callers passing in null */
return;
/* silent return to keep pcm code cleaner */
@@ -441,6 +542,7 @@ void pm_qos_remove_request(struct pm_qos_request *req)
pm_qos_update_target(pm_qos_array[req->pm_qos_class]->constraints,
req, PM_QOS_REMOVE_REQ,
PM_QOS_DEFAULT_VALUE);
+
memset(req, 0, sizeof(*req));
}
EXPORT_SYMBOL_GPL(pm_qos_remove_request);
--
1.9.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 2/4] QoS: Enhance framework to support per-cpu PM QoS request
@ 2014-08-27 20:14 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-08-27 20:14 UTC (permalink / raw)
To: linux-arm-kernel
From: Praveen Chidambaram <pchidamb@codeaurora.org>
QoS request can be better optimized if the request can be set only for
the required cpus and not all cpus. This helps save power on other
cores, while still guaranteeing the quality of service on the desired
cores.
Add a new enumeration to specify the PM QoS request type. The enums help
specify what is the intended target cpu of the request.
Enhance the QoS constraints data structures to support target value for
each core. Requests specify if the QoS is applicable to all cores
(default) or to a selective subset of the cores or to a core(s).
Idle and interested drivers can request a PM QoS value for a constraint
across all cpus, or a specific cpu or a set of cpus. Separate APIs have
been added to request for individual cpu or a cpumask. The default
behaviour of PM QoS is maintained i.e, requests that do not specify a
type of the request will continue to be effected on all cores.
The userspace sysfs interface does not support setting cpumask of a PM
QoS request.
Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
[lina.iyer: rework, add commit text, split changes into multiple patches]
diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index a5da5c7..c129517 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -41,6 +41,15 @@ registered notifiers are called only if the target value is now different.
Clients of pm_qos need to save the returned handle for future use in other
pm_qos API functions.
+The handle is a pm_qos_request object. By default the request object sets the
+request type to PM_QOS_REQ_ALL_CORES, in which case, the PM QoS request
+applies to all cores. However, the driver can also specify a request type to
+be either of
+ PM_QOS_REQ_ALL_CORES,
+ PM_QOS_REQ_AFFINE_CORES,
+
+Specify the cpumask when type is set to PM_QOS_REQ_AFFINE_CORES.
+
void pm_qos_update_request(handle, new_target_value):
Will update the list element pointed to by the handle with the new target value
and recompute the new aggregated target, calling the notification tree if the
@@ -54,6 +63,13 @@ the request.
int pm_qos_request(param_class):
Returns the aggregated value for a given PM QoS class.
+int pm_qos_request_for_cpu(param_class, cpu):
+Returns the aggregated value for a given PM QoS class for the specified cpu.
+
+int pm_qos_request_for_cpumask(param_class, cpumask):
+Returns the aggregated value for a given PM QoS class for the specified
+cpumask.
+
int pm_qos_request_active(handle):
Returns if the request is still active, i.e. it has not been removed from a
PM QoS class constraints list.
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index e1b763d..a3aa5b5 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -9,6 +9,8 @@
#include <linux/miscdevice.h>
#include <linux/device.h>
#include <linux/workqueue.h>
+#include <linux/cpumask.h>
+#include <linux/interrupt.h>
enum {
PM_QOS_RESERVED = 0,
@@ -40,7 +42,15 @@ enum pm_qos_flags_status {
#define PM_QOS_FLAG_NO_POWER_OFF (1 << 0)
#define PM_QOS_FLAG_REMOTE_WAKEUP (1 << 1)
+enum pm_qos_req_type {
+ PM_QOS_REQ_ALL_CORES = 0,
+ PM_QOS_REQ_AFFINE_CORES,
+};
+
struct pm_qos_request {
+ enum pm_qos_req_type type;
+ struct cpumask cpus_affine;
+ /* Internal structure members */
struct plist_node node;
int pm_qos_class;
struct delayed_work work; /* for pm_qos_update_request_timeout */
@@ -80,6 +90,7 @@ enum pm_qos_type {
struct pm_qos_constraints {
struct plist_head list;
s32 target_value; /* Do not change to 64 bit */
+ s32 target_per_cpu[NR_CPUS];
s32 default_value;
s32 no_constraint_value;
enum pm_qos_type type;
@@ -127,6 +138,8 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req,
void pm_qos_remove_request(struct pm_qos_request *req);
int pm_qos_request(int pm_qos_class);
+int pm_qos_request_for_cpu(int pm_qos_class, int cpu);
+int pm_qos_request_for_cpumask(int pm_qos_class, struct cpumask *mask);
int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
int pm_qos_request_active(struct pm_qos_request *req);
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index d0b9c0f..27f84a2 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -65,6 +65,8 @@ static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
static struct pm_qos_constraints cpu_dma_constraints = {
.list = PLIST_HEAD_INIT(cpu_dma_constraints.list),
.target_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
+ .target_per_cpu = { [0 ... (NR_CPUS - 1)] =
+ PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE },
.default_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
.no_constraint_value = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE,
.type = PM_QOS_MIN,
@@ -79,6 +81,8 @@ static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
static struct pm_qos_constraints network_lat_constraints = {
.list = PLIST_HEAD_INIT(network_lat_constraints.list),
.target_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
+ .target_per_cpu = { [0 ... (NR_CPUS - 1)] =
+ PM_QOS_NETWORK_LAT_DEFAULT_VALUE },
.default_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
.no_constraint_value = PM_QOS_NETWORK_LAT_DEFAULT_VALUE,
.type = PM_QOS_MIN,
@@ -94,6 +98,8 @@ static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
static struct pm_qos_constraints network_tput_constraints = {
.list = PLIST_HEAD_INIT(network_tput_constraints.list),
.target_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
+ .target_per_cpu = { [0 ... (NR_CPUS - 1)] =
+ PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE },
.default_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
.no_constraint_value = PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE,
.type = PM_QOS_MAX,
@@ -157,6 +163,43 @@ static inline void pm_qos_set_value(struct pm_qos_constraints *c, s32 value)
c->target_value = value;
}
+static inline void pm_qos_set_value_for_cpus(struct pm_qos_constraints *c)
+{
+ struct pm_qos_request *req = NULL;
+ int cpu;
+ s32 *qos_val;
+
+ qos_val = kcalloc(NR_CPUS, sizeof(*qos_val), GFP_KERNEL);
+ if (!qos_val) {
+ WARN("%s: No memory for PM QoS\n", __func__);
+ return;
+ }
+
+ for_each_possible_cpu(cpu)
+ qos_val[cpu] = c->default_value;
+
+ plist_for_each_entry(req, &c->list, node) {
+ for_each_cpu(cpu, &req->cpus_affine) {
+ switch (c->type) {
+ case PM_QOS_MIN:
+ if (qos_val[cpu] > req->node.prio)
+ qos_val[cpu] = req->node.prio;
+ break;
+ case PM_QOS_MAX:
+ if (req->node.prio > qos_val[cpu])
+ qos_val[cpu] = req->node.prio;
+ break;
+ default:
+ BUG();
+ break;
+ }
+ }
+ }
+
+ for_each_possible_cpu(cpu)
+ c->target_per_cpu[cpu] = qos_val[cpu];
+}
+
/**
* pm_qos_update_target - manages the constraints list and calls the notifiers
* if needed
@@ -206,6 +249,7 @@ int pm_qos_update_target(struct pm_qos_constraints *c,
curr_value = pm_qos_get_value(c);
pm_qos_set_value(c, curr_value);
+ pm_qos_set_value_for_cpus(c);
spin_unlock_irqrestore(&pm_qos_lock, flags);
@@ -298,6 +342,44 @@ int pm_qos_request(int pm_qos_class)
}
EXPORT_SYMBOL_GPL(pm_qos_request);
+int pm_qos_request_for_cpu(int pm_qos_class, int cpu)
+{
+ return pm_qos_array[pm_qos_class]->constraints->target_per_cpu[cpu];
+}
+EXPORT_SYMBOL(pm_qos_request_for_cpu);
+
+int pm_qos_request_for_cpumask(int pm_qos_class, struct cpumask *mask)
+{
+ unsigned long irqflags;
+ int cpu;
+ struct pm_qos_constraints *c = NULL;
+ int val;
+
+ spin_lock_irqsave(&pm_qos_lock, irqflags);
+ c = pm_qos_array[pm_qos_class]->constraints;
+ val = c->default_value;
+
+ for_each_cpu(cpu, mask) {
+ switch (c->type) {
+ case PM_QOS_MIN:
+ if (c->target_per_cpu[cpu] < val)
+ val = c->target_per_cpu[cpu];
+ break;
+ case PM_QOS_MAX:
+ if (c->target_per_cpu[cpu] > val)
+ val = c->target_per_cpu[cpu];
+ break;
+ default:
+ BUG();
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&pm_qos_lock, irqflags);
+
+ return val;
+}
+EXPORT_SYMBOL(pm_qos_request_for_cpumask);
+
int pm_qos_request_active(struct pm_qos_request *req)
{
return req->pm_qos_class != 0;
@@ -353,6 +435,24 @@ void pm_qos_add_request(struct pm_qos_request *req,
WARN(1, KERN_ERR "pm_qos_add_request() called for already added request\n");
return;
}
+
+ switch (req->type) {
+ case PM_QOS_REQ_AFFINE_CORES:
+ if (cpumask_empty(&req->cpus_affine)) {
+ req->type = PM_QOS_REQ_ALL_CORES;
+ cpumask_setall(&req->cpus_affine);
+ WARN(1, KERN_ERR "Affine cores not set for request with affinity flag\n");
+ }
+ break;
+
+ default:
+ WARN(1, KERN_ERR "Unknown request type %d\n", req->type);
+ /* fall through */
+ case PM_QOS_REQ_ALL_CORES:
+ cpumask_setall(&req->cpus_affine);
+ break;
+ }
+
req->pm_qos_class = pm_qos_class;
INIT_DELAYED_WORK(&req->work, pm_qos_work_fn);
trace_pm_qos_add_request(pm_qos_class, value);
@@ -426,6 +526,7 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req, s32 new_value,
*/
void pm_qos_remove_request(struct pm_qos_request *req)
{
+
if (!req) /*guard against callers passing in null */
return;
/* silent return to keep pcm code cleaner */
@@ -441,6 +542,7 @@ void pm_qos_remove_request(struct pm_qos_request *req)
pm_qos_update_target(pm_qos_array[req->pm_qos_class]->constraints,
req, PM_QOS_REMOVE_REQ,
PM_QOS_DEFAULT_VALUE);
+
memset(req, 0, sizeof(*req));
}
EXPORT_SYMBOL_GPL(pm_qos_remove_request);
--
1.9.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-08-27 20:14 ` Lina Iyer
@ 2014-08-27 20:14 ` Lina Iyer
-1 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-08-27 20:14 UTC (permalink / raw)
To: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
tglx, rjw
Cc: daniel.lezcano, Lina Iyer
PM QoS and other idle frameworks can do a better job of addressing power
and performance requirements for a cpu, knowing the IRQs that are
affine to that cpu. If a performance request is placed against serving
the IRQ faster and if the IRQ is affine to a set of cpus, then setting
the performance requirements only on those cpus help save power on the
rest of the cpus. PM QoS framework is one such framework interested in
knowing the smp_affinity of an IRQ and the change notificiation in this
regard. QoS requests for the CPU_DMA_LATENCY constraint currently apply
to all cpus, but when attached to an IRQ, can be applied only to the set
of cpus that IRQ's smp_affinity is set to. This allows other cpus to
enter deeper sleep states to save power. More than one framework/driver
can be interested in such information.
The current implementation allows only a single notification callback
whenever the IRQ's SMP affinity is changed. Adding a second notification
punts the existing notifier function out of registration. Add a list of
notifiers, allowing multiple clients to register for irq affinity
notifications.
The kref object associated with the struct irq_affinity_notify was used
to prevent the notifier object from being released if there is a pending
notification. It was incremented before the work item was scheduled and
was decremented when the notification was completed. If the kref count
was zero at the end of it, the release function gets a callback allowing
the module to release the irq_affinity_notify memory. This works well
for a single notification. When multiple clients are registered, no
single kref object can be used. Hence, the work function when scheduled,
will increase the kref count using the kref_get_unless_zero(), so if the
module had already unregistered the irq_affinity_notify object while the
work function was scheduled, it will not be notified.
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
diff --git a/drivers/infiniband/hw/qib/qib_iba7322.c b/drivers/infiniband/hw/qib/qib_iba7322.c
index a7eb325..62cb77d 100644
--- a/drivers/infiniband/hw/qib/qib_iba7322.c
+++ b/drivers/infiniband/hw/qib/qib_iba7322.c
@@ -3345,9 +3345,7 @@ static void reset_dca_notifier(struct qib_devdata *dd, struct qib_msix_entry *m)
"Disabling notifier on HCA %d irq %d\n",
dd->unit,
m->msix.vector);
- irq_set_affinity_notifier(
- m->msix.vector,
- NULL);
+ irq_release_affinity_notifier(m->notifier);
m->notifier = NULL;
}
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 698ad05..c1e227c 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -203,7 +203,7 @@ static inline int check_wakeup_irqs(void) { return 0; }
* struct irq_affinity_notify - context for notification of IRQ affinity changes
* @irq: Interrupt to which notification applies
* @kref: Reference count, for internal use
- * @work: Work item, for internal use
+ * @list: Add to the notifier list, for internal use
* @notify: Function to be called on change. This will be
* called in process context.
* @release: Function to be called on release. This will be
@@ -214,7 +214,7 @@ static inline int check_wakeup_irqs(void) { return 0; }
struct irq_affinity_notify {
unsigned int irq;
struct kref kref;
- struct work_struct work;
+ struct list_head list;
void (*notify)(struct irq_affinity_notify *, const cpumask_t *mask);
void (*release)(struct kref *ref);
};
@@ -265,6 +265,8 @@ extern int irq_set_affinity_hint(unsigned int irq, const struct cpumask *m);
extern int
irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify);
+extern int
+irq_release_affinity_notifier(struct irq_affinity_notify *notify);
#else /* CONFIG_SMP */
static inline int irq_set_affinity(unsigned int irq, const struct cpumask *m)
@@ -295,6 +297,12 @@ irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
{
return 0;
}
+
+static inline int
+irq_release_affinity_notifier(struct irq_affinity_notify *notify)
+{
+ return 0;
+}
#endif /* CONFIG_SMP */
/*
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 472c021..db3509e 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -31,7 +31,8 @@ struct irq_desc;
* @threads_handled_last: comparator field for deferred spurious detection of theraded handlers
* @lock: locking for SMP
* @affinity_hint: hint to user space for preferred irq affinity
- * @affinity_notify: context for notification of affinity changes
+ * @affinity_notify: list of notification clients for affinity changes
+ * @affinity_work: Work queue for handling affinity change notifications
* @pending_mask: pending rebalanced interrupts
* @threads_oneshot: bitfield to handle shared oneshot threads
* @threads_active: number of irqaction threads currently running
@@ -60,7 +61,8 @@ struct irq_desc {
struct cpumask *percpu_enabled;
#ifdef CONFIG_SMP
const struct cpumask *affinity_hint;
- struct irq_affinity_notify *affinity_notify;
+ struct list_head affinity_notify;
+ struct work_struct affinity_work;
#ifdef CONFIG_GENERIC_PENDING_IRQ
cpumask_var_t pending_mask;
#endif
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 1487a12..c95e1f3 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -91,6 +91,7 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
for_each_possible_cpu(cpu)
*per_cpu_ptr(desc->kstat_irqs, cpu) = 0;
desc_smp_init(desc, node);
+ INIT_LIST_HEAD(&desc->affinity_notify);
}
int nr_irqs = NR_IRQS;
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 3dc6a61..b6ff79c 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -209,10 +209,9 @@ int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
irq_copy_pending(desc, mask);
}
- if (desc->affinity_notify) {
- kref_get(&desc->affinity_notify->kref);
- schedule_work(&desc->affinity_notify->work);
- }
+ if (!list_empty(&desc->affinity_notify))
+ schedule_work(&desc->affinity_work);
+
irqd_set(data, IRQD_AFFINITY_SET);
return ret;
@@ -248,14 +247,14 @@ EXPORT_SYMBOL_GPL(irq_set_affinity_hint);
static void irq_affinity_notify(struct work_struct *work)
{
- struct irq_affinity_notify *notify =
- container_of(work, struct irq_affinity_notify, work);
- struct irq_desc *desc = irq_to_desc(notify->irq);
+ struct irq_desc *desc =
+ container_of(work, struct irq_desc, affinity_work);
cpumask_var_t cpumask;
unsigned long flags;
+ struct irq_affinity_notify *notify;
if (!desc || !alloc_cpumask_var(&cpumask, GFP_KERNEL))
- goto out;
+ return;
raw_spin_lock_irqsave(&desc->lock, flags);
if (irq_move_pending(&desc->irq_data))
@@ -264,11 +263,20 @@ static void irq_affinity_notify(struct work_struct *work)
cpumask_copy(cpumask, desc->irq_data.affinity);
raw_spin_unlock_irqrestore(&desc->lock, flags);
- notify->notify(notify, cpumask);
+ list_for_each_entry(notify, &desc->affinity_notify, list) {
+ /**
+ * Check and get the kref only if the kref has not been
+ * released by now. Its possible that the reference count
+ * is already 0, we dont want to notify those if they are
+ * already released.
+ */
+ if (!kref_get_unless_zero(¬ify->kref))
+ continue;
+ notify->notify(notify, cpumask);
+ kref_put(¬ify->kref, notify->release);
+ }
free_cpumask_var(cpumask);
-out:
- kref_put(¬ify->kref, notify->release);
}
/**
@@ -286,34 +294,50 @@ int
irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
{
struct irq_desc *desc = irq_to_desc(irq);
- struct irq_affinity_notify *old_notify;
unsigned long flags;
- /* The release function is promised process context */
- might_sleep();
-
if (!desc)
return -EINVAL;
- /* Complete initialisation of *notify */
- if (notify) {
- notify->irq = irq;
- kref_init(¬ify->kref);
- INIT_WORK(¬ify->work, irq_affinity_notify);
+ if (!notify) {
+ WARN("%s called with NULL notifier - use irq_release_affinity_notifier function instead.\n",
+ __func__);
+ return -EINVAL;
}
+ notify->irq = irq;
+ kref_init(¬ify->kref);
+ INIT_LIST_HEAD(¬ify->list);
raw_spin_lock_irqsave(&desc->lock, flags);
- old_notify = desc->affinity_notify;
- desc->affinity_notify = notify;
+ list_add(¬ify->list, &desc->affinity_notify);
raw_spin_unlock_irqrestore(&desc->lock, flags);
- if (old_notify)
- kref_put(&old_notify->kref, old_notify->release);
-
return 0;
}
EXPORT_SYMBOL_GPL(irq_set_affinity_notifier);
+/**
+ * irq_release_affinity_notifier - Remove us from notifications
+ * @notify: Context for notification
+ */
+int irq_release_affinity_notifier(struct irq_affinity_notify *notify)
+{
+ struct irq_desc *desc;
+ unsigned long flags;
+
+ if (!notify)
+ return -EINVAL;
+
+ desc = irq_to_desc(notify->irq);
+ raw_spin_lock_irqsave(&desc->lock, flags);
+ list_del(¬ify->list);
+ raw_spin_unlock_irqrestore(&desc->lock, flags);
+ kref_put(¬ify->kref, notify->release);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(irq_release_affinity_notifier);
+
#ifndef CONFIG_AUTO_IRQ_AFFINITY
/*
* Generic version of the affinity autoselector.
@@ -348,6 +372,8 @@ setup_affinity(unsigned int irq, struct irq_desc *desc, struct cpumask *mask)
if (cpumask_intersects(mask, nodemask))
cpumask_and(mask, mask, nodemask);
}
+ INIT_LIST_HEAD(&desc->affinity_notify);
+ INIT_WORK(&desc->affinity_work, irq_affinity_notify);
irq_do_set_affinity(&desc->irq_data, mask, false);
return 0;
}
@@ -1413,14 +1439,15 @@ EXPORT_SYMBOL_GPL(remove_irq);
void free_irq(unsigned int irq, void *dev_id)
{
struct irq_desc *desc = irq_to_desc(irq);
+ struct irq_affinity_notify *notify;
if (!desc || WARN_ON(irq_settings_is_per_cpu_devid(desc)))
return;
-#ifdef CONFIG_SMP
- if (WARN_ON(desc->affinity_notify))
- desc->affinity_notify = NULL;
-#endif
+ WARN_ON(!list_empty(&desc->affinity_notify));
+
+ list_for_each_entry(notify, &desc->affinity_notify, list)
+ kref_put(¬ify->kref, notify->release);
chip_bus_lock(desc);
kfree(__free_irq(irq, dev_id));
diff --git a/lib/cpu_rmap.c b/lib/cpu_rmap.c
index 4f134d8..0c8da50 100644
--- a/lib/cpu_rmap.c
+++ b/lib/cpu_rmap.c
@@ -235,7 +235,7 @@ void free_irq_cpu_rmap(struct cpu_rmap *rmap)
for (index = 0; index < rmap->used; index++) {
glue = rmap->obj[index];
- irq_set_affinity_notifier(glue->notify.irq, NULL);
+ irq_release_affinity_notifier(&glue->notify);
}
cpu_rmap_put(rmap);
--
1.9.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-08-27 20:14 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-08-27 20:14 UTC (permalink / raw)
To: linux-arm-kernel
PM QoS and other idle frameworks can do a better job of addressing power
and performance requirements for a cpu, knowing the IRQs that are
affine to that cpu. If a performance request is placed against serving
the IRQ faster and if the IRQ is affine to a set of cpus, then setting
the performance requirements only on those cpus help save power on the
rest of the cpus. PM QoS framework is one such framework interested in
knowing the smp_affinity of an IRQ and the change notificiation in this
regard. QoS requests for the CPU_DMA_LATENCY constraint currently apply
to all cpus, but when attached to an IRQ, can be applied only to the set
of cpus that IRQ's smp_affinity is set to. This allows other cpus to
enter deeper sleep states to save power. More than one framework/driver
can be interested in such information.
The current implementation allows only a single notification callback
whenever the IRQ's SMP affinity is changed. Adding a second notification
punts the existing notifier function out of registration. Add a list of
notifiers, allowing multiple clients to register for irq affinity
notifications.
The kref object associated with the struct irq_affinity_notify was used
to prevent the notifier object from being released if there is a pending
notification. It was incremented before the work item was scheduled and
was decremented when the notification was completed. If the kref count
was zero at the end of it, the release function gets a callback allowing
the module to release the irq_affinity_notify memory. This works well
for a single notification. When multiple clients are registered, no
single kref object can be used. Hence, the work function when scheduled,
will increase the kref count using the kref_get_unless_zero(), so if the
module had already unregistered the irq_affinity_notify object while the
work function was scheduled, it will not be notified.
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
diff --git a/drivers/infiniband/hw/qib/qib_iba7322.c b/drivers/infiniband/hw/qib/qib_iba7322.c
index a7eb325..62cb77d 100644
--- a/drivers/infiniband/hw/qib/qib_iba7322.c
+++ b/drivers/infiniband/hw/qib/qib_iba7322.c
@@ -3345,9 +3345,7 @@ static void reset_dca_notifier(struct qib_devdata *dd, struct qib_msix_entry *m)
"Disabling notifier on HCA %d irq %d\n",
dd->unit,
m->msix.vector);
- irq_set_affinity_notifier(
- m->msix.vector,
- NULL);
+ irq_release_affinity_notifier(m->notifier);
m->notifier = NULL;
}
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 698ad05..c1e227c 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -203,7 +203,7 @@ static inline int check_wakeup_irqs(void) { return 0; }
* struct irq_affinity_notify - context for notification of IRQ affinity changes
* @irq: Interrupt to which notification applies
* @kref: Reference count, for internal use
- * @work: Work item, for internal use
+ * @list: Add to the notifier list, for internal use
* @notify: Function to be called on change. This will be
* called in process context.
* @release: Function to be called on release. This will be
@@ -214,7 +214,7 @@ static inline int check_wakeup_irqs(void) { return 0; }
struct irq_affinity_notify {
unsigned int irq;
struct kref kref;
- struct work_struct work;
+ struct list_head list;
void (*notify)(struct irq_affinity_notify *, const cpumask_t *mask);
void (*release)(struct kref *ref);
};
@@ -265,6 +265,8 @@ extern int irq_set_affinity_hint(unsigned int irq, const struct cpumask *m);
extern int
irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify);
+extern int
+irq_release_affinity_notifier(struct irq_affinity_notify *notify);
#else /* CONFIG_SMP */
static inline int irq_set_affinity(unsigned int irq, const struct cpumask *m)
@@ -295,6 +297,12 @@ irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
{
return 0;
}
+
+static inline int
+irq_release_affinity_notifier(struct irq_affinity_notify *notify)
+{
+ return 0;
+}
#endif /* CONFIG_SMP */
/*
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 472c021..db3509e 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -31,7 +31,8 @@ struct irq_desc;
* @threads_handled_last: comparator field for deferred spurious detection of theraded handlers
* @lock: locking for SMP
* @affinity_hint: hint to user space for preferred irq affinity
- * @affinity_notify: context for notification of affinity changes
+ * @affinity_notify: list of notification clients for affinity changes
+ * @affinity_work: Work queue for handling affinity change notifications
* @pending_mask: pending rebalanced interrupts
* @threads_oneshot: bitfield to handle shared oneshot threads
* @threads_active: number of irqaction threads currently running
@@ -60,7 +61,8 @@ struct irq_desc {
struct cpumask *percpu_enabled;
#ifdef CONFIG_SMP
const struct cpumask *affinity_hint;
- struct irq_affinity_notify *affinity_notify;
+ struct list_head affinity_notify;
+ struct work_struct affinity_work;
#ifdef CONFIG_GENERIC_PENDING_IRQ
cpumask_var_t pending_mask;
#endif
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 1487a12..c95e1f3 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -91,6 +91,7 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
for_each_possible_cpu(cpu)
*per_cpu_ptr(desc->kstat_irqs, cpu) = 0;
desc_smp_init(desc, node);
+ INIT_LIST_HEAD(&desc->affinity_notify);
}
int nr_irqs = NR_IRQS;
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 3dc6a61..b6ff79c 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -209,10 +209,9 @@ int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
irq_copy_pending(desc, mask);
}
- if (desc->affinity_notify) {
- kref_get(&desc->affinity_notify->kref);
- schedule_work(&desc->affinity_notify->work);
- }
+ if (!list_empty(&desc->affinity_notify))
+ schedule_work(&desc->affinity_work);
+
irqd_set(data, IRQD_AFFINITY_SET);
return ret;
@@ -248,14 +247,14 @@ EXPORT_SYMBOL_GPL(irq_set_affinity_hint);
static void irq_affinity_notify(struct work_struct *work)
{
- struct irq_affinity_notify *notify =
- container_of(work, struct irq_affinity_notify, work);
- struct irq_desc *desc = irq_to_desc(notify->irq);
+ struct irq_desc *desc =
+ container_of(work, struct irq_desc, affinity_work);
cpumask_var_t cpumask;
unsigned long flags;
+ struct irq_affinity_notify *notify;
if (!desc || !alloc_cpumask_var(&cpumask, GFP_KERNEL))
- goto out;
+ return;
raw_spin_lock_irqsave(&desc->lock, flags);
if (irq_move_pending(&desc->irq_data))
@@ -264,11 +263,20 @@ static void irq_affinity_notify(struct work_struct *work)
cpumask_copy(cpumask, desc->irq_data.affinity);
raw_spin_unlock_irqrestore(&desc->lock, flags);
- notify->notify(notify, cpumask);
+ list_for_each_entry(notify, &desc->affinity_notify, list) {
+ /**
+ * Check and get the kref only if the kref has not been
+ * released by now. Its possible that the reference count
+ * is already 0, we dont want to notify those if they are
+ * already released.
+ */
+ if (!kref_get_unless_zero(¬ify->kref))
+ continue;
+ notify->notify(notify, cpumask);
+ kref_put(¬ify->kref, notify->release);
+ }
free_cpumask_var(cpumask);
-out:
- kref_put(¬ify->kref, notify->release);
}
/**
@@ -286,34 +294,50 @@ int
irq_set_affinity_notifier(unsigned int irq, struct irq_affinity_notify *notify)
{
struct irq_desc *desc = irq_to_desc(irq);
- struct irq_affinity_notify *old_notify;
unsigned long flags;
- /* The release function is promised process context */
- might_sleep();
-
if (!desc)
return -EINVAL;
- /* Complete initialisation of *notify */
- if (notify) {
- notify->irq = irq;
- kref_init(¬ify->kref);
- INIT_WORK(¬ify->work, irq_affinity_notify);
+ if (!notify) {
+ WARN("%s called with NULL notifier - use irq_release_affinity_notifier function instead.\n",
+ __func__);
+ return -EINVAL;
}
+ notify->irq = irq;
+ kref_init(¬ify->kref);
+ INIT_LIST_HEAD(¬ify->list);
raw_spin_lock_irqsave(&desc->lock, flags);
- old_notify = desc->affinity_notify;
- desc->affinity_notify = notify;
+ list_add(¬ify->list, &desc->affinity_notify);
raw_spin_unlock_irqrestore(&desc->lock, flags);
- if (old_notify)
- kref_put(&old_notify->kref, old_notify->release);
-
return 0;
}
EXPORT_SYMBOL_GPL(irq_set_affinity_notifier);
+/**
+ * irq_release_affinity_notifier - Remove us from notifications
+ * @notify: Context for notification
+ */
+int irq_release_affinity_notifier(struct irq_affinity_notify *notify)
+{
+ struct irq_desc *desc;
+ unsigned long flags;
+
+ if (!notify)
+ return -EINVAL;
+
+ desc = irq_to_desc(notify->irq);
+ raw_spin_lock_irqsave(&desc->lock, flags);
+ list_del(¬ify->list);
+ raw_spin_unlock_irqrestore(&desc->lock, flags);
+ kref_put(¬ify->kref, notify->release);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(irq_release_affinity_notifier);
+
#ifndef CONFIG_AUTO_IRQ_AFFINITY
/*
* Generic version of the affinity autoselector.
@@ -348,6 +372,8 @@ setup_affinity(unsigned int irq, struct irq_desc *desc, struct cpumask *mask)
if (cpumask_intersects(mask, nodemask))
cpumask_and(mask, mask, nodemask);
}
+ INIT_LIST_HEAD(&desc->affinity_notify);
+ INIT_WORK(&desc->affinity_work, irq_affinity_notify);
irq_do_set_affinity(&desc->irq_data, mask, false);
return 0;
}
@@ -1413,14 +1439,15 @@ EXPORT_SYMBOL_GPL(remove_irq);
void free_irq(unsigned int irq, void *dev_id)
{
struct irq_desc *desc = irq_to_desc(irq);
+ struct irq_affinity_notify *notify;
if (!desc || WARN_ON(irq_settings_is_per_cpu_devid(desc)))
return;
-#ifdef CONFIG_SMP
- if (WARN_ON(desc->affinity_notify))
- desc->affinity_notify = NULL;
-#endif
+ WARN_ON(!list_empty(&desc->affinity_notify));
+
+ list_for_each_entry(notify, &desc->affinity_notify, list)
+ kref_put(¬ify->kref, notify->release);
chip_bus_lock(desc);
kfree(__free_irq(irq, dev_id));
diff --git a/lib/cpu_rmap.c b/lib/cpu_rmap.c
index 4f134d8..0c8da50 100644
--- a/lib/cpu_rmap.c
+++ b/lib/cpu_rmap.c
@@ -235,7 +235,7 @@ void free_irq_cpu_rmap(struct cpu_rmap *rmap)
for (index = 0; index < rmap->used; index++) {
glue = rmap->obj[index];
- irq_set_affinity_notifier(glue->notify.irq, NULL);
+ irq_release_affinity_notifier(&glue->notify);
}
cpu_rmap_put(rmap);
--
1.9.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 4/4] QoS: Enable PM QoS requests to apply only on smp_affinity of an IRQ
2014-08-27 20:14 ` Lina Iyer
@ 2014-08-27 20:14 ` Lina Iyer
-1 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-08-27 20:14 UTC (permalink / raw)
To: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
tglx, rjw
Cc: daniel.lezcano, Lina Iyer, Praveen Chidambaram
Based on original work by pchidamb@codeaurora.org.
QoS requests that need to track an IRQ can be set to apply only on the
cpus to which the IRQ's smp_affinity attribute is set to. The PM QoS
framework will automatically track IRQ migration between the cores. The
QoS is updated to be applied only to the core(s) that the IRQ has been
migrated to.
The userspace sysfs interface does not support IRQ affinity.
Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
[lina.iyer: Split the change from a previous change, add commit text]
diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index c129517..32b864d 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -47,8 +47,10 @@ applies to all cores. However, the driver can also specify a request type to
be either of
PM_QOS_REQ_ALL_CORES,
PM_QOS_REQ_AFFINE_CORES,
+ PM_QOS_REQ_AFFINE_IRQ,
-Specify the cpumask when type is set to PM_QOS_REQ_AFFINE_CORES.
+Specify the cpumask when type is set to PM_QOS_REQ_AFFINE_CORES and specify
+the IRQ number with PM_QOS_REQ_AFFINE_IRQ.
void pm_qos_update_request(handle, new_target_value):
Will update the list element pointed to by the handle with the new target value
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index a3aa5b5..68b16b8 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -11,6 +11,7 @@
#include <linux/workqueue.h>
#include <linux/cpumask.h>
#include <linux/interrupt.h>
+#include <linux/completion.h>
enum {
PM_QOS_RESERVED = 0,
@@ -45,12 +46,16 @@ enum pm_qos_flags_status {
enum pm_qos_req_type {
PM_QOS_REQ_ALL_CORES = 0,
PM_QOS_REQ_AFFINE_CORES,
+ PM_QOS_REQ_AFFINE_IRQ,
};
struct pm_qos_request {
enum pm_qos_req_type type;
struct cpumask cpus_affine;
+ uint32_t irq;
/* Internal structure members */
+ struct irq_affinity_notify irq_notify;
+ struct completion irq_released;
struct plist_node node;
int pm_qos_class;
struct delayed_work work; /* for pm_qos_update_request_timeout */
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 27f84a2..c10e8bc 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -41,6 +41,9 @@
#include <linux/platform_device.h>
#include <linux/init.h>
#include <linux/kernel.h>
+#include <linux/irq.h>
+#include <linux/irqdesc.h>
+#include <linux/delay.h>
#include <linux/uaccess.h>
#include <linux/export.h>
@@ -412,6 +415,37 @@ static void pm_qos_work_fn(struct work_struct *work)
__pm_qos_update_request(req, PM_QOS_DEFAULT_VALUE);
}
+static void pm_qos_irq_release(struct kref *ref)
+{
+ unsigned long flags;
+ struct irq_affinity_notify *notify = container_of(ref,
+ struct irq_affinity_notify, kref);
+ struct pm_qos_request *req = container_of(notify,
+ struct pm_qos_request, irq_notify);
+
+ spin_lock_irqsave(&pm_qos_lock, flags);
+ cpumask_clear(&req->cpus_affine);
+ spin_unlock_irqrestore(&pm_qos_lock, flags);
+
+ complete(&req->irq_released);
+}
+
+static void pm_qos_irq_notify(struct irq_affinity_notify *notify,
+ const cpumask_t *mask)
+{
+ unsigned long flags;
+ struct pm_qos_request *req = container_of(notify,
+ struct pm_qos_request, irq_notify);
+ struct pm_qos_constraints *c =
+ pm_qos_array[req->pm_qos_class]->constraints;
+
+ spin_lock_irqsave(&pm_qos_lock, flags);
+ cpumask_copy(&req->cpus_affine, mask);
+ spin_unlock_irqrestore(&pm_qos_lock, flags);
+
+ pm_qos_update_target(c, req, PM_QOS_UPDATE_REQ, req->node.prio);
+}
+
/**
* pm_qos_add_request - inserts new qos request into the list
* @req: pointer to a preallocated handle
@@ -445,6 +479,34 @@ void pm_qos_add_request(struct pm_qos_request *req,
}
break;
+ case PM_QOS_REQ_AFFINE_IRQ:
+ if (irq_can_set_affinity(req->irq)) {
+ int ret = 0;
+ struct irq_desc *desc = irq_to_desc(req->irq);
+ struct cpumask *mask = desc->irq_data.affinity;
+
+ /* Get the current affinity */
+ cpumask_copy(&req->cpus_affine, mask);
+ req->irq_notify.irq = req->irq;
+ req->irq_notify.notify = pm_qos_irq_notify;
+ req->irq_notify.release = pm_qos_irq_release;
+
+ ret = irq_set_affinity_notifier(req->irq,
+ &req->irq_notify);
+ if (ret) {
+ WARN(1, KERN_ERR "IRQ affinity notify set failed\n");
+ req->type = PM_QOS_REQ_ALL_CORES;
+ cpumask_setall(&req->cpus_affine);
+ }
+ } else {
+ req->type = PM_QOS_REQ_ALL_CORES;
+ cpumask_setall(&req->cpus_affine);
+ WARN(1, KERN_ERR "IRQ-%d not set for request with affinity flag\n",
+ req->irq);
+ }
+ init_completion(&req->irq_released);
+ break;
+
default:
WARN(1, KERN_ERR "Unknown request type %d\n", req->type);
/* fall through */
@@ -526,11 +588,14 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req, s32 new_value,
*/
void pm_qos_remove_request(struct pm_qos_request *req)
{
-
if (!req) /*guard against callers passing in null */
return;
/* silent return to keep pcm code cleaner */
+ /* Remove ourselves from the irq notification */
+ if (req->type == PM_QOS_REQ_AFFINE_IRQ)
+ irq_release_affinity_notifier(&req->irq_notify);
+
if (!pm_qos_request_active(req)) {
WARN(1, KERN_ERR "pm_qos_remove_request() called for unknown object\n");
return;
@@ -543,6 +608,16 @@ void pm_qos_remove_request(struct pm_qos_request *req)
req, PM_QOS_REMOVE_REQ,
PM_QOS_DEFAULT_VALUE);
+ /**
+ * The 'release' callback of the notifier would not be called unless
+ * there are no active users of the irq_notify object, i.e, kref count
+ * is non-zero. This could happen if there is an active 'notify'
+ * callback happening while the pm_qos_remove request is called. Wait
+ * until the release callback clears the cpus_affine mask.
+ */
+ if (req->type == PM_QOS_REQ_AFFINE_IRQ)
+ wait_for_completion(&req->irq_released);
+
memset(req, 0, sizeof(*req));
}
EXPORT_SYMBOL_GPL(pm_qos_remove_request);
--
1.9.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 4/4] QoS: Enable PM QoS requests to apply only on smp_affinity of an IRQ
@ 2014-08-27 20:14 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-08-27 20:14 UTC (permalink / raw)
To: linux-arm-kernel
Based on original work by pchidamb at codeaurora.org.
QoS requests that need to track an IRQ can be set to apply only on the
cpus to which the IRQ's smp_affinity attribute is set to. The PM QoS
framework will automatically track IRQ migration between the cores. The
QoS is updated to be applied only to the core(s) that the IRQ has been
migrated to.
The userspace sysfs interface does not support IRQ affinity.
Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
[lina.iyer: Split the change from a previous change, add commit text]
diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index c129517..32b864d 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -47,8 +47,10 @@ applies to all cores. However, the driver can also specify a request type to
be either of
PM_QOS_REQ_ALL_CORES,
PM_QOS_REQ_AFFINE_CORES,
+ PM_QOS_REQ_AFFINE_IRQ,
-Specify the cpumask when type is set to PM_QOS_REQ_AFFINE_CORES.
+Specify the cpumask when type is set to PM_QOS_REQ_AFFINE_CORES and specify
+the IRQ number with PM_QOS_REQ_AFFINE_IRQ.
void pm_qos_update_request(handle, new_target_value):
Will update the list element pointed to by the handle with the new target value
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index a3aa5b5..68b16b8 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -11,6 +11,7 @@
#include <linux/workqueue.h>
#include <linux/cpumask.h>
#include <linux/interrupt.h>
+#include <linux/completion.h>
enum {
PM_QOS_RESERVED = 0,
@@ -45,12 +46,16 @@ enum pm_qos_flags_status {
enum pm_qos_req_type {
PM_QOS_REQ_ALL_CORES = 0,
PM_QOS_REQ_AFFINE_CORES,
+ PM_QOS_REQ_AFFINE_IRQ,
};
struct pm_qos_request {
enum pm_qos_req_type type;
struct cpumask cpus_affine;
+ uint32_t irq;
/* Internal structure members */
+ struct irq_affinity_notify irq_notify;
+ struct completion irq_released;
struct plist_node node;
int pm_qos_class;
struct delayed_work work; /* for pm_qos_update_request_timeout */
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 27f84a2..c10e8bc 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -41,6 +41,9 @@
#include <linux/platform_device.h>
#include <linux/init.h>
#include <linux/kernel.h>
+#include <linux/irq.h>
+#include <linux/irqdesc.h>
+#include <linux/delay.h>
#include <linux/uaccess.h>
#include <linux/export.h>
@@ -412,6 +415,37 @@ static void pm_qos_work_fn(struct work_struct *work)
__pm_qos_update_request(req, PM_QOS_DEFAULT_VALUE);
}
+static void pm_qos_irq_release(struct kref *ref)
+{
+ unsigned long flags;
+ struct irq_affinity_notify *notify = container_of(ref,
+ struct irq_affinity_notify, kref);
+ struct pm_qos_request *req = container_of(notify,
+ struct pm_qos_request, irq_notify);
+
+ spin_lock_irqsave(&pm_qos_lock, flags);
+ cpumask_clear(&req->cpus_affine);
+ spin_unlock_irqrestore(&pm_qos_lock, flags);
+
+ complete(&req->irq_released);
+}
+
+static void pm_qos_irq_notify(struct irq_affinity_notify *notify,
+ const cpumask_t *mask)
+{
+ unsigned long flags;
+ struct pm_qos_request *req = container_of(notify,
+ struct pm_qos_request, irq_notify);
+ struct pm_qos_constraints *c =
+ pm_qos_array[req->pm_qos_class]->constraints;
+
+ spin_lock_irqsave(&pm_qos_lock, flags);
+ cpumask_copy(&req->cpus_affine, mask);
+ spin_unlock_irqrestore(&pm_qos_lock, flags);
+
+ pm_qos_update_target(c, req, PM_QOS_UPDATE_REQ, req->node.prio);
+}
+
/**
* pm_qos_add_request - inserts new qos request into the list
* @req: pointer to a preallocated handle
@@ -445,6 +479,34 @@ void pm_qos_add_request(struct pm_qos_request *req,
}
break;
+ case PM_QOS_REQ_AFFINE_IRQ:
+ if (irq_can_set_affinity(req->irq)) {
+ int ret = 0;
+ struct irq_desc *desc = irq_to_desc(req->irq);
+ struct cpumask *mask = desc->irq_data.affinity;
+
+ /* Get the current affinity */
+ cpumask_copy(&req->cpus_affine, mask);
+ req->irq_notify.irq = req->irq;
+ req->irq_notify.notify = pm_qos_irq_notify;
+ req->irq_notify.release = pm_qos_irq_release;
+
+ ret = irq_set_affinity_notifier(req->irq,
+ &req->irq_notify);
+ if (ret) {
+ WARN(1, KERN_ERR "IRQ affinity notify set failed\n");
+ req->type = PM_QOS_REQ_ALL_CORES;
+ cpumask_setall(&req->cpus_affine);
+ }
+ } else {
+ req->type = PM_QOS_REQ_ALL_CORES;
+ cpumask_setall(&req->cpus_affine);
+ WARN(1, KERN_ERR "IRQ-%d not set for request with affinity flag\n",
+ req->irq);
+ }
+ init_completion(&req->irq_released);
+ break;
+
default:
WARN(1, KERN_ERR "Unknown request type %d\n", req->type);
/* fall through */
@@ -526,11 +588,14 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req, s32 new_value,
*/
void pm_qos_remove_request(struct pm_qos_request *req)
{
-
if (!req) /*guard against callers passing in null */
return;
/* silent return to keep pcm code cleaner */
+ /* Remove ourselves from the irq notification */
+ if (req->type == PM_QOS_REQ_AFFINE_IRQ)
+ irq_release_affinity_notifier(&req->irq_notify);
+
if (!pm_qos_request_active(req)) {
WARN(1, KERN_ERR "pm_qos_remove_request() called for unknown object\n");
return;
@@ -543,6 +608,16 @@ void pm_qos_remove_request(struct pm_qos_request *req)
req, PM_QOS_REMOVE_REQ,
PM_QOS_DEFAULT_VALUE);
+ /**
+ * The 'release' callback of the notifier would not be called unless
+ * there are no active users of the irq_notify object, i.e, kref count
+ * is non-zero. This could happen if there is an active 'notify'
+ * callback happening while the pm_qos_remove request is called. Wait
+ * until the release callback clears the cpus_affine mask.
+ */
+ if (req->type == PM_QOS_REQ_AFFINE_IRQ)
+ wait_for_completion(&req->irq_released);
+
memset(req, 0, sizeof(*req));
}
EXPORT_SYMBOL_GPL(pm_qos_remove_request);
--
1.9.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-08-27 20:14 ` Lina Iyer
@ 2014-08-27 20:56 ` Thomas Gleixner
-1 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-08-27 20:56 UTC (permalink / raw)
To: Lina Iyer
Cc: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
rjw, daniel.lezcano
On Wed, 27 Aug 2014, Lina Iyer wrote:
> PM QoS and other idle frameworks can do a better job of addressing power
> and performance requirements for a cpu, knowing the IRQs that are
> affine to that cpu. If a performance request is placed against serving
> the IRQ faster and if the IRQ is affine to a set of cpus, then setting
> the performance requirements only on those cpus help save power on the
> rest of the cpus. PM QoS framework is one such framework interested in
> knowing the smp_affinity of an IRQ and the change notificiation in this
> regard. QoS requests for the CPU_DMA_LATENCY constraint currently apply
> to all cpus, but when attached to an IRQ, can be applied only to the set
> of cpus that IRQ's smp_affinity is set to. This allows other cpus to
> enter deeper sleep states to save power. More than one framework/driver
> can be interested in such information.
All you are describing is the fact, that there is only a single
notifier possible right now, but you completely miss to describe WHY
you need multiple ones.
The existing notifier was introduced to tell the driver that the irq
affinity has changed, so it can move its buffers to the proper NUMA
node. So if that driver gets this information then it can tell the PM
QoS code that its affinity changed so that stuff can react
accordingly, right?
This is going nowhere unless you provide a proper usecase and
arguments why this is necessary. Handwaving and I want a pony
arguments are not sufficient.
Thanks,
tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-08-27 20:56 ` Thomas Gleixner
0 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-08-27 20:56 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 27 Aug 2014, Lina Iyer wrote:
> PM QoS and other idle frameworks can do a better job of addressing power
> and performance requirements for a cpu, knowing the IRQs that are
> affine to that cpu. If a performance request is placed against serving
> the IRQ faster and if the IRQ is affine to a set of cpus, then setting
> the performance requirements only on those cpus help save power on the
> rest of the cpus. PM QoS framework is one such framework interested in
> knowing the smp_affinity of an IRQ and the change notificiation in this
> regard. QoS requests for the CPU_DMA_LATENCY constraint currently apply
> to all cpus, but when attached to an IRQ, can be applied only to the set
> of cpus that IRQ's smp_affinity is set to. This allows other cpus to
> enter deeper sleep states to save power. More than one framework/driver
> can be interested in such information.
All you are describing is the fact, that there is only a single
notifier possible right now, but you completely miss to describe WHY
you need multiple ones.
The existing notifier was introduced to tell the driver that the irq
affinity has changed, so it can move its buffers to the proper NUMA
node. So if that driver gets this information then it can tell the PM
QoS code that its affinity changed so that stuff can react
accordingly, right?
This is going nowhere unless you provide a proper usecase and
arguments why this is necessary. Handwaving and I want a pony
arguments are not sufficient.
Thanks,
tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-08-27 20:56 ` Thomas Gleixner
@ 2014-09-02 18:43 ` Lina Iyer
-1 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-09-02 18:43 UTC (permalink / raw)
To: Thomas Gleixner
Cc: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
rjw, daniel.lezcano
On Wed, Aug 27 2014 at 14:56 -0600, Thomas Gleixner wrote:
>On Wed, 27 Aug 2014, Lina Iyer wrote:
>
>> PM QoS and other idle frameworks can do a better job of addressing power
>> and performance requirements for a cpu, knowing the IRQs that are
>> affine to that cpu. If a performance request is placed against serving
>> the IRQ faster and if the IRQ is affine to a set of cpus, then setting
>> the performance requirements only on those cpus help save power on the
>> rest of the cpus. PM QoS framework is one such framework interested in
>> knowing the smp_affinity of an IRQ and the change notificiation in this
>> regard. QoS requests for the CPU_DMA_LATENCY constraint currently apply
>> to all cpus, but when attached to an IRQ, can be applied only to the set
>> of cpus that IRQ's smp_affinity is set to. This allows other cpus to
>> enter deeper sleep states to save power. More than one framework/driver
>> can be interested in such information.
>
>All you are describing is the fact, that there is only a single
>notifier possible right now, but you completely miss to describe WHY
>you need multiple ones.
>
>The existing notifier was introduced to tell the driver that the irq
>affinity has changed, so it can move its buffers to the proper NUMA
>node. So if that driver gets this information then it can tell the PM
>QoS code that its affinity changed so that stuff can react
>accordingly, right?
>
With the new PM QoS changes, multiple drivers would now be interested in
knowing the smp_affinity changes of the same IRQ. PM QoS abstracts the
notifier in the framework, so individual drivers dont have to register
for notification themselves and handle affinity notifications. But PM
QoS needs to coexist with NUMA and other arch drivers that need to
modify based on their arch specific needs upon affinity change
notifications.
Modifying the IRQ notifications to list, benefits having a simpler
mechanism to notify all arch drivers and frameworks like PM QoS.
>This is going nowhere unless you provide a proper usecase and
>arguments why this is necessary. Handwaving and I want a pony
>arguments are not sufficient.
>
>Thanks,
>
> tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-09-02 18:43 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-09-02 18:43 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 27 2014 at 14:56 -0600, Thomas Gleixner wrote:
>On Wed, 27 Aug 2014, Lina Iyer wrote:
>
>> PM QoS and other idle frameworks can do a better job of addressing power
>> and performance requirements for a cpu, knowing the IRQs that are
>> affine to that cpu. If a performance request is placed against serving
>> the IRQ faster and if the IRQ is affine to a set of cpus, then setting
>> the performance requirements only on those cpus help save power on the
>> rest of the cpus. PM QoS framework is one such framework interested in
>> knowing the smp_affinity of an IRQ and the change notificiation in this
>> regard. QoS requests for the CPU_DMA_LATENCY constraint currently apply
>> to all cpus, but when attached to an IRQ, can be applied only to the set
>> of cpus that IRQ's smp_affinity is set to. This allows other cpus to
>> enter deeper sleep states to save power. More than one framework/driver
>> can be interested in such information.
>
>All you are describing is the fact, that there is only a single
>notifier possible right now, but you completely miss to describe WHY
>you need multiple ones.
>
>The existing notifier was introduced to tell the driver that the irq
>affinity has changed, so it can move its buffers to the proper NUMA
>node. So if that driver gets this information then it can tell the PM
>QoS code that its affinity changed so that stuff can react
>accordingly, right?
>
With the new PM QoS changes, multiple drivers would now be interested in
knowing the smp_affinity changes of the same IRQ. PM QoS abstracts the
notifier in the framework, so individual drivers dont have to register
for notification themselves and handle affinity notifications. But PM
QoS needs to coexist with NUMA and other arch drivers that need to
modify based on their arch specific needs upon affinity change
notifications.
Modifying the IRQ notifications to list, benefits having a simpler
mechanism to notify all arch drivers and frameworks like PM QoS.
>This is going nowhere unless you provide a proper usecase and
>arguments why this is necessary. Handwaving and I want a pony
>arguments are not sufficient.
>
>Thanks,
>
> tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-09-02 18:43 ` Lina Iyer
@ 2014-09-02 20:56 ` Thomas Gleixner
-1 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-09-02 20:56 UTC (permalink / raw)
To: Lina Iyer
Cc: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
rjw, daniel.lezcano
On Tue, 2 Sep 2014, Lina Iyer wrote:
> On Wed, Aug 27 2014 at 14:56 -0600, Thomas Gleixner wrote:
> > On Wed, 27 Aug 2014, Lina Iyer wrote:
> >
> > All you are describing is the fact, that there is only a single
> > notifier possible right now, but you completely miss to describe WHY
> > you need multiple ones.
> >
> > The existing notifier was introduced to tell the driver that the irq
> > affinity has changed, so it can move its buffers to the proper NUMA
> > node. So if that driver gets this information then it can tell the PM
> > QoS code that its affinity changed so that stuff can react
> > accordingly, right?
> >
> With the new PM QoS changes, multiple drivers would now be interested in
> knowing the smp_affinity changes of the same IRQ. PM QoS abstracts the
Again, you fail to tell me WHY multiple drivers are interested and
which drivers are interested and what they do with that information.
> notifier in the framework, so individual drivers dont have to register
> for notification themselves and handle affinity notifications. But PM
> QoS needs to coexist with NUMA and other arch drivers that need to
> modify based on their arch specific needs upon affinity change
> notifications.
Lots of unspecified blurb. What has NUMA to do with other arch
drivers? Are you actually understanding what this is all about?
> Modifying the IRQ notifications to list, benefits having a simpler
> mechanism to notify all arch drivers and frameworks like PM QoS.
Painting my bikeshed blue benefits all neighbours and institutions
like the Royal Navy, right?
Lina, this is leading nowhere. You just make completely unspecified
claims and try to push your solution to a not explained problem
through. That's not how it works, at least not with me.
So please sit down and write up a proper description of the problem
you are trying to solve.
All I can see from your postings so far is:
1) You want to use the notification for PM QoS
2) You run into conflicts with the existing notifiers
3) You want to solve that conflict
What you are not telling is:
1) In which way is PM QoS going to use that notifier
2) How does that conflict with the existing users. And we only
have two of them:
- InfiniPath 7322 chip driver
- cpu_rmap driver, which is used by
- solarflare NIC driver
- mellanox mlx4 driver
- cisco enic driver
So how is that conflicting with your ARM centric PM QoS work?
Please provide proper debug info about such a conflict, if it
exists at all.
3) Which arch drivers are involved?
So far none, at least not in mainline according to
"grep irq_set_affinity_notifier arch/".
If you have out of tree code which uses this, then we want to
see it first to understand what the arch driver is trying to
solve by using that notification mechanism.
The more I think about what you are not telling the more I get the
feeling that this whole PM QoS thing you try to do is a complete
design disaster.
>From your changelog:
"PM QoS and other idle frameworks can do a better job of addressing
power and performance requirements for a cpu, knowing the IRQs that
are affine to that cpu."
I agree with that. PM might make better decisions if it knows which
activities are targeted at a particular cpu.
So someone figured that out and you got tasked to implement it. So you
looked for a way to get to this information and you found the existing
notifier and decided to (ab)use it for your purpose. But you did not
spend a split second to figure out why this is wrong and even useless.
At the point where PM QoS or whatever decides to use that information,
it does not know at all what the current affinity mask is. So in your
patch 4/4, which I ignored so far you do:
+ struct irq_desc *desc = irq_to_desc(req->irq);
+ struct cpumask *mask = desc->irq_data.affinity;
WTF?
You did not even have the courtesy to use the proper functions for
this. And of course you access all of this unlocked. So how is that
supposed to work with a concurrent affinity update? Not at
all.
Fiddling with irq_desc is a NONO: Consult your preferred search
machine or just the kernel changelogs about the consequences.
Looking at the other patches you really seem to have missed out on
some other NONOs:
struct pm_qos_request {
+ enum pm_qos_req_type type;
+ struct cpumask cpus_affine;
+ /* Internal structure members */
struct plist_node node;
int pm_qos_class;
struct delayed_work work; /* for pm_qos_update_request_timeout */
@@ -80,6 +90,7 @@ enum pm_qos_type {
struct pm_qos_constraints {
struct plist_head list;
s32 target_value; /* Do not change to 64 bit */
+ s32 target_per_cpu[NR_CPUS];
s32 default_value;
s32 no_constraint_value;
enum pm_qos_type type;
Go figure how well "struct cpumask" and a s32 array sizeof NR_CPUS
bodes on kernels compiled for NR_CPUS=8192 or larger.
Just get it: You are hacking core code, not some random ARM SoC
driver.
That aside, your design or the lack thereof sucks. While your hackery
might work somehow for the PM QoS problem at hand - ignoring the hard
to debug race window of the concurrent affinity update - it's
completely useless for other PM related decisions despite your claims
that its a benefit for all of that.
How would a general "keep track of the targets of all interrupts in
the system" mechanism make use of this? Not at all. Simply because it
has no idea which interrupts are requested at any given time. Sure
your answer will be "add another notifier" to solve that problem. But
that's the completely wrong answer because notifiers are a PITA and
should go away. Here is Linus' POV of notifiers and I really agree
with it:
"Notifiers are a disgrace, and almost all of them are a major design
mistake. They all have locking problems, the introduce internal
arbitrary API's that are hard to fix later (because you have random
people who decided to hook into them, which is the whole *point* of
those notifier chains)."
And so I really fight that notifier chain tooth and nails until someone
provides me a proper argument WHY it cant be solved with proper
explicit and understandable mechanisms.
Back to your PM QoS trainwreck:
I'm really impressed that you did not notice at all how inaccurate and
therefor useless the affinity information which you are consulting is.
Did it ever hit even the outer hemisphere of your brain, that
irqdata.affinity is the affinity which was requested by the caller of
an affinity setter function and not the effective affinity?
I bet it did not, simply because you did not pay any attention and
bother to analyze that proper. Otherwise you would have tried to hack
around this as well in some disgusting way.
Really great savings, if the affinity is left at default to all cpus,
but the effective affinity is a single cpu due to irq routing
restrictions in the hardware.
Surely you tested against single CPU affinities and achieved great
results, but that's not a generic and useful solution.
Thanks,
tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-09-02 20:56 ` Thomas Gleixner
0 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-09-02 20:56 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 2 Sep 2014, Lina Iyer wrote:
> On Wed, Aug 27 2014 at 14:56 -0600, Thomas Gleixner wrote:
> > On Wed, 27 Aug 2014, Lina Iyer wrote:
> >
> > All you are describing is the fact, that there is only a single
> > notifier possible right now, but you completely miss to describe WHY
> > you need multiple ones.
> >
> > The existing notifier was introduced to tell the driver that the irq
> > affinity has changed, so it can move its buffers to the proper NUMA
> > node. So if that driver gets this information then it can tell the PM
> > QoS code that its affinity changed so that stuff can react
> > accordingly, right?
> >
> With the new PM QoS changes, multiple drivers would now be interested in
> knowing the smp_affinity changes of the same IRQ. PM QoS abstracts the
Again, you fail to tell me WHY multiple drivers are interested and
which drivers are interested and what they do with that information.
> notifier in the framework, so individual drivers dont have to register
> for notification themselves and handle affinity notifications. But PM
> QoS needs to coexist with NUMA and other arch drivers that need to
> modify based on their arch specific needs upon affinity change
> notifications.
Lots of unspecified blurb. What has NUMA to do with other arch
drivers? Are you actually understanding what this is all about?
> Modifying the IRQ notifications to list, benefits having a simpler
> mechanism to notify all arch drivers and frameworks like PM QoS.
Painting my bikeshed blue benefits all neighbours and institutions
like the Royal Navy, right?
Lina, this is leading nowhere. You just make completely unspecified
claims and try to push your solution to a not explained problem
through. That's not how it works, at least not with me.
So please sit down and write up a proper description of the problem
you are trying to solve.
All I can see from your postings so far is:
1) You want to use the notification for PM QoS
2) You run into conflicts with the existing notifiers
3) You want to solve that conflict
What you are not telling is:
1) In which way is PM QoS going to use that notifier
2) How does that conflict with the existing users. And we only
have two of them:
- InfiniPath 7322 chip driver
- cpu_rmap driver, which is used by
- solarflare NIC driver
- mellanox mlx4 driver
- cisco enic driver
So how is that conflicting with your ARM centric PM QoS work?
Please provide proper debug info about such a conflict, if it
exists at all.
3) Which arch drivers are involved?
So far none, at least not in mainline according to
"grep irq_set_affinity_notifier arch/".
If you have out of tree code which uses this, then we want to
see it first to understand what the arch driver is trying to
solve by using that notification mechanism.
The more I think about what you are not telling the more I get the
feeling that this whole PM QoS thing you try to do is a complete
design disaster.
>From your changelog:
"PM QoS and other idle frameworks can do a better job of addressing
power and performance requirements for a cpu, knowing the IRQs that
are affine to that cpu."
I agree with that. PM might make better decisions if it knows which
activities are targeted at a particular cpu.
So someone figured that out and you got tasked to implement it. So you
looked for a way to get to this information and you found the existing
notifier and decided to (ab)use it for your purpose. But you did not
spend a split second to figure out why this is wrong and even useless.
At the point where PM QoS or whatever decides to use that information,
it does not know at all what the current affinity mask is. So in your
patch 4/4, which I ignored so far you do:
+ struct irq_desc *desc = irq_to_desc(req->irq);
+ struct cpumask *mask = desc->irq_data.affinity;
WTF?
You did not even have the courtesy to use the proper functions for
this. And of course you access all of this unlocked. So how is that
supposed to work with a concurrent affinity update? Not at
all.
Fiddling with irq_desc is a NONO: Consult your preferred search
machine or just the kernel changelogs about the consequences.
Looking at the other patches you really seem to have missed out on
some other NONOs:
struct pm_qos_request {
+ enum pm_qos_req_type type;
+ struct cpumask cpus_affine;
+ /* Internal structure members */
struct plist_node node;
int pm_qos_class;
struct delayed_work work; /* for pm_qos_update_request_timeout */
@@ -80,6 +90,7 @@ enum pm_qos_type {
struct pm_qos_constraints {
struct plist_head list;
s32 target_value; /* Do not change to 64 bit */
+ s32 target_per_cpu[NR_CPUS];
s32 default_value;
s32 no_constraint_value;
enum pm_qos_type type;
Go figure how well "struct cpumask" and a s32 array sizeof NR_CPUS
bodes on kernels compiled for NR_CPUS=8192 or larger.
Just get it: You are hacking core code, not some random ARM SoC
driver.
That aside, your design or the lack thereof sucks. While your hackery
might work somehow for the PM QoS problem at hand - ignoring the hard
to debug race window of the concurrent affinity update - it's
completely useless for other PM related decisions despite your claims
that its a benefit for all of that.
How would a general "keep track of the targets of all interrupts in
the system" mechanism make use of this? Not at all. Simply because it
has no idea which interrupts are requested at any given time. Sure
your answer will be "add another notifier" to solve that problem. But
that's the completely wrong answer because notifiers are a PITA and
should go away. Here is Linus' POV of notifiers and I really agree
with it:
"Notifiers are a disgrace, and almost all of them are a major design
mistake. They all have locking problems, the introduce internal
arbitrary API's that are hard to fix later (because you have random
people who decided to hook into them, which is the whole *point* of
those notifier chains)."
And so I really fight that notifier chain tooth and nails until someone
provides me a proper argument WHY it cant be solved with proper
explicit and understandable mechanisms.
Back to your PM QoS trainwreck:
I'm really impressed that you did not notice at all how inaccurate and
therefor useless the affinity information which you are consulting is.
Did it ever hit even the outer hemisphere of your brain, that
irqdata.affinity is the affinity which was requested by the caller of
an affinity setter function and not the effective affinity?
I bet it did not, simply because you did not pay any attention and
bother to analyze that proper. Otherwise you would have tried to hack
around this as well in some disgusting way.
Really great savings, if the affinity is left at default to all cpus,
but the effective affinity is a single cpu due to irq routing
restrictions in the hardware.
Surely you tested against single CPU affinities and achieved great
results, but that's not a generic and useful solution.
Thanks,
tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
[not found] ` <20140924221023.GD1004@ilina-mac.local>
@ 2014-09-25 15:43 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-09-25 15:43 UTC (permalink / raw)
To: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
tglx, rjw
On Tue, Sep 02 2014 at 14:56 -0600, Thomas Gleixner wrote:
>On Tue, 2 Sep 2014, Lina Iyer wrote:
>> On Wed, Aug 27 2014 at 14:56 -0600, Thomas Gleixner wrote:
>> > On Wed, 27 Aug 2014, Lina Iyer wrote:
>> >
>> > All you are describing is the fact, that there is only a single
>> > notifier possible right now, but you completely miss to describe WHY
>> > you need multiple ones.
>> >
>> > The existing notifier was introduced to tell the driver that the irq
>> > affinity has changed, so it can move its buffers to the proper NUMA
>> > node. So if that driver gets this information then it can tell the PM
>> > QoS code that its affinity changed so that stuff can react
>> > accordingly, right?
>> >
>> With the new PM QoS changes, multiple drivers would now be interested in
>> knowing the smp_affinity changes of the same IRQ. PM QoS abstracts the
>
>Again, you fail to tell me WHY multiple drivers are interested and
>which drivers are interested and what they do with that information.
>
>> notifier in the framework, so individual drivers dont have to register
>> for notification themselves and handle affinity notifications. But PM
>> QoS needs to coexist with NUMA and other arch drivers that need to
>> modify based on their arch specific needs upon affinity change
>> notifications.
>
>Lots of unspecified blurb. What has NUMA to do with other arch
>drivers? Are you actually understanding what this is all about?
>
>> Modifying the IRQ notifications to list, benefits having a simpler
>> mechanism to notify all arch drivers and frameworks like PM QoS.
>
>Painting my bikeshed blue benefits all neighbours and institutions
>like the Royal Navy, right?
Sorry about not being very clear in my responses.
>
>Lina, this is leading nowhere. You just make completely unspecified
>claims and try to push your solution to a not explained problem
>through. That's not how it works, at least not with me.
>
>So please sit down and write up a proper description of the problem
>you are trying to solve.
>
>All I can see from your postings so far is:
>
> 1) You want to use the notification for PM QoS
>
> 2) You run into conflicts with the existing notifiers
>
> 3) You want to solve that conflict
>
Here is the premise of my patchset -
When a cpuidle governor selects the idle state of the cpu, it uses
the PM QoS CPU_DMA_LATENCY constraint value in determining the
acceptable tolerance in exit latency of the cpu, before choosing the low
power state for the cpu. Drivers specify the PM QoS request for this
constraint, indicating their tolerance. The aggregated QoS request is
used in determining the state of each cpu.
Now, when drivers make a request, they usually have a device IRQ in
mind, which generally needs to be addressed within the stipulated time
after the interrupt fires. By default, the IRQs' have affinity towards
many cpus. However, when used with a IRQ balancer, that sets the
affinity only to a subset of the available cpus, it seems inefficient
that all cpus obey the QoS requirement in terms of power.
Say, when an USB driver specifies a 2 millisecond QoS constraint, it
prevents all the cpus in the SoC from entering any power down state,
which need more than 2 ms to be effective. Thats a lot of leakage
current that could be saved. The IRQ in many instances would just
wake up cpu0, while other cpus remain in high leakage idle states. This
information is critical to the governor to determine the best idle state
it could allow the cpu to be in.
So the change here is to specify QoS requests against a subset of cpus,
which when aggregated, the idle governor can distill out the QoS
requirement only for that cpu. When requests are made with a set of cpus
as the intended target for the QoS value, the PM QoS framework
calculates the QoS value for each cpu. The governor can then be modified
to request the QoS CPU_DMA_LATENCY constraint on an individual cpu and
can choose the deepest idle state that would respect the requirement.
By default QoS requests do not have a preference for cpus and that is
the behavior with these patches too. Using the per-cpu PM QoS, the
drivers can now specify a set of cpus that need to obey the QoS
requests, if they know the cpus or they would specify an IRQ, if the
request is to avoid latency in handling the device IRQ. The driver is
not expected to handle the IRQ's smp_affinity changes and would be done
by the PM QoS framework on behalf of the driver. This is not enforced on
the driver that they need to specify an IRQ or a subset of cpus for
their request. If there is no IRQ or cpu preference, then PM QoS
defaults to applying the request for all cpus. I would expect many of
the existing drivers still want this behavior.
In the case a driver is aware that the QoS request to guarantee a
certain performance in handling the device IRQ, the request can be made
to track the IRQ. PM QoS registers for a smp_affinity change and does
the best judgement based on the affinity. The IRQ may still fire on any
of the cpus in the affinity mask which would make it comparable to the
existing QoS behavior and no power saving is achieved.
>What you are not telling is:
>
> 1) In which way is PM QoS going to use that notifier
>
> 2) How does that conflict with the existing users. And we only
> have two of them:
>
> - InfiniPath 7322 chip driver
>
> - cpu_rmap driver, which is used by
>
> - solarflare NIC driver
> - mellanox mlx4 driver
> - cisco enic driver
>
> So how is that conflicting with your ARM centric PM QoS work?
>
> Please provide proper debug info about such a conflict, if it
> exists at all.
>
> 3) Which arch drivers are involved?
>
> So far none, at least not in mainline according to
> "grep irq_set_affinity_notifier arch/".
>
> If you have out of tree code which uses this, then we want to
> see it first to understand what the arch driver is trying to
> solve by using that notification mechanism.
>
I assumed that PM QoS is also used in conjunction with NUMA. I did
notice a few systems that used the smp_affinity notifications. My
assumption was such systems may have drivers that could prefer a per-cpu
QoS request. per-cpu PM QoS is not an ARM specific change, though this
benefits quite a bit in a power constrained environment. An octa core
system with a low power or a high performance distinction amongst the
cores, clearly benefits from this feature, when used with an IRQ
balancer.
It didnt seem right me that PM QoS gets notified from InifiniPath and NIC
drivers, while others have PM QoS registering directly.
As far my usecases are concerned in an ARM chipset, I did not see any
conflict with the existing users of the smp affinity notifications. But,
I assume the systems may want to use per-cpu PM QoS for some of their
IRQs on a later date. Hence the change to a notification mechanism.
>The more I think about what you are not telling the more I get the
>feeling that this whole PM QoS thing you try to do is a complete
>design disaster.
>
>From your changelog:
>
> "PM QoS and other idle frameworks can do a better job of addressing
> power and performance requirements for a cpu, knowing the IRQs that
> are affine to that cpu."
>
>I agree with that. PM might make better decisions if it knows which
>activities are targeted at a particular cpu.
>
>So someone figured that out and you got tasked to implement it. So you
>looked for a way to get to this information and you found the existing
>notifier and decided to (ab)use it for your purpose. But you did not
>spend a split second to figure out why this is wrong and even useless.
>
>At the point where PM QoS or whatever decides to use that information,
>it does not know at all what the current affinity mask is. So in your
>patch 4/4, which I ignored so far you do:
>
>+ struct irq_desc *desc = irq_to_desc(req->irq);
>+ struct cpumask *mask = desc->irq_data.affinity;
>
>WTF?
>
Sorry, bad access on my part. I will fix it.
>You did not even have the courtesy to use the proper functions for
>this. And of course you access all of this unlocked. So how is that
>supposed to work with a concurrent affinity update? Not at
>all.
>
>Fiddling with irq_desc is a NONO: Consult your preferred search
>machine or just the kernel changelogs about the consequences.
>
>Looking at the other patches you really seem to have missed out on
>some other NONOs:
>
> struct pm_qos_request {
>+ enum pm_qos_req_type type;
>+ struct cpumask cpus_affine;
>+ /* Internal structure members */
> struct plist_node node;
> int pm_qos_class;
> struct delayed_work work; /* for pm_qos_update_request_timeout */
>@@ -80,6 +90,7 @@ enum pm_qos_type {
> struct pm_qos_constraints {
> struct plist_head list;
> s32 target_value; /* Do not change to 64 bit */
>+ s32 target_per_cpu[NR_CPUS];
> s32 default_value;
> s32 no_constraint_value;
> enum pm_qos_type type;
>
>Go figure how well "struct cpumask" and a s32 array sizeof NR_CPUS
>bodes on kernels compiled for NR_CPUS=8192 or larger.
>
Sorry, I did not imagine the possiblity of these failures.
I figured that I would not use up a per-cpu variable, since the access
to the values may be from one cpu where the code is being processed, but
seems like there are quite a few merits, that I overlooked.
>Just get it: You are hacking core code, not some random ARM SoC
>driver.
>
>That aside, your design or the lack thereof sucks. While your hackery
>might work somehow for the PM QoS problem at hand - ignoring the hard
>to debug race window of the concurrent affinity update - it's
>completely useless for other PM related decisions despite your claims
>that its a benefit for all of that.
Yes, there is a race condition in PM QoS, where the notification between
the time, the request is added and the notification is registered.
AFAIK, the behavior in the irq/manage.c seems to be held the same. If
there is a concurrent change in notification, the schedule_work would
not reschedule and possibly not send a notification, which happens today
as well. I will revisit the change and look, if I can make it better.
>
>How would a general "keep track of the targets of all interrupts in
>the system" mechanism make use of this?
Sorry, I do not understand your question.
PM QoS is only interested in the IRQs specified in the QoS request. If
there are no requests that need to be associated with an IRQ, then PM
QoS will not register for an affinity change notification.
>Not at all. Simply because it
>has no idea which interrupts are requested at any given time. Sure
>your answer will be "add another notifier" to solve that problem. But
>that's the completely wrong answer because notifiers are a PITA and
>should go away. Here is Linus' POV of notifiers and I really agree
>with it:
>
> "Notifiers are a disgrace, and almost all of them are a major design
> mistake. They all have locking problems, the introduce internal
> arbitrary API's that are hard to fix later (because you have random
> people who decided to hook into them, which is the whole *point* of
> those notifier chains)."
>
>And so I really fight that notifier chain tooth and nails until someone
>provides me a proper argument WHY it cant be solved with proper
>explicit and understandable mechanisms.
>
FWIW, I do not like notifications either. I feel it adds unpredictablity
to the system. If I didnt think, that NUMA and other systems might
benefit from per-cpu PM QoS feature, I would not have added this
notification mechanism and instead registered directly. I admit to
having not much of a knowledge about NUMA, but atleast PM QoS seems to
be architecture agnostic and all systems could use this generic kernel
feature.
Also, the notification added is restricted to PM QoS framework. The
smp-affinity notifications are not relayed to the driver making the PM
QoS request.
The way it is with per-cpu PM QoS is that the framework registers for
notification, *only* if there is a QoS request tagged with an IRQ. When
a request does not specify an IRQ, the QoS framework does not register
for any IRQ affinity notifications. Also, its possible that multiple
drivers may have a QoS request and could tag the same IRQ, in which
case, the notfication list helps. But surely, I can use other methods in
PM QoS to do the same thing, without the need for a notification list in
the IRQ framework. But that still doesnt solve for other systems that
may have NUMA or NIC drivers using per-cpu PM QoS.
When notified PM QoS updates its request list and amends the request
affinity and recalculates the target value. In this patch, I currently
update requests when the irq notifications are sent out.
>Back to your PM QoS trainwreck:
>
>I'm really impressed that you did not notice at all how inaccurate and
>therefor useless the affinity information which you are consulting is.
>
>Did it ever hit even the outer hemisphere of your brain, that
>irqdata.affinity is the affinity which was requested by the caller of
>an affinity setter function and not the effective affinity?
>
Yes and the effective affinity may not match up to irqdata.affinity,
but the only reliable information we have is irqdata.affinity. We are
trying to do the best with the information available. It still is better
than having no information about affinity.
>I bet it did not, simply because you did not pay any attention and
>bother to analyze that proper. Otherwise you would have tried to hack
>around this as well in some disgusting way.
>
>Really great savings, if the affinity is left at default to all cpus,
>but the effective affinity is a single cpu due to irq routing
>restrictions in the hardware.
>
>Surely you tested against single CPU affinities and achieved great
>results, but that's not a generic and useful solution.
Here is what needs to happen, before the power savings can be
beneficial.
- Drivers specifying PM QoS request update their drivers to
track the request against an IRQ
- SoCs that have a way of constraining the IRQ affinity to a
subset of cpus. Or may use an IRQ balancer or may be design
limited to have IRQs affine to a set of cpus.
- Menu and other cpuidle governors consider per-cpu PM QoS in
determining the idle state of the cpu.
We may not see any benefit without all these in play on a given system.
Like you say, with single CPU affinity and with these changes, we do see
quite a bit of power savings from the performance cores.
Obviously, I seem to be doing it wrong with this change. I am ready to
amend and start over, if there are better ways to get affinity
information.
Thanks,
Lina
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-09-25 15:43 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-09-25 15:43 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 02 2014 at 14:56 -0600, Thomas Gleixner wrote:
>On Tue, 2 Sep 2014, Lina Iyer wrote:
>> On Wed, Aug 27 2014 at 14:56 -0600, Thomas Gleixner wrote:
>> > On Wed, 27 Aug 2014, Lina Iyer wrote:
>> >
>> > All you are describing is the fact, that there is only a single
>> > notifier possible right now, but you completely miss to describe WHY
>> > you need multiple ones.
>> >
>> > The existing notifier was introduced to tell the driver that the irq
>> > affinity has changed, so it can move its buffers to the proper NUMA
>> > node. So if that driver gets this information then it can tell the PM
>> > QoS code that its affinity changed so that stuff can react
>> > accordingly, right?
>> >
>> With the new PM QoS changes, multiple drivers would now be interested in
>> knowing the smp_affinity changes of the same IRQ. PM QoS abstracts the
>
>Again, you fail to tell me WHY multiple drivers are interested and
>which drivers are interested and what they do with that information.
>
>> notifier in the framework, so individual drivers dont have to register
>> for notification themselves and handle affinity notifications. But PM
>> QoS needs to coexist with NUMA and other arch drivers that need to
>> modify based on their arch specific needs upon affinity change
>> notifications.
>
>Lots of unspecified blurb. What has NUMA to do with other arch
>drivers? Are you actually understanding what this is all about?
>
>> Modifying the IRQ notifications to list, benefits having a simpler
>> mechanism to notify all arch drivers and frameworks like PM QoS.
>
>Painting my bikeshed blue benefits all neighbours and institutions
>like the Royal Navy, right?
Sorry about not being very clear in my responses.
>
>Lina, this is leading nowhere. You just make completely unspecified
>claims and try to push your solution to a not explained problem
>through. That's not how it works, at least not with me.
>
>So please sit down and write up a proper description of the problem
>you are trying to solve.
>
>All I can see from your postings so far is:
>
> 1) You want to use the notification for PM QoS
>
> 2) You run into conflicts with the existing notifiers
>
> 3) You want to solve that conflict
>
Here is the premise of my patchset -
When a cpuidle governor selects the idle state of the cpu, it uses
the PM QoS CPU_DMA_LATENCY constraint value in determining the
acceptable tolerance in exit latency of the cpu, before choosing the low
power state for the cpu. Drivers specify the PM QoS request for this
constraint, indicating their tolerance. The aggregated QoS request is
used in determining the state of each cpu.
Now, when drivers make a request, they usually have a device IRQ in
mind, which generally needs to be addressed within the stipulated time
after the interrupt fires. By default, the IRQs' have affinity towards
many cpus. However, when used with a IRQ balancer, that sets the
affinity only to a subset of the available cpus, it seems inefficient
that all cpus obey the QoS requirement in terms of power.
Say, when an USB driver specifies a 2 millisecond QoS constraint, it
prevents all the cpus in the SoC from entering any power down state,
which need more than 2 ms to be effective. Thats a lot of leakage
current that could be saved. The IRQ in many instances would just
wake up cpu0, while other cpus remain in high leakage idle states. This
information is critical to the governor to determine the best idle state
it could allow the cpu to be in.
So the change here is to specify QoS requests against a subset of cpus,
which when aggregated, the idle governor can distill out the QoS
requirement only for that cpu. When requests are made with a set of cpus
as the intended target for the QoS value, the PM QoS framework
calculates the QoS value for each cpu. The governor can then be modified
to request the QoS CPU_DMA_LATENCY constraint on an individual cpu and
can choose the deepest idle state that would respect the requirement.
By default QoS requests do not have a preference for cpus and that is
the behavior with these patches too. Using the per-cpu PM QoS, the
drivers can now specify a set of cpus that need to obey the QoS
requests, if they know the cpus or they would specify an IRQ, if the
request is to avoid latency in handling the device IRQ. The driver is
not expected to handle the IRQ's smp_affinity changes and would be done
by the PM QoS framework on behalf of the driver. This is not enforced on
the driver that they need to specify an IRQ or a subset of cpus for
their request. If there is no IRQ or cpu preference, then PM QoS
defaults to applying the request for all cpus. I would expect many of
the existing drivers still want this behavior.
In the case a driver is aware that the QoS request to guarantee a
certain performance in handling the device IRQ, the request can be made
to track the IRQ. PM QoS registers for a smp_affinity change and does
the best judgement based on the affinity. The IRQ may still fire on any
of the cpus in the affinity mask which would make it comparable to the
existing QoS behavior and no power saving is achieved.
>What you are not telling is:
>
> 1) In which way is PM QoS going to use that notifier
>
> 2) How does that conflict with the existing users. And we only
> have two of them:
>
> - InfiniPath 7322 chip driver
>
> - cpu_rmap driver, which is used by
>
> - solarflare NIC driver
> - mellanox mlx4 driver
> - cisco enic driver
>
> So how is that conflicting with your ARM centric PM QoS work?
>
> Please provide proper debug info about such a conflict, if it
> exists at all.
>
> 3) Which arch drivers are involved?
>
> So far none, at least not in mainline according to
> "grep irq_set_affinity_notifier arch/".
>
> If you have out of tree code which uses this, then we want to
> see it first to understand what the arch driver is trying to
> solve by using that notification mechanism.
>
I assumed that PM QoS is also used in conjunction with NUMA. I did
notice a few systems that used the smp_affinity notifications. My
assumption was such systems may have drivers that could prefer a per-cpu
QoS request. per-cpu PM QoS is not an ARM specific change, though this
benefits quite a bit in a power constrained environment. An octa core
system with a low power or a high performance distinction amongst the
cores, clearly benefits from this feature, when used with an IRQ
balancer.
It didnt seem right me that PM QoS gets notified from InifiniPath and NIC
drivers, while others have PM QoS registering directly.
As far my usecases are concerned in an ARM chipset, I did not see any
conflict with the existing users of the smp affinity notifications. But,
I assume the systems may want to use per-cpu PM QoS for some of their
IRQs on a later date. Hence the change to a notification mechanism.
>The more I think about what you are not telling the more I get the
>feeling that this whole PM QoS thing you try to do is a complete
>design disaster.
>
>From your changelog:
>
> "PM QoS and other idle frameworks can do a better job of addressing
> power and performance requirements for a cpu, knowing the IRQs that
> are affine to that cpu."
>
>I agree with that. PM might make better decisions if it knows which
>activities are targeted at a particular cpu.
>
>So someone figured that out and you got tasked to implement it. So you
>looked for a way to get to this information and you found the existing
>notifier and decided to (ab)use it for your purpose. But you did not
>spend a split second to figure out why this is wrong and even useless.
>
>At the point where PM QoS or whatever decides to use that information,
>it does not know at all what the current affinity mask is. So in your
>patch 4/4, which I ignored so far you do:
>
>+ struct irq_desc *desc = irq_to_desc(req->irq);
>+ struct cpumask *mask = desc->irq_data.affinity;
>
>WTF?
>
Sorry, bad access on my part. I will fix it.
>You did not even have the courtesy to use the proper functions for
>this. And of course you access all of this unlocked. So how is that
>supposed to work with a concurrent affinity update? Not at
>all.
>
>Fiddling with irq_desc is a NONO: Consult your preferred search
>machine or just the kernel changelogs about the consequences.
>
>Looking at the other patches you really seem to have missed out on
>some other NONOs:
>
> struct pm_qos_request {
>+ enum pm_qos_req_type type;
>+ struct cpumask cpus_affine;
>+ /* Internal structure members */
> struct plist_node node;
> int pm_qos_class;
> struct delayed_work work; /* for pm_qos_update_request_timeout */
>@@ -80,6 +90,7 @@ enum pm_qos_type {
> struct pm_qos_constraints {
> struct plist_head list;
> s32 target_value; /* Do not change to 64 bit */
>+ s32 target_per_cpu[NR_CPUS];
> s32 default_value;
> s32 no_constraint_value;
> enum pm_qos_type type;
>
>Go figure how well "struct cpumask" and a s32 array sizeof NR_CPUS
>bodes on kernels compiled for NR_CPUS=8192 or larger.
>
Sorry, I did not imagine the possiblity of these failures.
I figured that I would not use up a per-cpu variable, since the access
to the values may be from one cpu where the code is being processed, but
seems like there are quite a few merits, that I overlooked.
>Just get it: You are hacking core code, not some random ARM SoC
>driver.
>
>That aside, your design or the lack thereof sucks. While your hackery
>might work somehow for the PM QoS problem at hand - ignoring the hard
>to debug race window of the concurrent affinity update - it's
>completely useless for other PM related decisions despite your claims
>that its a benefit for all of that.
Yes, there is a race condition in PM QoS, where the notification between
the time, the request is added and the notification is registered.
AFAIK, the behavior in the irq/manage.c seems to be held the same. If
there is a concurrent change in notification, the schedule_work would
not reschedule and possibly not send a notification, which happens today
as well. I will revisit the change and look, if I can make it better.
>
>How would a general "keep track of the targets of all interrupts in
>the system" mechanism make use of this?
Sorry, I do not understand your question.
PM QoS is only interested in the IRQs specified in the QoS request. If
there are no requests that need to be associated with an IRQ, then PM
QoS will not register for an affinity change notification.
>Not at all. Simply because it
>has no idea which interrupts are requested at any given time. Sure
>your answer will be "add another notifier" to solve that problem. But
>that's the completely wrong answer because notifiers are a PITA and
>should go away. Here is Linus' POV of notifiers and I really agree
>with it:
>
> "Notifiers are a disgrace, and almost all of them are a major design
> mistake. They all have locking problems, the introduce internal
> arbitrary API's that are hard to fix later (because you have random
> people who decided to hook into them, which is the whole *point* of
> those notifier chains)."
>
>And so I really fight that notifier chain tooth and nails until someone
>provides me a proper argument WHY it cant be solved with proper
>explicit and understandable mechanisms.
>
FWIW, I do not like notifications either. I feel it adds unpredictablity
to the system. If I didnt think, that NUMA and other systems might
benefit from per-cpu PM QoS feature, I would not have added this
notification mechanism and instead registered directly. I admit to
having not much of a knowledge about NUMA, but atleast PM QoS seems to
be architecture agnostic and all systems could use this generic kernel
feature.
Also, the notification added is restricted to PM QoS framework. The
smp-affinity notifications are not relayed to the driver making the PM
QoS request.
The way it is with per-cpu PM QoS is that the framework registers for
notification, *only* if there is a QoS request tagged with an IRQ. When
a request does not specify an IRQ, the QoS framework does not register
for any IRQ affinity notifications. Also, its possible that multiple
drivers may have a QoS request and could tag the same IRQ, in which
case, the notfication list helps. But surely, I can use other methods in
PM QoS to do the same thing, without the need for a notification list in
the IRQ framework. But that still doesnt solve for other systems that
may have NUMA or NIC drivers using per-cpu PM QoS.
When notified PM QoS updates its request list and amends the request
affinity and recalculates the target value. In this patch, I currently
update requests when the irq notifications are sent out.
>Back to your PM QoS trainwreck:
>
>I'm really impressed that you did not notice at all how inaccurate and
>therefor useless the affinity information which you are consulting is.
>
>Did it ever hit even the outer hemisphere of your brain, that
>irqdata.affinity is the affinity which was requested by the caller of
>an affinity setter function and not the effective affinity?
>
Yes and the effective affinity may not match up to irqdata.affinity,
but the only reliable information we have is irqdata.affinity. We are
trying to do the best with the information available. It still is better
than having no information about affinity.
>I bet it did not, simply because you did not pay any attention and
>bother to analyze that proper. Otherwise you would have tried to hack
>around this as well in some disgusting way.
>
>Really great savings, if the affinity is left at default to all cpus,
>but the effective affinity is a single cpu due to irq routing
>restrictions in the hardware.
>
>Surely you tested against single CPU affinities and achieved great
>results, but that's not a generic and useful solution.
Here is what needs to happen, before the power savings can be
beneficial.
- Drivers specifying PM QoS request update their drivers to
track the request against an IRQ
- SoCs that have a way of constraining the IRQ affinity to a
subset of cpus. Or may use an IRQ balancer or may be design
limited to have IRQs affine to a set of cpus.
- Menu and other cpuidle governors consider per-cpu PM QoS in
determining the idle state of the cpu.
We may not see any benefit without all these in play on a given system.
Like you say, with single CPU affinity and with these changes, we do see
quite a bit of power savings from the performance cores.
Obviously, I seem to be doing it wrong with this change. I am ready to
amend and start over, if there are better ways to get affinity
information.
Thanks,
Lina
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-09-02 20:56 ` Thomas Gleixner
@ 2014-09-25 15:50 ` Lina Iyer
-1 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-09-25 15:50 UTC (permalink / raw)
To: Thomas Gleixner
Cc: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
rjw, daniel.lezcano
Resending...
Hoping to retain the thread information, this time.
On Tue, Sep 02 2014 at 14:56 -0600, Thomas Gleixner wrote:
>On Tue, 2 Sep 2014, Lina Iyer wrote:
>> On Wed, Aug 27 2014 at 14:56 -0600, Thomas Gleixner wrote:
>> > On Wed, 27 Aug 2014, Lina Iyer wrote:
>> >
>> > All you are describing is the fact, that there is only a single
>> > notifier possible right now, but you completely miss to describe WHY
>> > you need multiple ones.
>> >
>> > The existing notifier was introduced to tell the driver that the irq
>> > affinity has changed, so it can move its buffers to the proper NUMA
>> > node. So if that driver gets this information then it can tell the PM
>> > QoS code that its affinity changed so that stuff can react
>> > accordingly, right?
>> >
>> With the new PM QoS changes, multiple drivers would now be interested in
>> knowing the smp_affinity changes of the same IRQ. PM QoS abstracts the
>
>Again, you fail to tell me WHY multiple drivers are interested and
>which drivers are interested and what they do with that information.
>
>> notifier in the framework, so individual drivers dont have to register
>> for notification themselves and handle affinity notifications. But PM
>> QoS needs to coexist with NUMA and other arch drivers that need to
>> modify based on their arch specific needs upon affinity change
>> notifications.
>
>Lots of unspecified blurb. What has NUMA to do with other arch
>drivers? Are you actually understanding what this is all about?
>
>> Modifying the IRQ notifications to list, benefits having a simpler
>> mechanism to notify all arch drivers and frameworks like PM QoS.
>
>Painting my bikeshed blue benefits all neighbours and institutions
>like the Royal Navy, right?
Sorry about not being very clear in my responses.
>
>Lina, this is leading nowhere. You just make completely unspecified
>claims and try to push your solution to a not explained problem
>through. That's not how it works, at least not with me.
>
>So please sit down and write up a proper description of the problem
>you are trying to solve.
>
>All I can see from your postings so far is:
>
> 1) You want to use the notification for PM QoS
>
> 2) You run into conflicts with the existing notifiers
>
> 3) You want to solve that conflict
>
Here is the premise of my patchset -
When a cpuidle governor selects the idle state of the cpu, it uses
the PM QoS CPU_DMA_LATENCY constraint value in determining the
acceptable tolerance in exit latency of the cpu, before choosing the low
power state for the cpu. Drivers specify the PM QoS request for this
constraint, indicating their tolerance. The aggregated QoS request is
used in determining the state of each cpu.
Now, when drivers make a request, they usually have a device IRQ in
mind, which generally needs to be addressed within the stipulated time
after the interrupt fires. By default, the IRQs' have affinity towards
many cpus. However, when used with a IRQ balancer, that sets the
affinity only to a subset of the available cpus, it seems inefficient
that all cpus obey the QoS requirement in terms of power.
Say, when an USB driver specifies a 2 millisecond QoS constraint, it
prevents all the cpus in the SoC from entering any power down state,
which need more than 2 ms to be effective. Thats a lot of leakage
current that could be saved. The IRQ in many instances would just
wake up cpu0, while other cpus remain in high leakage idle states. This
information is critical to the governor to determine the best idle state
it could allow the cpu to be in.
So the change here is to specify QoS requests against a subset of cpus,
which when aggregated, the idle governor can distill out the QoS
requirement only for that cpu. When requests are made with a set of cpus
as the intended target for the QoS value, the PM QoS framework
calculates the QoS value for each cpu. The governor can then be modified
to request the QoS CPU_DMA_LATENCY constraint on an individual cpu and
can choose the deepest idle state that would respect the requirement.
By default QoS requests do not have a preference for cpus and that is
the behavior with these patches too. Using the per-cpu PM QoS, the
drivers can now specify a set of cpus that need to obey the QoS
requests, if they know the cpus or they would specify an IRQ, if the
request is to avoid latency in handling the device IRQ. The driver is
not expected to handle the IRQ's smp_affinity changes and would be done
by the PM QoS framework on behalf of the driver. This is not enforced on
the driver that they need to specify an IRQ or a subset of cpus for
their request. If there is no IRQ or cpu preference, then PM QoS
defaults to applying the request for all cpus. I would expect many of
the existing drivers still want this behavior.
In the case a driver is aware that the QoS request to guarantee a
certain performance in handling the device IRQ, the request can be made
to track the IRQ. PM QoS registers for a smp_affinity change and does
the best judgement based on the affinity. The IRQ may still fire on any
of the cpus in the affinity mask which would make it comparable to the
existing QoS behavior and no power saving is achieved.
>What you are not telling is:
>
> 1) In which way is PM QoS going to use that notifier
>
> 2) How does that conflict with the existing users. And we only
> have two of them:
>
> - InfiniPath 7322 chip driver
>
> - cpu_rmap driver, which is used by
>
> - solarflare NIC driver
> - mellanox mlx4 driver
> - cisco enic driver
>
> So how is that conflicting with your ARM centric PM QoS work?
>
> Please provide proper debug info about such a conflict, if it
> exists at all.
>
> 3) Which arch drivers are involved?
>
> So far none, at least not in mainline according to
> "grep irq_set_affinity_notifier arch/".
>
> If you have out of tree code which uses this, then we want to
> see it first to understand what the arch driver is trying to
> solve by using that notification mechanism.
>
I assumed that PM QoS is also used in conjunction with NUMA. I did
notice a few systems that used the smp_affinity notifications. My
assumption was such systems may have drivers that could prefer a per-cpu
QoS request. per-cpu PM QoS is not an ARM specific change, though this
benefits quite a bit in a power constrained environment. An octa core
system with a low power or a high performance distinction amongst the
cores, clearly benefits from this feature, when used with an IRQ
balancer.
It didnt seem right me that PM QoS gets notified from InifiniPath and NIC
drivers, while others have PM QoS registering directly.
As far my usecases are concerned in an ARM chipset, I did not see any
conflict with the existing users of the smp affinity notifications. But,
I assume the systems may want to use per-cpu PM QoS for some of their
IRQs on a later date. Hence the change to a notification mechanism.
>The more I think about what you are not telling the more I get the
>feeling that this whole PM QoS thing you try to do is a complete
>design disaster.
>
>From your changelog:
>
> "PM QoS and other idle frameworks can do a better job of addressing
> power and performance requirements for a cpu, knowing the IRQs that
> are affine to that cpu."
>
>I agree with that. PM might make better decisions if it knows which
>activities are targeted at a particular cpu.
>
>So someone figured that out and you got tasked to implement it. So you
>looked for a way to get to this information and you found the existing
>notifier and decided to (ab)use it for your purpose. But you did not
>spend a split second to figure out why this is wrong and even useless.
>
>At the point where PM QoS or whatever decides to use that information,
>it does not know at all what the current affinity mask is. So in your
>patch 4/4, which I ignored so far you do:
>
>+ struct irq_desc *desc = irq_to_desc(req->irq);
>+ struct cpumask *mask = desc->irq_data.affinity;
>
>WTF?
>
Sorry, bad access on my part. I will fix it.
>You did not even have the courtesy to use the proper functions for
>this. And of course you access all of this unlocked. So how is that
>supposed to work with a concurrent affinity update? Not at
>all.
>
>Fiddling with irq_desc is a NONO: Consult your preferred search
>machine or just the kernel changelogs about the consequences.
>
>Looking at the other patches you really seem to have missed out on
>some other NONOs:
>
> struct pm_qos_request {
>+ enum pm_qos_req_type type;
>+ struct cpumask cpus_affine;
>+ /* Internal structure members */
> struct plist_node node;
> int pm_qos_class;
> struct delayed_work work; /* for pm_qos_update_request_timeout */
>@@ -80,6 +90,7 @@ enum pm_qos_type {
> struct pm_qos_constraints {
> struct plist_head list;
> s32 target_value; /* Do not change to 64 bit */
>+ s32 target_per_cpu[NR_CPUS];
> s32 default_value;
> s32 no_constraint_value;
> enum pm_qos_type type;
>
>Go figure how well "struct cpumask" and a s32 array sizeof NR_CPUS
>bodes on kernels compiled for NR_CPUS=8192 or larger.
>
Sorry, I did not imagine the possiblity of these failures.
I figured that I would not use up a per-cpu variable, since the access
to the values may be from one cpu where the code is being processed, but
seems like there are quite a few merits, that I overlooked.
>Just get it: You are hacking core code, not some random ARM SoC
>driver.
>
>That aside, your design or the lack thereof sucks. While your hackery
>might work somehow for the PM QoS problem at hand - ignoring the hard
>to debug race window of the concurrent affinity update - it's
>completely useless for other PM related decisions despite your claims
>that its a benefit for all of that.
Yes, there is a race condition in PM QoS, where the notification between
the time, the request is added and the notification is registered.
AFAIK, the behavior in the irq/manage.c seems to be held the same. If
there is a concurrent change in notification, the schedule_work would
not reschedule and possibly not send a notification, which happens today
as well. I will revisit the change and look, if I can make it better.
>
>How would a general "keep track of the targets of all interrupts in
>the system" mechanism make use of this?
Sorry, I do not understand your question.
PM QoS is only interested in the IRQs specified in the QoS request. If
there are no requests that need to be associated with an IRQ, then PM
QoS will not register for an affinity change notification.
>Not at all. Simply because it
>has no idea which interrupts are requested at any given time. Sure
>your answer will be "add another notifier" to solve that problem. But
>that's the completely wrong answer because notifiers are a PITA and
>should go away. Here is Linus' POV of notifiers and I really agree
>with it:
>
> "Notifiers are a disgrace, and almost all of them are a major design
> mistake. They all have locking problems, the introduce internal
> arbitrary API's that are hard to fix later (because you have random
> people who decided to hook into them, which is the whole *point* of
> those notifier chains)."
>
>And so I really fight that notifier chain tooth and nails until someone
>provides me a proper argument WHY it cant be solved with proper
>explicit and understandable mechanisms.
>
FWIW, I do not like notifications either. I feel it adds unpredictablity
to the system. If I didnt think, that NUMA and other systems might
benefit from per-cpu PM QoS feature, I would not have added this
notification mechanism and instead registered directly. I admit to
having not much of a knowledge about NUMA, but atleast PM QoS seems to
be architecture agnostic and all systems could use this generic kernel
feature.
Also, the notification added is restricted to PM QoS framework. The
smp-affinity notifications are not relayed to the driver making the PM
QoS request.
The way it is with per-cpu PM QoS is that the framework registers for
notification, *only* if there is a QoS request tagged with an IRQ. When
a request does not specify an IRQ, the QoS framework does not register
for any IRQ affinity notifications. Also, its possible that multiple
drivers may have a QoS request and could tag the same IRQ, in which
case, the notfication list helps. But surely, I can use other methods in
PM QoS to do the same thing, without the need for a notification list in
the IRQ framework. But that still doesnt solve for other systems that
may have NUMA or NIC drivers using per-cpu PM QoS.
When notified PM QoS updates its request list and amends the request
affinity and recalculates the target value. In this patch, I currently
update requests when the irq notifications are sent out.
>Back to your PM QoS trainwreck:
>
>I'm really impressed that you did not notice at all how inaccurate and
>therefor useless the affinity information which you are consulting is.
>
>Did it ever hit even the outer hemisphere of your brain, that
>irqdata.affinity is the affinity which was requested by the caller of
>an affinity setter function and not the effective affinity?
>
Yes and the effective affinity may not match up to irqdata.affinity,
but the only reliable information we have is irqdata.affinity. We are
trying to do the best with the information available. It still is better
than having no information about affinity.
>I bet it did not, simply because you did not pay any attention and
>bother to analyze that proper. Otherwise you would have tried to hack
>around this as well in some disgusting way.
>
>Really great savings, if the affinity is left at default to all cpus,
>but the effective affinity is a single cpu due to irq routing
>restrictions in the hardware.
>
>Surely you tested against single CPU affinities and achieved great
>results, but that's not a generic and useful solution.
Here is what needs to happen, before the power savings can be
beneficial.
- Drivers specifying PM QoS request update their drivers to
track the request against an IRQ
- SoCs that have a way of constraining the IRQ affinity to a
subset of cpus. Or may use an IRQ balancer or may be design
limited to have IRQs affine to a set of cpus.
- Menu and other cpuidle governors consider per-cpu PM QoS in
determining the idle state of the cpu.
We may not see any benefit without all these in play on a given system.
Like you say, with single CPU affinity and with these changes, we do see
quite a bit of power savings from the performance cores.
Obviously, I seem to be doing it wrong with this change. I am ready to
amend and start over, if there are better ways to get affinity
information.
Thanks,
Lina
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-09-25 15:50 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-09-25 15:50 UTC (permalink / raw)
To: linux-arm-kernel
Resending...
Hoping to retain the thread information, this time.
On Tue, Sep 02 2014 at 14:56 -0600, Thomas Gleixner wrote:
>On Tue, 2 Sep 2014, Lina Iyer wrote:
>> On Wed, Aug 27 2014 at 14:56 -0600, Thomas Gleixner wrote:
>> > On Wed, 27 Aug 2014, Lina Iyer wrote:
>> >
>> > All you are describing is the fact, that there is only a single
>> > notifier possible right now, but you completely miss to describe WHY
>> > you need multiple ones.
>> >
>> > The existing notifier was introduced to tell the driver that the irq
>> > affinity has changed, so it can move its buffers to the proper NUMA
>> > node. So if that driver gets this information then it can tell the PM
>> > QoS code that its affinity changed so that stuff can react
>> > accordingly, right?
>> >
>> With the new PM QoS changes, multiple drivers would now be interested in
>> knowing the smp_affinity changes of the same IRQ. PM QoS abstracts the
>
>Again, you fail to tell me WHY multiple drivers are interested and
>which drivers are interested and what they do with that information.
>
>> notifier in the framework, so individual drivers dont have to register
>> for notification themselves and handle affinity notifications. But PM
>> QoS needs to coexist with NUMA and other arch drivers that need to
>> modify based on their arch specific needs upon affinity change
>> notifications.
>
>Lots of unspecified blurb. What has NUMA to do with other arch
>drivers? Are you actually understanding what this is all about?
>
>> Modifying the IRQ notifications to list, benefits having a simpler
>> mechanism to notify all arch drivers and frameworks like PM QoS.
>
>Painting my bikeshed blue benefits all neighbours and institutions
>like the Royal Navy, right?
Sorry about not being very clear in my responses.
>
>Lina, this is leading nowhere. You just make completely unspecified
>claims and try to push your solution to a not explained problem
>through. That's not how it works, at least not with me.
>
>So please sit down and write up a proper description of the problem
>you are trying to solve.
>
>All I can see from your postings so far is:
>
> 1) You want to use the notification for PM QoS
>
> 2) You run into conflicts with the existing notifiers
>
> 3) You want to solve that conflict
>
Here is the premise of my patchset -
When a cpuidle governor selects the idle state of the cpu, it uses
the PM QoS CPU_DMA_LATENCY constraint value in determining the
acceptable tolerance in exit latency of the cpu, before choosing the low
power state for the cpu. Drivers specify the PM QoS request for this
constraint, indicating their tolerance. The aggregated QoS request is
used in determining the state of each cpu.
Now, when drivers make a request, they usually have a device IRQ in
mind, which generally needs to be addressed within the stipulated time
after the interrupt fires. By default, the IRQs' have affinity towards
many cpus. However, when used with a IRQ balancer, that sets the
affinity only to a subset of the available cpus, it seems inefficient
that all cpus obey the QoS requirement in terms of power.
Say, when an USB driver specifies a 2 millisecond QoS constraint, it
prevents all the cpus in the SoC from entering any power down state,
which need more than 2 ms to be effective. Thats a lot of leakage
current that could be saved. The IRQ in many instances would just
wake up cpu0, while other cpus remain in high leakage idle states. This
information is critical to the governor to determine the best idle state
it could allow the cpu to be in.
So the change here is to specify QoS requests against a subset of cpus,
which when aggregated, the idle governor can distill out the QoS
requirement only for that cpu. When requests are made with a set of cpus
as the intended target for the QoS value, the PM QoS framework
calculates the QoS value for each cpu. The governor can then be modified
to request the QoS CPU_DMA_LATENCY constraint on an individual cpu and
can choose the deepest idle state that would respect the requirement.
By default QoS requests do not have a preference for cpus and that is
the behavior with these patches too. Using the per-cpu PM QoS, the
drivers can now specify a set of cpus that need to obey the QoS
requests, if they know the cpus or they would specify an IRQ, if the
request is to avoid latency in handling the device IRQ. The driver is
not expected to handle the IRQ's smp_affinity changes and would be done
by the PM QoS framework on behalf of the driver. This is not enforced on
the driver that they need to specify an IRQ or a subset of cpus for
their request. If there is no IRQ or cpu preference, then PM QoS
defaults to applying the request for all cpus. I would expect many of
the existing drivers still want this behavior.
In the case a driver is aware that the QoS request to guarantee a
certain performance in handling the device IRQ, the request can be made
to track the IRQ. PM QoS registers for a smp_affinity change and does
the best judgement based on the affinity. The IRQ may still fire on any
of the cpus in the affinity mask which would make it comparable to the
existing QoS behavior and no power saving is achieved.
>What you are not telling is:
>
> 1) In which way is PM QoS going to use that notifier
>
> 2) How does that conflict with the existing users. And we only
> have two of them:
>
> - InfiniPath 7322 chip driver
>
> - cpu_rmap driver, which is used by
>
> - solarflare NIC driver
> - mellanox mlx4 driver
> - cisco enic driver
>
> So how is that conflicting with your ARM centric PM QoS work?
>
> Please provide proper debug info about such a conflict, if it
> exists at all.
>
> 3) Which arch drivers are involved?
>
> So far none, at least not in mainline according to
> "grep irq_set_affinity_notifier arch/".
>
> If you have out of tree code which uses this, then we want to
> see it first to understand what the arch driver is trying to
> solve by using that notification mechanism.
>
I assumed that PM QoS is also used in conjunction with NUMA. I did
notice a few systems that used the smp_affinity notifications. My
assumption was such systems may have drivers that could prefer a per-cpu
QoS request. per-cpu PM QoS is not an ARM specific change, though this
benefits quite a bit in a power constrained environment. An octa core
system with a low power or a high performance distinction amongst the
cores, clearly benefits from this feature, when used with an IRQ
balancer.
It didnt seem right me that PM QoS gets notified from InifiniPath and NIC
drivers, while others have PM QoS registering directly.
As far my usecases are concerned in an ARM chipset, I did not see any
conflict with the existing users of the smp affinity notifications. But,
I assume the systems may want to use per-cpu PM QoS for some of their
IRQs on a later date. Hence the change to a notification mechanism.
>The more I think about what you are not telling the more I get the
>feeling that this whole PM QoS thing you try to do is a complete
>design disaster.
>
>From your changelog:
>
> "PM QoS and other idle frameworks can do a better job of addressing
> power and performance requirements for a cpu, knowing the IRQs that
> are affine to that cpu."
>
>I agree with that. PM might make better decisions if it knows which
>activities are targeted at a particular cpu.
>
>So someone figured that out and you got tasked to implement it. So you
>looked for a way to get to this information and you found the existing
>notifier and decided to (ab)use it for your purpose. But you did not
>spend a split second to figure out why this is wrong and even useless.
>
>At the point where PM QoS or whatever decides to use that information,
>it does not know at all what the current affinity mask is. So in your
>patch 4/4, which I ignored so far you do:
>
>+ struct irq_desc *desc = irq_to_desc(req->irq);
>+ struct cpumask *mask = desc->irq_data.affinity;
>
>WTF?
>
Sorry, bad access on my part. I will fix it.
>You did not even have the courtesy to use the proper functions for
>this. And of course you access all of this unlocked. So how is that
>supposed to work with a concurrent affinity update? Not at
>all.
>
>Fiddling with irq_desc is a NONO: Consult your preferred search
>machine or just the kernel changelogs about the consequences.
>
>Looking at the other patches you really seem to have missed out on
>some other NONOs:
>
> struct pm_qos_request {
>+ enum pm_qos_req_type type;
>+ struct cpumask cpus_affine;
>+ /* Internal structure members */
> struct plist_node node;
> int pm_qos_class;
> struct delayed_work work; /* for pm_qos_update_request_timeout */
>@@ -80,6 +90,7 @@ enum pm_qos_type {
> struct pm_qos_constraints {
> struct plist_head list;
> s32 target_value; /* Do not change to 64 bit */
>+ s32 target_per_cpu[NR_CPUS];
> s32 default_value;
> s32 no_constraint_value;
> enum pm_qos_type type;
>
>Go figure how well "struct cpumask" and a s32 array sizeof NR_CPUS
>bodes on kernels compiled for NR_CPUS=8192 or larger.
>
Sorry, I did not imagine the possiblity of these failures.
I figured that I would not use up a per-cpu variable, since the access
to the values may be from one cpu where the code is being processed, but
seems like there are quite a few merits, that I overlooked.
>Just get it: You are hacking core code, not some random ARM SoC
>driver.
>
>That aside, your design or the lack thereof sucks. While your hackery
>might work somehow for the PM QoS problem at hand - ignoring the hard
>to debug race window of the concurrent affinity update - it's
>completely useless for other PM related decisions despite your claims
>that its a benefit for all of that.
Yes, there is a race condition in PM QoS, where the notification between
the time, the request is added and the notification is registered.
AFAIK, the behavior in the irq/manage.c seems to be held the same. If
there is a concurrent change in notification, the schedule_work would
not reschedule and possibly not send a notification, which happens today
as well. I will revisit the change and look, if I can make it better.
>
>How would a general "keep track of the targets of all interrupts in
>the system" mechanism make use of this?
Sorry, I do not understand your question.
PM QoS is only interested in the IRQs specified in the QoS request. If
there are no requests that need to be associated with an IRQ, then PM
QoS will not register for an affinity change notification.
>Not at all. Simply because it
>has no idea which interrupts are requested at any given time. Sure
>your answer will be "add another notifier" to solve that problem. But
>that's the completely wrong answer because notifiers are a PITA and
>should go away. Here is Linus' POV of notifiers and I really agree
>with it:
>
> "Notifiers are a disgrace, and almost all of them are a major design
> mistake. They all have locking problems, the introduce internal
> arbitrary API's that are hard to fix later (because you have random
> people who decided to hook into them, which is the whole *point* of
> those notifier chains)."
>
>And so I really fight that notifier chain tooth and nails until someone
>provides me a proper argument WHY it cant be solved with proper
>explicit and understandable mechanisms.
>
FWIW, I do not like notifications either. I feel it adds unpredictablity
to the system. If I didnt think, that NUMA and other systems might
benefit from per-cpu PM QoS feature, I would not have added this
notification mechanism and instead registered directly. I admit to
having not much of a knowledge about NUMA, but atleast PM QoS seems to
be architecture agnostic and all systems could use this generic kernel
feature.
Also, the notification added is restricted to PM QoS framework. The
smp-affinity notifications are not relayed to the driver making the PM
QoS request.
The way it is with per-cpu PM QoS is that the framework registers for
notification, *only* if there is a QoS request tagged with an IRQ. When
a request does not specify an IRQ, the QoS framework does not register
for any IRQ affinity notifications. Also, its possible that multiple
drivers may have a QoS request and could tag the same IRQ, in which
case, the notfication list helps. But surely, I can use other methods in
PM QoS to do the same thing, without the need for a notification list in
the IRQ framework. But that still doesnt solve for other systems that
may have NUMA or NIC drivers using per-cpu PM QoS.
When notified PM QoS updates its request list and amends the request
affinity and recalculates the target value. In this patch, I currently
update requests when the irq notifications are sent out.
>Back to your PM QoS trainwreck:
>
>I'm really impressed that you did not notice at all how inaccurate and
>therefor useless the affinity information which you are consulting is.
>
>Did it ever hit even the outer hemisphere of your brain, that
>irqdata.affinity is the affinity which was requested by the caller of
>an affinity setter function and not the effective affinity?
>
Yes and the effective affinity may not match up to irqdata.affinity,
but the only reliable information we have is irqdata.affinity. We are
trying to do the best with the information available. It still is better
than having no information about affinity.
>I bet it did not, simply because you did not pay any attention and
>bother to analyze that proper. Otherwise you would have tried to hack
>around this as well in some disgusting way.
>
>Really great savings, if the affinity is left at default to all cpus,
>but the effective affinity is a single cpu due to irq routing
>restrictions in the hardware.
>
>Surely you tested against single CPU affinities and achieved great
>results, but that's not a generic and useful solution.
Here is what needs to happen, before the power savings can be
beneficial.
- Drivers specifying PM QoS request update their drivers to
track the request against an IRQ
- SoCs that have a way of constraining the IRQ affinity to a
subset of cpus. Or may use an IRQ balancer or may be design
limited to have IRQs affine to a set of cpus.
- Menu and other cpuidle governors consider per-cpu PM QoS in
determining the idle state of the cpu.
We may not see any benefit without all these in play on a given system.
Like you say, with single CPU affinity and with these changes, we do see
quite a bit of power savings from the performance cores.
Obviously, I seem to be doing it wrong with this change. I am ready to
amend and start over, if there are better ways to get affinity
information.
Thanks,
Lina
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-09-02 20:56 ` Thomas Gleixner
@ 2014-09-25 20:35 ` Kevin Hilman
-1 siblings, 0 replies; 38+ messages in thread
From: Kevin Hilman @ 2014-09-25 20:35 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Lina Iyer, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
rjw, daniel.lezcano
Hi Thomas,
Thomas Gleixner <tglx@linutronix.de> writes:
[...]
> All I can see from your postings so far is:
>
> 1) You want to use the notification for PM QoS
>
> 2) You run into conflicts with the existing notifiers
>
> 3) You want to solve that conflict
>
> What you are not telling is:
>
> 1) In which way is PM QoS going to use that notifier
>
> 2) How does that conflict with the existing users. And we only
> have two of them:
>
> - InfiniPath 7322 chip driver
>
> - cpu_rmap driver, which is used by
> - solarflare NIC driver
> - mellanox mlx4 driver
> - cisco enic driver
>
> So how is that conflicting with your ARM centric PM QoS work?
>
> Please provide proper debug info about such a conflict, if it
> exists at all.
Ignoring any new users of the affinity notifiers for now, aren't the
existing users you list above conflicting with each other already?
Maybe I'm missing something, or maybe we're just lucky and nobody uses
them together, but irq_set_affinity_notifier() only allows a single
notifier to be registered at any given time. So if you had a system
with the inifinipath driver and a cpu_rmap user, only the one who's
lucky enough to be the last to call irq_set_affinity_notifier() will
ever be notified.
So I think the main question for Lina is: assuming we need more than one
co-existing driver/subsystem to get IRQ affinity notifiers (and we
already seem to have them, but are lucky they don't currently co-exist),
how should the IRQ core be changed to support that? Is the list
approach proposed in $SUBJECT path reasonable?
Kevin
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-09-25 20:35 ` Kevin Hilman
0 siblings, 0 replies; 38+ messages in thread
From: Kevin Hilman @ 2014-09-25 20:35 UTC (permalink / raw)
To: linux-arm-kernel
Hi Thomas,
Thomas Gleixner <tglx@linutronix.de> writes:
[...]
> All I can see from your postings so far is:
>
> 1) You want to use the notification for PM QoS
>
> 2) You run into conflicts with the existing notifiers
>
> 3) You want to solve that conflict
>
> What you are not telling is:
>
> 1) In which way is PM QoS going to use that notifier
>
> 2) How does that conflict with the existing users. And we only
> have two of them:
>
> - InfiniPath 7322 chip driver
>
> - cpu_rmap driver, which is used by
> - solarflare NIC driver
> - mellanox mlx4 driver
> - cisco enic driver
>
> So how is that conflicting with your ARM centric PM QoS work?
>
> Please provide proper debug info about such a conflict, if it
> exists at all.
Ignoring any new users of the affinity notifiers for now, aren't the
existing users you list above conflicting with each other already?
Maybe I'm missing something, or maybe we're just lucky and nobody uses
them together, but irq_set_affinity_notifier() only allows a single
notifier to be registered at any given time. So if you had a system
with the inifinipath driver and a cpu_rmap user, only the one who's
lucky enough to be the last to call irq_set_affinity_notifier() will
ever be notified.
So I think the main question for Lina is: assuming we need more than one
co-existing driver/subsystem to get IRQ affinity notifiers (and we
already seem to have them, but are lucky they don't currently co-exist),
how should the IRQ core be changed to support that? Is the list
approach proposed in $SUBJECT path reasonable?
Kevin
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-09-25 20:35 ` Kevin Hilman
@ 2014-09-26 9:29 ` Thomas Gleixner
-1 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-09-26 9:29 UTC (permalink / raw)
To: Kevin Hilman
Cc: Lina Iyer, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
rjw, daniel.lezcano
On Thu, 25 Sep 2014, Kevin Hilman wrote:
> Maybe I'm missing something, or maybe we're just lucky and nobody uses
> them together, but irq_set_affinity_notifier() only allows a single
> notifier to be registered at any given time. So if you had a system
A single notifier per irq .....
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-09-26 9:29 ` Thomas Gleixner
0 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-09-26 9:29 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 25 Sep 2014, Kevin Hilman wrote:
> Maybe I'm missing something, or maybe we're just lucky and nobody uses
> them together, but irq_set_affinity_notifier() only allows a single
> notifier to be registered at any given time. So if you had a system
A single notifier per irq .....
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-09-26 9:29 ` Thomas Gleixner
@ 2014-09-26 9:40 ` Russell King - ARM Linux
-1 siblings, 0 replies; 38+ messages in thread
From: Russell King - ARM Linux @ 2014-09-26 9:40 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Kevin Hilman, ulf.hansson, linux-pm, daniel.lezcano, rjw,
linux-kernel, Lina Iyer, linux-arm-kernel
On Fri, Sep 26, 2014 at 11:29:56AM +0200, Thomas Gleixner wrote:
> On Thu, 25 Sep 2014, Kevin Hilman wrote:
> > Maybe I'm missing something, or maybe we're just lucky and nobody uses
> > them together, but irq_set_affinity_notifier() only allows a single
> > notifier to be registered at any given time. So if you had a system
>
> A single notifier per irq .....
So what about two drivers wanting to use this notifier, but sharing an
interrupt?
It sounds to me like this notifier was misdesigned from the very start,
and it should always have supported multiple notifiers.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-09-26 9:40 ` Russell King - ARM Linux
0 siblings, 0 replies; 38+ messages in thread
From: Russell King - ARM Linux @ 2014-09-26 9:40 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Sep 26, 2014 at 11:29:56AM +0200, Thomas Gleixner wrote:
> On Thu, 25 Sep 2014, Kevin Hilman wrote:
> > Maybe I'm missing something, or maybe we're just lucky and nobody uses
> > them together, but irq_set_affinity_notifier() only allows a single
> > notifier to be registered at any given time. So if you had a system
>
> A single notifier per irq .....
So what about two drivers wanting to use this notifier, but sharing an
interrupt?
It sounds to me like this notifier was misdesigned from the very start,
and it should always have supported multiple notifiers.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-09-26 9:40 ` Russell King - ARM Linux
@ 2014-09-26 15:10 ` Kevin Hilman
-1 siblings, 0 replies; 38+ messages in thread
From: Kevin Hilman @ 2014-09-26 15:10 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Thomas Gleixner, ulf.hansson, linux-pm, daniel.lezcano, rjw,
linux-kernel, Lina Iyer, linux-arm-kernel
Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> On Fri, Sep 26, 2014 at 11:29:56AM +0200, Thomas Gleixner wrote:
>> On Thu, 25 Sep 2014, Kevin Hilman wrote:
>> > Maybe I'm missing something, or maybe we're just lucky and nobody uses
>> > them together, but irq_set_affinity_notifier() only allows a single
>> > notifier to be registered at any given time. So if you had a system
>>
>> A single notifier per irq .....
>
> So what about two drivers wanting to use this notifier, but sharing an
> interrupt?
>
> It sounds to me like this notifier was misdesigned from the very start,
> and it should always have supported multiple notifiers.
I agree.
$SUBJECT patch tries to add that support, and is part of a series
wanting to use these notifiers in the PM QoS subsystem, while at the
same time not breaking existing users.
I suppose this series could be written without $SUBJECT patch, and
crossing fingers in the hopes that an existing user of the notifiers
doesn't also need to use the pm_qos constraints, but that seems like
knowingly leaving an armed landmine laying around.
Kevin
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-09-26 15:10 ` Kevin Hilman
0 siblings, 0 replies; 38+ messages in thread
From: Kevin Hilman @ 2014-09-26 15:10 UTC (permalink / raw)
To: linux-arm-kernel
Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> On Fri, Sep 26, 2014 at 11:29:56AM +0200, Thomas Gleixner wrote:
>> On Thu, 25 Sep 2014, Kevin Hilman wrote:
>> > Maybe I'm missing something, or maybe we're just lucky and nobody uses
>> > them together, but irq_set_affinity_notifier() only allows a single
>> > notifier to be registered at any given time. So if you had a system
>>
>> A single notifier per irq .....
>
> So what about two drivers wanting to use this notifier, but sharing an
> interrupt?
>
> It sounds to me like this notifier was misdesigned from the very start,
> and it should always have supported multiple notifiers.
I agree.
$SUBJECT patch tries to add that support, and is part of a series
wanting to use these notifiers in the PM QoS subsystem, while at the
same time not breaking existing users.
I suppose this series could be written without $SUBJECT patch, and
crossing fingers in the hopes that an existing user of the notifiers
doesn't also need to use the pm_qos constraints, but that seems like
knowingly leaving an armed landmine laying around.
Kevin
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-09-26 9:40 ` Russell King - ARM Linux
@ 2014-10-08 14:20 ` Thomas Gleixner
-1 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-10-08 14:20 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Kevin Hilman, ulf.hansson, linux-pm, daniel.lezcano, rjw,
linux-kernel, Lina Iyer, linux-arm-kernel
On Fri, 26 Sep 2014, Russell King - ARM Linux wrote:
> On Fri, Sep 26, 2014 at 11:29:56AM +0200, Thomas Gleixner wrote:
> > On Thu, 25 Sep 2014, Kevin Hilman wrote:
> > > Maybe I'm missing something, or maybe we're just lucky and nobody uses
> > > them together, but irq_set_affinity_notifier() only allows a single
> > > notifier to be registered at any given time. So if you had a system
> >
> > A single notifier per irq .....
>
> So what about two drivers wanting to use this notifier, but sharing an
> interrupt?
>
> It sounds to me like this notifier was misdesigned from the very start,
> and it should always have supported multiple notifiers.
Well, it was designed for a particular usecase which does not require
multiple notifiers. And we wanted it as simple as possible.
So while we could make it take multiple notifiers, it's just wrong for
the use case in question. QoS wants a system wide overview and not a
gazillion notifiers with life time issues etc.
So the proper solution is a general notifier, which gets called if any
interrupt affinity changes. That allows the QoS code or any other
interested party to gain a coherent per CPU knowledge.
That also avoid the whole lifetime issues of notifiers on interrupts,
when they are teared down and the descriptor gets freed. Of course
such a teardown or a simple disable_irq should inform the QoS code as
well so it knows that the CPU is not longer targeted by this
interrupt. Nothing we want to whack into a per irq notifier, really.
Thoughts?
tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-10-08 14:20 ` Thomas Gleixner
0 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-10-08 14:20 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 26 Sep 2014, Russell King - ARM Linux wrote:
> On Fri, Sep 26, 2014 at 11:29:56AM +0200, Thomas Gleixner wrote:
> > On Thu, 25 Sep 2014, Kevin Hilman wrote:
> > > Maybe I'm missing something, or maybe we're just lucky and nobody uses
> > > them together, but irq_set_affinity_notifier() only allows a single
> > > notifier to be registered at any given time. So if you had a system
> >
> > A single notifier per irq .....
>
> So what about two drivers wanting to use this notifier, but sharing an
> interrupt?
>
> It sounds to me like this notifier was misdesigned from the very start,
> and it should always have supported multiple notifiers.
Well, it was designed for a particular usecase which does not require
multiple notifiers. And we wanted it as simple as possible.
So while we could make it take multiple notifiers, it's just wrong for
the use case in question. QoS wants a system wide overview and not a
gazillion notifiers with life time issues etc.
So the proper solution is a general notifier, which gets called if any
interrupt affinity changes. That allows the QoS code or any other
interested party to gain a coherent per CPU knowledge.
That also avoid the whole lifetime issues of notifiers on interrupts,
when they are teared down and the descriptor gets freed. Of course
such a teardown or a simple disable_irq should inform the QoS code as
well so it knows that the CPU is not longer targeted by this
interrupt. Nothing we want to whack into a per irq notifier, really.
Thoughts?
tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-09-25 15:50 ` Lina Iyer
@ 2014-10-08 15:03 ` Thomas Gleixner
-1 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-10-08 15:03 UTC (permalink / raw)
To: Lina Iyer
Cc: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
rjw, daniel.lezcano
On Thu, 25 Sep 2014, Lina Iyer wrote:
> > How would a general "keep track of the targets of all interrupts in
> > the system" mechanism make use of this?
> Sorry, I do not understand your question.
> PM QoS is only interested in the IRQs specified in the QoS request. If
> there are no requests that need to be associated with an IRQ, then PM
> QoS will not register for an affinity change notification.
Right, and I really hate the whole per irq notifier. It's a rats nest
of life time issues and other problems.
It also does not tell you whether an irq is disabled, reenabled or
removed, which will change the qos constraints as well unless you
plaster all drivers with updates to qos for those cases.
So what about adding a qos field to irq_data itself, have a function
to update it and let the irq core keep track of the per cpu irq
relevant qos constraints and provide an evaluation function or a
notifier for the PM/idle code?
That's going to need some serious thought as well, but it should avoid
most of the nasty notifier and lifetime issue which the per irq
notifiers provide.
Thoughts?
tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-10-08 15:03 ` Thomas Gleixner
0 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-10-08 15:03 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 25 Sep 2014, Lina Iyer wrote:
> > How would a general "keep track of the targets of all interrupts in
> > the system" mechanism make use of this?
> Sorry, I do not understand your question.
> PM QoS is only interested in the IRQs specified in the QoS request. If
> there are no requests that need to be associated with an IRQ, then PM
> QoS will not register for an affinity change notification.
Right, and I really hate the whole per irq notifier. It's a rats nest
of life time issues and other problems.
It also does not tell you whether an irq is disabled, reenabled or
removed, which will change the qos constraints as well unless you
plaster all drivers with updates to qos for those cases.
So what about adding a qos field to irq_data itself, have a function
to update it and let the irq core keep track of the per cpu irq
relevant qos constraints and provide an evaluation function or a
notifier for the PM/idle code?
That's going to need some serious thought as well, but it should avoid
most of the nasty notifier and lifetime issue which the per irq
notifiers provide.
Thoughts?
tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-10-08 15:03 ` Thomas Gleixner
@ 2014-10-10 15:11 ` Lina Iyer
-1 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-10-10 15:11 UTC (permalink / raw)
To: Thomas Gleixner
Cc: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
rjw, daniel.lezcano
On Wed, Oct 08 2014 at 09:03 -0600, Thomas Gleixner wrote:
>On Thu, 25 Sep 2014, Lina Iyer wrote:
>> > How would a general "keep track of the targets of all interrupts in
>> > the system" mechanism make use of this?
>> Sorry, I do not understand your question.
>> PM QoS is only interested in the IRQs specified in the QoS request. If
>> there are no requests that need to be associated with an IRQ, then PM
>> QoS will not register for an affinity change notification.
>
>Right, and I really hate the whole per irq notifier. It's a rats nest
>of life time issues and other problems.
>
>It also does not tell you whether an irq is disabled, reenabled or
>removed, which will change the qos constraints as well unless you
>plaster all drivers with updates to qos for those cases.
>
>So what about adding a qos field to irq_data itself, have a function
>to update it and let the irq core keep track of the per cpu irq
>relevant qos constraints and provide an evaluation function or a
>notifier for the PM/idle code?
If that isnt intrusive in the IRQ core, then we can make it work for PM
QoS. The issue that I am concerned is that, it might result in back and
forth between IRQ and PM QoS frameworks. If that doesnt happen, then we
are good with this approach.
>
>That's going to need some serious thought as well, but it should avoid
>most of the nasty notifier and lifetime issue which the per irq
>notifiers provide.
Sure. I will look into this.
>
>Thoughts?
Thank you.
Lina
>
>
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-10-10 15:11 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-10-10 15:11 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 08 2014 at 09:03 -0600, Thomas Gleixner wrote:
>On Thu, 25 Sep 2014, Lina Iyer wrote:
>> > How would a general "keep track of the targets of all interrupts in
>> > the system" mechanism make use of this?
>> Sorry, I do not understand your question.
>> PM QoS is only interested in the IRQs specified in the QoS request. If
>> there are no requests that need to be associated with an IRQ, then PM
>> QoS will not register for an affinity change notification.
>
>Right, and I really hate the whole per irq notifier. It's a rats nest
>of life time issues and other problems.
>
>It also does not tell you whether an irq is disabled, reenabled or
>removed, which will change the qos constraints as well unless you
>plaster all drivers with updates to qos for those cases.
>
>So what about adding a qos field to irq_data itself, have a function
>to update it and let the irq core keep track of the per cpu irq
>relevant qos constraints and provide an evaluation function or a
>notifier for the PM/idle code?
If that isnt intrusive in the IRQ core, then we can make it work for PM
QoS. The issue that I am concerned is that, it might result in back and
forth between IRQ and PM QoS frameworks. If that doesnt happen, then we
are good with this approach.
>
>That's going to need some serious thought as well, but it should avoid
>most of the nasty notifier and lifetime issue which the per irq
>notifiers provide.
Sure. I will look into this.
>
>Thoughts?
Thank you.
Lina
>
>
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-10-10 15:11 ` Lina Iyer
@ 2014-10-17 7:29 ` Thomas Gleixner
-1 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-10-17 7:29 UTC (permalink / raw)
To: Lina Iyer
Cc: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
rjw, daniel.lezcano
On Fri, 10 Oct 2014, Lina Iyer wrote:
> On Wed, Oct 08 2014 at 09:03 -0600, Thomas Gleixner wrote:
> > On Thu, 25 Sep 2014, Lina Iyer wrote:
> > > > How would a general "keep track of the targets of all interrupts in
> > > > the system" mechanism make use of this?
> > > Sorry, I do not understand your question.
> > > PM QoS is only interested in the IRQs specified in the QoS request. If
> > > there are no requests that need to be associated with an IRQ, then PM
> > > QoS will not register for an affinity change notification.
> >
> > Right, and I really hate the whole per irq notifier. It's a rats nest
> > of life time issues and other problems.
> >
> > It also does not tell you whether an irq is disabled, reenabled or
> > removed, which will change the qos constraints as well unless you
> > plaster all drivers with updates to qos for those cases.
> >
> > So what about adding a qos field to irq_data itself, have a function
> > to update it and let the irq core keep track of the per cpu irq
> > relevant qos constraints and provide an evaluation function or a
> > notifier for the PM/idle code?
>
> If that isnt intrusive in the IRQ core, then we can make it work for PM
> QoS. The issue that I am concerned is that, it might result in back and
> forth between IRQ and PM QoS frameworks. If that doesnt happen, then we
> are good with this approach.
I can't tell that upfront, but I think it's worth to explore it.
Thanks,
tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-10-17 7:29 ` Thomas Gleixner
0 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2014-10-17 7:29 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 10 Oct 2014, Lina Iyer wrote:
> On Wed, Oct 08 2014 at 09:03 -0600, Thomas Gleixner wrote:
> > On Thu, 25 Sep 2014, Lina Iyer wrote:
> > > > How would a general "keep track of the targets of all interrupts in
> > > > the system" mechanism make use of this?
> > > Sorry, I do not understand your question.
> > > PM QoS is only interested in the IRQs specified in the QoS request. If
> > > there are no requests that need to be associated with an IRQ, then PM
> > > QoS will not register for an affinity change notification.
> >
> > Right, and I really hate the whole per irq notifier. It's a rats nest
> > of life time issues and other problems.
> >
> > It also does not tell you whether an irq is disabled, reenabled or
> > removed, which will change the qos constraints as well unless you
> > plaster all drivers with updates to qos for those cases.
> >
> > So what about adding a qos field to irq_data itself, have a function
> > to update it and let the irq core keep track of the per cpu irq
> > relevant qos constraints and provide an evaluation function or a
> > notifier for the PM/idle code?
>
> If that isnt intrusive in the IRQ core, then we can make it work for PM
> QoS. The issue that I am concerned is that, it might result in back and
> forth between IRQ and PM QoS frameworks. If that doesnt happen, then we
> are good with this approach.
I can't tell that upfront, but I think it's worth to explore it.
Thanks,
tglx
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
2014-10-17 7:29 ` Thomas Gleixner
@ 2014-11-18 6:22 ` Lina Iyer
-1 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-11-18 6:22 UTC (permalink / raw)
To: Thomas Gleixner
Cc: khilman, ulf.hansson, linux-arm-kernel, linux-pm, linux-kernel,
rjw, daniel.lezcano
Hi Thomas,
On Fri, Oct 17 2014 at 01:29 -0600, Thomas Gleixner wrote:
>On Fri, 10 Oct 2014, Lina Iyer wrote:
>> On Wed, Oct 08 2014 at 09:03 -0600, Thomas Gleixner wrote:
>> > On Thu, 25 Sep 2014, Lina Iyer wrote:
>> > > > How would a general "keep track of the targets of all interrupts in
>> > > > the system" mechanism make use of this?
>> > > Sorry, I do not understand your question.
>> > > PM QoS is only interested in the IRQs specified in the QoS request. If
>> > > there are no requests that need to be associated with an IRQ, then PM
>> > > QoS will not register for an affinity change notification.
>> >
>> > Right, and I really hate the whole per irq notifier. It's a rats nest
>> > of life time issues and other problems.
>> >
>> > It also does not tell you whether an irq is disabled, reenabled or
>> > removed, which will change the qos constraints as well unless you
>> > plaster all drivers with updates to qos for those cases.
>> >
>> > So what about adding a qos field to irq_data itself, have a function
>> > to update it and let the irq core keep track of the per cpu irq
>> > relevant qos constraints and provide an evaluation function or a
>> > notifier for the PM/idle code?
>>
>> If that isnt intrusive in the IRQ core, then we can make it work for PM
>> QoS. The issue that I am concerned is that, it might result in back and
>> forth between IRQ and PM QoS frameworks. If that doesnt happen, then we
>> are good with this approach.
>
>I can't tell that upfront, but I think it's worth to explore it.
>
I was able to review the options and I attempted a few methods, but
off-loading the QoS onto the IRQ framework, made it quite complex to
manage it. QoS values for each of the four constraints and the
constraints could be one of 3 types - min, max or sum, makes it a whole
lot of mess handling it in IRQ code.
I was able to get QoS to be notified of IRQ affinity changes without
using notifiers, but, I still am yet to find a way to modify QoS
requests on enable/disable of IRQ.
I will send the RFC of the new patchset, would be interested in taking a
look and let me know what you think.
Thanks,
Lina
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification
@ 2014-11-18 6:22 ` Lina Iyer
0 siblings, 0 replies; 38+ messages in thread
From: Lina Iyer @ 2014-11-18 6:22 UTC (permalink / raw)
To: linux-arm-kernel
Hi Thomas,
On Fri, Oct 17 2014 at 01:29 -0600, Thomas Gleixner wrote:
>On Fri, 10 Oct 2014, Lina Iyer wrote:
>> On Wed, Oct 08 2014 at 09:03 -0600, Thomas Gleixner wrote:
>> > On Thu, 25 Sep 2014, Lina Iyer wrote:
>> > > > How would a general "keep track of the targets of all interrupts in
>> > > > the system" mechanism make use of this?
>> > > Sorry, I do not understand your question.
>> > > PM QoS is only interested in the IRQs specified in the QoS request. If
>> > > there are no requests that need to be associated with an IRQ, then PM
>> > > QoS will not register for an affinity change notification.
>> >
>> > Right, and I really hate the whole per irq notifier. It's a rats nest
>> > of life time issues and other problems.
>> >
>> > It also does not tell you whether an irq is disabled, reenabled or
>> > removed, which will change the qos constraints as well unless you
>> > plaster all drivers with updates to qos for those cases.
>> >
>> > So what about adding a qos field to irq_data itself, have a function
>> > to update it and let the irq core keep track of the per cpu irq
>> > relevant qos constraints and provide an evaluation function or a
>> > notifier for the PM/idle code?
>>
>> If that isnt intrusive in the IRQ core, then we can make it work for PM
>> QoS. The issue that I am concerned is that, it might result in back and
>> forth between IRQ and PM QoS frameworks. If that doesnt happen, then we
>> are good with this approach.
>
>I can't tell that upfront, but I think it's worth to explore it.
>
I was able to review the options and I attempted a few methods, but
off-loading the QoS onto the IRQ framework, made it quite complex to
manage it. QoS values for each of the four constraints and the
constraints could be one of 3 types - min, max or sum, makes it a whole
lot of mess handling it in IRQ code.
I was able to get QoS to be notified of IRQ affinity changes without
using notifiers, but, I still am yet to find a way to modify QoS
requests on enable/disable of IRQ.
I will send the RFC of the new patchset, would be interested in taking a
look and let me know what you think.
Thanks,
Lina
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2014-11-18 6:22 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-27 20:14 [PATCH v3 0/4] PM QoS: per-cpu PM QoS support Lina Iyer
2014-08-27 20:14 ` Lina Iyer
2014-08-27 20:14 ` [PATCH v3 1/4] QoS: Modify data structures and function arguments for scalability Lina Iyer
2014-08-27 20:14 ` Lina Iyer
2014-08-27 20:14 ` [PATCH v3 2/4] QoS: Enhance framework to support per-cpu PM QoS request Lina Iyer
2014-08-27 20:14 ` Lina Iyer
2014-08-27 20:14 ` [PATCH v3 3/4] irq: Allow multiple clients to register for irq affinity notification Lina Iyer
2014-08-27 20:14 ` Lina Iyer
2014-08-27 20:56 ` Thomas Gleixner
2014-08-27 20:56 ` Thomas Gleixner
2014-09-02 18:43 ` Lina Iyer
2014-09-02 18:43 ` Lina Iyer
2014-09-02 20:56 ` Thomas Gleixner
2014-09-02 20:56 ` Thomas Gleixner
[not found] ` <20140924221023.GD1004@ilina-mac.local>
2014-09-25 15:43 ` Lina Iyer
2014-09-25 15:43 ` Lina Iyer
2014-09-25 15:50 ` Lina Iyer
2014-09-25 15:50 ` Lina Iyer
2014-10-08 15:03 ` Thomas Gleixner
2014-10-08 15:03 ` Thomas Gleixner
2014-10-10 15:11 ` Lina Iyer
2014-10-10 15:11 ` Lina Iyer
2014-10-17 7:29 ` Thomas Gleixner
2014-10-17 7:29 ` Thomas Gleixner
2014-11-18 6:22 ` Lina Iyer
2014-11-18 6:22 ` Lina Iyer
2014-09-25 20:35 ` Kevin Hilman
2014-09-25 20:35 ` Kevin Hilman
2014-09-26 9:29 ` Thomas Gleixner
2014-09-26 9:29 ` Thomas Gleixner
2014-09-26 9:40 ` Russell King - ARM Linux
2014-09-26 9:40 ` Russell King - ARM Linux
2014-09-26 15:10 ` Kevin Hilman
2014-09-26 15:10 ` Kevin Hilman
2014-10-08 14:20 ` Thomas Gleixner
2014-10-08 14:20 ` Thomas Gleixner
2014-08-27 20:14 ` [PATCH v3 4/4] QoS: Enable PM QoS requests to apply only on smp_affinity of an IRQ Lina Iyer
2014-08-27 20:14 ` Lina Iyer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.