drivers/interconnect/core.c | 80 ++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 40 deletions(-)
Replace mutex with rt_mutex to prevent priority inversion between
clients, which can cause unacceptable delays in some cases.
Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
---
We've run into a number of cases internally and from customers where
high priority, RT clients (typically display) become blocked for long
periods of time on lower priority, non-RT clients. Switching to rt_mutex
has proven to help performance in these cases.
drivers/interconnect/core.c | 80 ++++++++++++++++++-------------------
1 file changed, 40 insertions(+), 40 deletions(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 25debded65a8..a536c013d9ca 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -14,7 +14,7 @@
#include <linux/interconnect-provider.h>
#include <linux/list.h>
#include <linux/module.h>
-#include <linux/mutex.h>
+#include <linux/rtmutex.h>
#include <linux/slab.h>
#include <linux/of.h>
#include <linux/overflow.h>
@@ -28,7 +28,7 @@ static DEFINE_IDR(icc_idr);
static LIST_HEAD(icc_providers);
static int providers_count;
static bool synced_state;
-static DEFINE_MUTEX(icc_lock);
+static DEFINE_RT_MUTEX(icc_lock);
static struct dentry *icc_debugfs_dir;
static void icc_summary_show_one(struct seq_file *s, struct icc_node *n)
@@ -47,7 +47,7 @@ static int icc_summary_show(struct seq_file *s, void *data)
seq_puts(s, " node tag avg peak\n");
seq_puts(s, "--------------------------------------------------------------------\n");
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
list_for_each_entry(provider, &icc_providers, provider_list) {
struct icc_node *n;
@@ -73,7 +73,7 @@ static int icc_summary_show(struct seq_file *s, void *data)
}
}
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
return 0;
}
@@ -104,7 +104,7 @@ static int icc_graph_show(struct seq_file *s, void *data)
int i;
seq_puts(s, "digraph {\n\trankdir = LR\n\tnode [shape = record]\n");
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
/* draw providers as cluster subgraphs */
cluster_index = 0;
@@ -136,7 +136,7 @@ static int icc_graph_show(struct seq_file *s, void *data)
icc_graph_show_link(s, 1, n,
n->links[i]);
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
seq_puts(s, "}");
return 0;
@@ -362,7 +362,7 @@ struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec)
if (!spec)
return ERR_PTR(-EINVAL);
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
list_for_each_entry(provider, &icc_providers, provider_list) {
if (provider->dev->of_node == spec->np) {
if (provider->xlate_extended) {
@@ -378,7 +378,7 @@ struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec)
}
}
}
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
if (IS_ERR(node))
return ERR_CAST(node);
@@ -490,9 +490,9 @@ struct icc_path *of_icc_get_by_index(struct device *dev, int idx)
return ERR_CAST(dst_data);
}
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
path = path_find(dev, src_data->node, dst_data->node);
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
if (IS_ERR(path)) {
dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
goto free_icc_data;
@@ -577,12 +577,12 @@ void icc_set_tag(struct icc_path *path, u32 tag)
if (!path)
return;
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
for (i = 0; i < path->num_nodes; i++)
path->reqs[i].tag = tag;
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
}
EXPORT_SYMBOL_GPL(icc_set_tag);
@@ -632,7 +632,7 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
if (WARN_ON(IS_ERR(path) || !path->num_nodes))
return -EINVAL;
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
old_avg = path->reqs[0].avg_bw;
old_peak = path->reqs[0].peak_bw;
@@ -664,7 +664,7 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
apply_constraints(path);
}
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
trace_icc_set_bw_end(path, ret);
@@ -682,12 +682,12 @@ static int __icc_enable(struct icc_path *path, bool enable)
if (WARN_ON(IS_ERR(path) || !path->num_nodes))
return -EINVAL;
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
for (i = 0; i < path->num_nodes; i++)
path->reqs[i].enabled = enable;
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
return icc_set_bw(path, path->reqs[0].avg_bw,
path->reqs[0].peak_bw);
@@ -726,7 +726,7 @@ struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
struct icc_node *src, *dst;
struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
src = node_find(src_id);
if (!src)
@@ -748,7 +748,7 @@ struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
path = ERR_PTR(-ENOMEM);
}
out:
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
return path;
}
EXPORT_SYMBOL_GPL(icc_get);
@@ -773,14 +773,14 @@ void icc_put(struct icc_path *path)
if (ret)
pr_err("%s: error (%d)\n", __func__, ret);
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
for (i = 0; i < path->num_nodes; i++) {
node = path->reqs[i].node;
hlist_del(&path->reqs[i].req_node);
if (!WARN_ON(!node->provider->users))
node->provider->users--;
}
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
kfree_const(path->name);
kfree(path);
@@ -822,11 +822,11 @@ struct icc_node *icc_node_create(int id)
{
struct icc_node *node;
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
node = icc_node_create_nolock(id);
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
return node;
}
@@ -840,7 +840,7 @@ void icc_node_destroy(int id)
{
struct icc_node *node;
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
node = node_find(id);
if (node) {
@@ -848,7 +848,7 @@ void icc_node_destroy(int id)
WARN_ON(!hlist_empty(&node->req_list));
}
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
kfree(node);
}
@@ -876,7 +876,7 @@ int icc_link_create(struct icc_node *node, const int dst_id)
if (!node->provider)
return -EINVAL;
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
dst = node_find(dst_id);
if (!dst) {
@@ -900,7 +900,7 @@ int icc_link_create(struct icc_node *node, const int dst_id)
node->links[node->num_links++] = dst;
out:
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
return ret;
}
@@ -925,7 +925,7 @@ int icc_link_destroy(struct icc_node *src, struct icc_node *dst)
if (IS_ERR_OR_NULL(dst))
return -EINVAL;
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
for (slot = 0; slot < src->num_links; slot++)
if (src->links[slot] == dst)
@@ -946,7 +946,7 @@ int icc_link_destroy(struct icc_node *src, struct icc_node *dst)
ret = -ENOMEM;
out:
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
return ret;
}
@@ -962,7 +962,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
if (WARN_ON(node->provider))
return;
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
node->provider = provider;
list_add_tail(&node->node_list, &provider->nodes);
@@ -988,7 +988,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
node->avg_bw = 0;
node->peak_bw = 0;
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
}
EXPORT_SYMBOL_GPL(icc_node_add);
@@ -998,11 +998,11 @@ EXPORT_SYMBOL_GPL(icc_node_add);
*/
void icc_node_del(struct icc_node *node)
{
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
list_del(&node->node_list);
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
}
EXPORT_SYMBOL_GPL(icc_node_del);
@@ -1041,12 +1041,12 @@ int icc_provider_add(struct icc_provider *provider)
if (WARN_ON(!provider->xlate && !provider->xlate_extended))
return -EINVAL;
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
INIT_LIST_HEAD(&provider->nodes);
list_add_tail(&provider->provider_list, &icc_providers);
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
dev_dbg(provider->dev, "interconnect provider added to topology\n");
@@ -1060,22 +1060,22 @@ EXPORT_SYMBOL_GPL(icc_provider_add);
*/
void icc_provider_del(struct icc_provider *provider)
{
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
if (provider->users) {
pr_warn("interconnect provider still has %d users\n",
provider->users);
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
return;
}
if (!list_empty(&provider->nodes)) {
pr_warn("interconnect provider still has nodes\n");
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
return;
}
list_del(&provider->provider_list);
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
}
EXPORT_SYMBOL_GPL(icc_provider_del);
@@ -1110,7 +1110,7 @@ void icc_sync_state(struct device *dev)
if (count < providers_count)
return;
- mutex_lock(&icc_lock);
+ rt_mutex_lock(&icc_lock);
synced_state = true;
list_for_each_entry(p, &icc_providers, provider_list) {
dev_dbg(p->dev, "interconnect provider is in synced state\n");
@@ -1123,7 +1123,7 @@ void icc_sync_state(struct device *dev)
}
}
}
- mutex_unlock(&icc_lock);
+ rt_mutex_unlock(&icc_lock);
}
EXPORT_SYMBOL_GPL(icc_sync_state);
--
2.17.1Hi Mike,
Thanks for the patch!
On 6.09.22 22:14, Mike Tipton wrote:
> Replace mutex with rt_mutex to prevent priority inversion between
> clients, which can cause unacceptable delays in some cases.
It would be nice if you have any numbers to share in the commit text.
> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
> ---
>
> We've run into a number of cases internally and from customers where
> high priority, RT clients (typically display) become blocked for long
> periods of time on lower priority, non-RT clients. Switching to rt_mutex
> has proven to help performance in these cases.
I am wondering if avoiding the inversion on this specific lock is the right
solution, as there could be other locks that may cause similar issues. Do we
see similar issue with clocks for example or is it just with interconnects?
> drivers/interconnect/core.c | 80 ++++++++++++++++++-------------------
> 1 file changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 25debded65a8..a536c013d9ca 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -14,7 +14,7 @@
> #include <linux/interconnect-provider.h>
> #include <linux/list.h>
> #include <linux/module.h>
> -#include <linux/mutex.h>
> +#include <linux/rtmutex.h>
> #include <linux/slab.h>
> #include <linux/of.h>
> #include <linux/overflow.h>
> @@ -28,7 +28,7 @@ static DEFINE_IDR(icc_idr);
> static LIST_HEAD(icc_providers);
> static int providers_count;
> static bool synced_state;
> -static DEFINE_MUTEX(icc_lock);
> +static DEFINE_RT_MUTEX(icc_lock);
> static struct dentry *icc_debugfs_dir;
>
> static void icc_summary_show_one(struct seq_file *s, struct icc_node *n)
> @@ -47,7 +47,7 @@ static int icc_summary_show(struct seq_file *s, void *data)
> seq_puts(s, " node tag avg peak\n");
> seq_puts(s, "--------------------------------------------------------------------\n");
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> list_for_each_entry(provider, &icc_providers, provider_list) {
> struct icc_node *n;
> @@ -73,7 +73,7 @@ static int icc_summary_show(struct seq_file *s, void *data)
> }
> }
>
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
>
> return 0;
> }
> @@ -104,7 +104,7 @@ static int icc_graph_show(struct seq_file *s, void *data)
> int i;
>
> seq_puts(s, "digraph {\n\trankdir = LR\n\tnode [shape = record]\n");
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> /* draw providers as cluster subgraphs */
> cluster_index = 0;
> @@ -136,7 +136,7 @@ static int icc_graph_show(struct seq_file *s, void *data)
> icc_graph_show_link(s, 1, n,
> n->links[i]);
>
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
> seq_puts(s, "}");
>
> return 0;
> @@ -362,7 +362,7 @@ struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec)
> if (!spec)
> return ERR_PTR(-EINVAL);
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
> list_for_each_entry(provider, &icc_providers, provider_list) {
> if (provider->dev->of_node == spec->np) {
> if (provider->xlate_extended) {
> @@ -378,7 +378,7 @@ struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec)
> }
> }
> }
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
>
> if (IS_ERR(node))
> return ERR_CAST(node);
> @@ -490,9 +490,9 @@ struct icc_path *of_icc_get_by_index(struct device *dev, int idx)
> return ERR_CAST(dst_data);
> }
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
> path = path_find(dev, src_data->node, dst_data->node);
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
> if (IS_ERR(path)) {
> dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> goto free_icc_data;
> @@ -577,12 +577,12 @@ void icc_set_tag(struct icc_path *path, u32 tag)
> if (!path)
> return;
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> for (i = 0; i < path->num_nodes; i++)
> path->reqs[i].tag = tag;
>
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
> }
> EXPORT_SYMBOL_GPL(icc_set_tag);
>
> @@ -632,7 +632,7 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> if (WARN_ON(IS_ERR(path) || !path->num_nodes))
> return -EINVAL;
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> old_avg = path->reqs[0].avg_bw;
> old_peak = path->reqs[0].peak_bw;
> @@ -664,7 +664,7 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> apply_constraints(path);
> }
>
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
>
> trace_icc_set_bw_end(path, ret);
>
> @@ -682,12 +682,12 @@ static int __icc_enable(struct icc_path *path, bool enable)
> if (WARN_ON(IS_ERR(path) || !path->num_nodes))
> return -EINVAL;
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> for (i = 0; i < path->num_nodes; i++)
> path->reqs[i].enabled = enable;
>
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
>
> return icc_set_bw(path, path->reqs[0].avg_bw,
> path->reqs[0].peak_bw);
> @@ -726,7 +726,7 @@ struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
> struct icc_node *src, *dst;
> struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> src = node_find(src_id);
> if (!src)
> @@ -748,7 +748,7 @@ struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
> path = ERR_PTR(-ENOMEM);
> }
> out:
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
> return path;
> }
> EXPORT_SYMBOL_GPL(icc_get);
> @@ -773,14 +773,14 @@ void icc_put(struct icc_path *path)
> if (ret)
> pr_err("%s: error (%d)\n", __func__, ret);
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
> for (i = 0; i < path->num_nodes; i++) {
> node = path->reqs[i].node;
> hlist_del(&path->reqs[i].req_node);
> if (!WARN_ON(!node->provider->users))
> node->provider->users--;
> }
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
>
> kfree_const(path->name);
> kfree(path);
> @@ -822,11 +822,11 @@ struct icc_node *icc_node_create(int id)
> {
> struct icc_node *node;
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> node = icc_node_create_nolock(id);
>
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
>
> return node;
> }
> @@ -840,7 +840,7 @@ void icc_node_destroy(int id)
> {
> struct icc_node *node;
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> node = node_find(id);
> if (node) {
> @@ -848,7 +848,7 @@ void icc_node_destroy(int id)
> WARN_ON(!hlist_empty(&node->req_list));
> }
>
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
>
> kfree(node);
> }
> @@ -876,7 +876,7 @@ int icc_link_create(struct icc_node *node, const int dst_id)
> if (!node->provider)
> return -EINVAL;
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> dst = node_find(dst_id);
> if (!dst) {
> @@ -900,7 +900,7 @@ int icc_link_create(struct icc_node *node, const int dst_id)
> node->links[node->num_links++] = dst;
>
> out:
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
>
> return ret;
> }
> @@ -925,7 +925,7 @@ int icc_link_destroy(struct icc_node *src, struct icc_node *dst)
> if (IS_ERR_OR_NULL(dst))
> return -EINVAL;
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> for (slot = 0; slot < src->num_links; slot++)
> if (src->links[slot] == dst)
> @@ -946,7 +946,7 @@ int icc_link_destroy(struct icc_node *src, struct icc_node *dst)
> ret = -ENOMEM;
>
> out:
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
>
> return ret;
> }
> @@ -962,7 +962,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
> if (WARN_ON(node->provider))
> return;
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> node->provider = provider;
> list_add_tail(&node->node_list, &provider->nodes);
> @@ -988,7 +988,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
> node->avg_bw = 0;
> node->peak_bw = 0;
>
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
> }
> EXPORT_SYMBOL_GPL(icc_node_add);
>
> @@ -998,11 +998,11 @@ EXPORT_SYMBOL_GPL(icc_node_add);
> */
> void icc_node_del(struct icc_node *node)
> {
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> list_del(&node->node_list);
>
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
> }
> EXPORT_SYMBOL_GPL(icc_node_del);
>
> @@ -1041,12 +1041,12 @@ int icc_provider_add(struct icc_provider *provider)
> if (WARN_ON(!provider->xlate && !provider->xlate_extended))
> return -EINVAL;
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
>
> INIT_LIST_HEAD(&provider->nodes);
> list_add_tail(&provider->provider_list, &icc_providers);
>
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
>
> dev_dbg(provider->dev, "interconnect provider added to topology\n");
>
> @@ -1060,22 +1060,22 @@ EXPORT_SYMBOL_GPL(icc_provider_add);
> */
> void icc_provider_del(struct icc_provider *provider)
> {
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
> if (provider->users) {
> pr_warn("interconnect provider still has %d users\n",
> provider->users);
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
> return;
> }
>
> if (!list_empty(&provider->nodes)) {
> pr_warn("interconnect provider still has nodes\n");
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
> return;
> }
>
> list_del(&provider->provider_list);
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
> }
> EXPORT_SYMBOL_GPL(icc_provider_del);
>
> @@ -1110,7 +1110,7 @@ void icc_sync_state(struct device *dev)
> if (count < providers_count)
> return;
>
> - mutex_lock(&icc_lock);
> + rt_mutex_lock(&icc_lock);
> synced_state = true;
> list_for_each_entry(p, &icc_providers, provider_list) {
> dev_dbg(p->dev, "interconnect provider is in synced state\n");
> @@ -1123,7 +1123,7 @@ void icc_sync_state(struct device *dev)
> }
> }
> }
> - mutex_unlock(&icc_lock);
> + rt_mutex_unlock(&icc_lock);
> }
> EXPORT_SYMBOL_GPL(icc_sync_state);
We should also update Kconfig, otherwise i am expecting that the build will fail if
CONFIG_RT_MUTEXES is not enabled.
Thanks,
Georgi
From: Georgi Djakov > Sent: 07 September 2022 08:35 > > Hi Mike, > > Thanks for the patch! > > On 6.09.22 22:14, Mike Tipton wrote: > > Replace mutex with rt_mutex to prevent priority inversion between > > clients, which can cause unacceptable delays in some cases. > > It would be nice if you have any numbers to share in the commit text. > > > Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com> > > --- > > > > We've run into a number of cases internally and from customers where > > high priority, RT clients (typically display) become blocked for long > > periods of time on lower priority, non-RT clients. Switching to rt_mutex > > has proven to help performance in these cases. > > I am wondering if avoiding the inversion on this specific lock is the right > solution, as there could be other locks that may cause similar issues. Do we > see similar issue with clocks for example or is it just with interconnects? My thoughts exactly. Although I've not tried to use the RT kernel I have tried to get a highly threaded RT (audio) userspace application to run reliable. Userspace only has sleep locks so you get all the priority inversion problems. This big problems arise with a highly contended lock that is never held for very long - a prime candidate for spin lock. If a process is even interrupted while holding the lock the entire application stops. The only way I found to avoid this is to replace all the locking with atomic operations. I can't see why the RT kernel doesn't have exactly the same issues. Given how long a process switch takes I really suspect that most spinlocks should remain spinlocks. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Sep 07, 2022 at 10:35:26AM +0300, Georgi Djakov wrote:
> Hi Mike,
>
> Thanks for the patch!
>
> On 6.09.22 22:14, Mike Tipton wrote:
> > Replace mutex with rt_mutex to prevent priority inversion between
> > clients, which can cause unacceptable delays in some cases.
>
> It would be nice if you have any numbers to share in the commit text.
I can try to dig up some numbers, but mileage will vary tremendously of
course. Improvement is really only seen in certain high-concurrency
scenarios.
>
> > Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
> > ---
> >
> > We've run into a number of cases internally and from customers where
> > high priority, RT clients (typically display) become blocked for long
> > periods of time on lower priority, non-RT clients. Switching to rt_mutex
> > has proven to help performance in these cases.
>
> I am wondering if avoiding the inversion on this specific lock is the right
> solution, as there could be other locks that may cause similar issues. Do we
> see similar issue with clocks for example or is it just with interconnects?
I raised these same concerns internally, since some of the clients
experiencing delays also request clocks and regulators. However, I
believe they primarily request interconnects in these critical paths and
not clocks/regulators. At least not as frequently as they request
interconnect. Additionally, I suspect the average interconnect latencies
are higher than clock on our platforms, since interconnect will always
result in blocking calls to RPM/RPMh. I also wouldn't be surprised if we
have more consistent contention on interconnect, since certain clients
update DDR BW quite frequently. I suppose at some point the same
rt_mutex argument could be made for clock and regulator as well, but
to-date we've only needed to change interconnect to see improvement.
I'm not sure what an alternative, generic solution would be. We have
many clients requesting many different paths. Some are more
latency-sensitive and higher priority than others. If these use cases
overlap, then we're subject to these sorts of priority inversion issues.
Bumping the priority of all clients to match the highest priority one
isn't really possible.
>
> > drivers/interconnect/core.c | 80 ++++++++++++++++++-------------------
> > 1 file changed, 40 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> > index 25debded65a8..a536c013d9ca 100644
> > --- a/drivers/interconnect/core.c
> > +++ b/drivers/interconnect/core.c
> > @@ -14,7 +14,7 @@
> > #include <linux/interconnect-provider.h>
> > #include <linux/list.h>
> > #include <linux/module.h>
> > -#include <linux/mutex.h>
> > +#include <linux/rtmutex.h>
> > #include <linux/slab.h>
> > #include <linux/of.h>
> > #include <linux/overflow.h>
> > @@ -28,7 +28,7 @@ static DEFINE_IDR(icc_idr);
> > static LIST_HEAD(icc_providers);
> > static int providers_count;
> > static bool synced_state;
> > -static DEFINE_MUTEX(icc_lock);
> > +static DEFINE_RT_MUTEX(icc_lock);
> > static struct dentry *icc_debugfs_dir;
> > static void icc_summary_show_one(struct seq_file *s, struct icc_node *n)
> > @@ -47,7 +47,7 @@ static int icc_summary_show(struct seq_file *s, void *data)
> > seq_puts(s, " node tag avg peak\n");
> > seq_puts(s, "--------------------------------------------------------------------\n");
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > list_for_each_entry(provider, &icc_providers, provider_list) {
> > struct icc_node *n;
> > @@ -73,7 +73,7 @@ static int icc_summary_show(struct seq_file *s, void *data)
> > }
> > }
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > return 0;
> > }
> > @@ -104,7 +104,7 @@ static int icc_graph_show(struct seq_file *s, void *data)
> > int i;
> > seq_puts(s, "digraph {\n\trankdir = LR\n\tnode [shape = record]\n");
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > /* draw providers as cluster subgraphs */
> > cluster_index = 0;
> > @@ -136,7 +136,7 @@ static int icc_graph_show(struct seq_file *s, void *data)
> > icc_graph_show_link(s, 1, n,
> > n->links[i]);
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > seq_puts(s, "}");
> > return 0;
> > @@ -362,7 +362,7 @@ struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec)
> > if (!spec)
> > return ERR_PTR(-EINVAL);
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > list_for_each_entry(provider, &icc_providers, provider_list) {
> > if (provider->dev->of_node == spec->np) {
> > if (provider->xlate_extended) {
> > @@ -378,7 +378,7 @@ struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec)
> > }
> > }
> > }
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > if (IS_ERR(node))
> > return ERR_CAST(node);
> > @@ -490,9 +490,9 @@ struct icc_path *of_icc_get_by_index(struct device *dev, int idx)
> > return ERR_CAST(dst_data);
> > }
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > path = path_find(dev, src_data->node, dst_data->node);
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > if (IS_ERR(path)) {
> > dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> > goto free_icc_data;
> > @@ -577,12 +577,12 @@ void icc_set_tag(struct icc_path *path, u32 tag)
> > if (!path)
> > return;
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > for (i = 0; i < path->num_nodes; i++)
> > path->reqs[i].tag = tag;
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > }
> > EXPORT_SYMBOL_GPL(icc_set_tag);
> > @@ -632,7 +632,7 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> > if (WARN_ON(IS_ERR(path) || !path->num_nodes))
> > return -EINVAL;
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > old_avg = path->reqs[0].avg_bw;
> > old_peak = path->reqs[0].peak_bw;
> > @@ -664,7 +664,7 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> > apply_constraints(path);
> > }
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > trace_icc_set_bw_end(path, ret);
> > @@ -682,12 +682,12 @@ static int __icc_enable(struct icc_path *path, bool enable)
> > if (WARN_ON(IS_ERR(path) || !path->num_nodes))
> > return -EINVAL;
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > for (i = 0; i < path->num_nodes; i++)
> > path->reqs[i].enabled = enable;
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > return icc_set_bw(path, path->reqs[0].avg_bw,
> > path->reqs[0].peak_bw);
> > @@ -726,7 +726,7 @@ struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
> > struct icc_node *src, *dst;
> > struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > src = node_find(src_id);
> > if (!src)
> > @@ -748,7 +748,7 @@ struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
> > path = ERR_PTR(-ENOMEM);
> > }
> > out:
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > return path;
> > }
> > EXPORT_SYMBOL_GPL(icc_get);
> > @@ -773,14 +773,14 @@ void icc_put(struct icc_path *path)
> > if (ret)
> > pr_err("%s: error (%d)\n", __func__, ret);
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > for (i = 0; i < path->num_nodes; i++) {
> > node = path->reqs[i].node;
> > hlist_del(&path->reqs[i].req_node);
> > if (!WARN_ON(!node->provider->users))
> > node->provider->users--;
> > }
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > kfree_const(path->name);
> > kfree(path);
> > @@ -822,11 +822,11 @@ struct icc_node *icc_node_create(int id)
> > {
> > struct icc_node *node;
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > node = icc_node_create_nolock(id);
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > return node;
> > }
> > @@ -840,7 +840,7 @@ void icc_node_destroy(int id)
> > {
> > struct icc_node *node;
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > node = node_find(id);
> > if (node) {
> > @@ -848,7 +848,7 @@ void icc_node_destroy(int id)
> > WARN_ON(!hlist_empty(&node->req_list));
> > }
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > kfree(node);
> > }
> > @@ -876,7 +876,7 @@ int icc_link_create(struct icc_node *node, const int dst_id)
> > if (!node->provider)
> > return -EINVAL;
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > dst = node_find(dst_id);
> > if (!dst) {
> > @@ -900,7 +900,7 @@ int icc_link_create(struct icc_node *node, const int dst_id)
> > node->links[node->num_links++] = dst;
> > out:
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > return ret;
> > }
> > @@ -925,7 +925,7 @@ int icc_link_destroy(struct icc_node *src, struct icc_node *dst)
> > if (IS_ERR_OR_NULL(dst))
> > return -EINVAL;
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > for (slot = 0; slot < src->num_links; slot++)
> > if (src->links[slot] == dst)
> > @@ -946,7 +946,7 @@ int icc_link_destroy(struct icc_node *src, struct icc_node *dst)
> > ret = -ENOMEM;
> > out:
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > return ret;
> > }
> > @@ -962,7 +962,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
> > if (WARN_ON(node->provider))
> > return;
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > node->provider = provider;
> > list_add_tail(&node->node_list, &provider->nodes);
> > @@ -988,7 +988,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
> > node->avg_bw = 0;
> > node->peak_bw = 0;
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > }
> > EXPORT_SYMBOL_GPL(icc_node_add);
> > @@ -998,11 +998,11 @@ EXPORT_SYMBOL_GPL(icc_node_add);
> > */
> > void icc_node_del(struct icc_node *node)
> > {
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > list_del(&node->node_list);
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > }
> > EXPORT_SYMBOL_GPL(icc_node_del);
> > @@ -1041,12 +1041,12 @@ int icc_provider_add(struct icc_provider *provider)
> > if (WARN_ON(!provider->xlate && !provider->xlate_extended))
> > return -EINVAL;
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > INIT_LIST_HEAD(&provider->nodes);
> > list_add_tail(&provider->provider_list, &icc_providers);
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > dev_dbg(provider->dev, "interconnect provider added to topology\n");
> > @@ -1060,22 +1060,22 @@ EXPORT_SYMBOL_GPL(icc_provider_add);
> > */
> > void icc_provider_del(struct icc_provider *provider)
> > {
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > if (provider->users) {
> > pr_warn("interconnect provider still has %d users\n",
> > provider->users);
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > return;
> > }
> > if (!list_empty(&provider->nodes)) {
> > pr_warn("interconnect provider still has nodes\n");
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > return;
> > }
> > list_del(&provider->provider_list);
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > }
> > EXPORT_SYMBOL_GPL(icc_provider_del);
> > @@ -1110,7 +1110,7 @@ void icc_sync_state(struct device *dev)
> > if (count < providers_count)
> > return;
> > - mutex_lock(&icc_lock);
> > + rt_mutex_lock(&icc_lock);
> > synced_state = true;
> > list_for_each_entry(p, &icc_providers, provider_list) {
> > dev_dbg(p->dev, "interconnect provider is in synced state\n");
> > @@ -1123,7 +1123,7 @@ void icc_sync_state(struct device *dev)
> > }
> > }
> > }
> > - mutex_unlock(&icc_lock);
> > + rt_mutex_unlock(&icc_lock);
> > }
> > EXPORT_SYMBOL_GPL(icc_sync_state);
>
> We should also update Kconfig, otherwise i am expecting that the build will fail if
> CONFIG_RT_MUTEXES is not enabled.
You're right, it will.
>
> Thanks,
> Georgi
On 9/7/2022 10:59 PM, Mike Tipton wrote:
> On Wed, Sep 07, 2022 at 10:35:26AM +0300, Georgi Djakov wrote:
>> Hi Mike,
>>
>> Thanks for the patch!
>>
>> On 6.09.22 22:14, Mike Tipton wrote:
>>> Replace mutex with rt_mutex to prevent priority inversion between
>>> clients, which can cause unacceptable delays in some cases.
>>
>> It would be nice if you have any numbers to share in the commit text.
>
> I can try to dig up some numbers, but mileage will vary tremendously of
> course. Improvement is really only seen in certain high-concurrency
> scenarios.
We need to revisit this thread because the issue has been reported again
recently.
Here is the data I can provide regarding the performance issue. Please
check if it is sufficient for the commit message to understand the change.
The CFS normal tasks holding the mutex lock were runnable for
approximately 40ms in a busy load scenario, causing the RT task to wait
for the mutex for about 40ms, which resulted in the RT task not being
'real-time' enough and causing janks. Changing the mutex to an RT mutex
helped the caller of the interface, such as icc_set_bw, to ensure that
RT tasks can deliver RT priority to the current RT mutex owner quickly,
thereby improving performance in this scenario.
*Before the change the scenario is like:
+------------+ +-----------------+
| RT Task A | |Normal cfs task B|
+------------+
+-----------------+
mutex_lock(&icc_lock)
call icc_set_bw()
wait mutex_unlock(&icc_lock)
wait other high priority tasks
mutex_unlock(&icc_lock)
get the lock
*After the change the solution will be like:
+------------+ +-----------------+
| RT Task A | |Normal cfs task B|
+------------+ +-----------------+
rt_mutex_lock(&icc_lock)
wait other high priority task
call icc_set_bw()
rt_mutex_lock(&icc_lock)
-->boost task_B prio
Get the chance to run
-->mutex_unlock(&icc_lock)
-->deboost task_B prio
get the lock with RT prio
Please comment if more information is needed to apply the current
similar patch. If there are no objections, we can rebase and upload a
new patch set accordingly.
>
>>
>>> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com>
>>> ---
>>>
>>> We've run into a number of cases internally and from customers where
>>> high priority, RT clients (typically display) become blocked for long
>>> periods of time on lower priority, non-RT clients. Switching to rt_mutex
>>> has proven to help performance in these cases.
>>
>> I am wondering if avoiding the inversion on this specific lock is the right
>> solution, as there could be other locks that may cause similar issues. Do we
>> see similar issue with clocks for example or is it just with interconnects?
The current issue has been captured in multiple projects, while it has
only been reported in interconnects so far. We need to understand
whether specific mutex locks such as those from clocks, are being called
by RT tasks in any possible scenarios.
>
> I raised these same concerns internally, since some of the clients
> experiencing delays also request clocks and regulators. However, I
> believe they primarily request interconnects in these critical paths and
> not clocks/regulators. At least not as frequently as they request
> interconnect. Additionally, I suspect the average interconnect latencies
> are higher than clock on our platforms, since interconnect will always
> result in blocking calls to RPM/RPMh. I also wouldn't be surprised if we
> have more consistent contention on interconnect, since certain clients
> update DDR BW quite frequently. I suppose at some point the same
> rt_mutex argument could be made for clock and regulator as well, but
> to-date we've only needed to change interconnect to see improvement.
>
> I'm not sure what an alternative, generic solution would be. We have
> many clients requesting many different paths. Some are more
> latency-sensitive and higher priority than others. If these use cases
> overlap, then we're subject to these sorts of priority inversion issues.
> Bumping the priority of all clients to match the highest priority one
> isn't really possible.
Based on my understanding, rt_mutex is a good API to solve this type of
issue.
--
Thx and BRs,
Aiqun(Maria) Yu
On Thu, Apr 17, 2025 at 07:13:52PM +0800, Aiqun(Maria) Yu wrote: > On 9/7/2022 10:59 PM, Mike Tipton wrote: > > On Wed, Sep 07, 2022 at 10:35:26AM +0300, Georgi Djakov wrote: > >> Hi Mike, > >> > >> Thanks for the patch! > >> > >> On 6.09.22 22:14, Mike Tipton wrote: > >>> Replace mutex with rt_mutex to prevent priority inversion between > >>> clients, which can cause unacceptable delays in some cases. > >> > >> It would be nice if you have any numbers to share in the commit text. > > > > I can try to dig up some numbers, but mileage will vary tremendously of > > course. Improvement is really only seen in certain high-concurrency > > scenarios. > > We need to revisit this thread because the issue has been reported again > recently. > Here is the data I can provide regarding the performance issue. Please > check if it is sufficient for the commit message to understand the change. > The CFS normal tasks holding the mutex lock were runnable for > approximately 40ms in a busy load scenario, causing the RT task to wait > for the mutex for about 40ms, which resulted in the RT task not being > 'real-time' enough and causing janks. Changing the mutex to an RT mutex > helped the caller of the interface, such as icc_set_bw, to ensure that > RT tasks can deliver RT priority to the current RT mutex owner quickly, > thereby improving performance in this scenario. I'll post an updated and rebased version of this patch soon. In addition to the display use cases, it was recently reported for certain other GPU use cases as well. > > > > > I'm not sure what an alternative, generic solution would be. We have > > many clients requesting many different paths. Some are more > > latency-sensitive and higher priority than others. If these use cases > > overlap, then we're subject to these sorts of priority inversion issues. > > Bumping the priority of all clients to match the highest priority one > > isn't really possible. > > Based on my understanding, rt_mutex is a good API to solve this type of > issue. I agree and still don't see any generic solution aside from this.
© 2016 - 2026 Red Hat, Inc.