[v1] pstore: Switch pmsg_lock to an rt_mutex to avoid priority inversion

[RFC PATCH] pstore: Switch pmsg_lock to an rt_mutex to avoid priority inversion

Posted by John Stultz 3 years, 4 months ago

Wei Wang reported seeing priority inversion caused latencies
caused by contention on pmsg_lock, and suggested it be switched
to a rt_mutex.

I was initially hesitant this would help, as the tasks in that
trace all seemed to be SCHED_NORMAL, so the benefit would be
limited to only nice boosting.

However, another similar issue was raised where the priority
inversion was seen did involve a blocked RT task so it is clear
this would be helpful in that case.

Feedback would be appreciate!

Cc: Wei Wang <wvw@google.com>
Cc: Midas Chien<midaschieh@google.com>
Cc: Connor O'Brien <connoro@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: kernel-team@android.com
Reported-by: Wei Wang <wvw@google.com>
Signed-off-by: John Stultz <jstultz@google.com>
---
 fs/pstore/pmsg.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index d8542ec2f38c..18cf94b597e0 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -7,9 +7,10 @@
 #include <linux/device.h>
 #include <linux/fs.h>
 #include <linux/uaccess.h>
+#include <linux/rtmutex.h>
 #include "internal.h"
 
-static DEFINE_MUTEX(pmsg_lock);
+static DEFINE_RT_MUTEX(pmsg_lock);
 
 static ssize_t write_pmsg(struct file *file, const char __user *buf,
 			  size_t count, loff_t *ppos)
@@ -28,9 +29,9 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
 	if (!access_ok(buf, count))
 		return -EFAULT;
 
-	mutex_lock(&pmsg_lock);
+	rt_mutex_lock(&pmsg_lock);
 	ret = psinfo->write_user(&record, buf);
-	mutex_unlock(&pmsg_lock);
+	rt_mutex_unlock(&pmsg_lock);
 	return ret ? ret : count;
 }
 
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [RFC PATCH] pstore: Switch pmsg_lock to an rt_mutex to avoid priority inversion

Posted by Kees Cook 3 years, 3 months ago

On Mon, Dec 05, 2022 at 08:32:53PM +0000, John Stultz wrote:
> Wei Wang reported seeing priority inversion caused latencies
> caused by contention on pmsg_lock, and suggested it be switched
> to a rt_mutex.
> 
> I was initially hesitant this would help, as the tasks in that
> trace all seemed to be SCHED_NORMAL, so the benefit would be
> limited to only nice boosting.
> 
> However, another similar issue was raised where the priority
> inversion was seen did involve a blocked RT task so it is clear
> this would be helpful in that case.
> 
> Feedback would be appreciate!

This looks fine to me. Is there an appropriate "Fixes:" tag that could
be used?

-Kees

> 
> Cc: Wei Wang <wvw@google.com>
> Cc: Midas Chien<midaschieh@google.com>
> Cc: Connor O'Brien <connoro@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: kernel-team@android.com
> Reported-by: Wei Wang <wvw@google.com>
> Signed-off-by: John Stultz <jstultz@google.com>
> ---
>  fs/pstore/pmsg.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
> index d8542ec2f38c..18cf94b597e0 100644
> --- a/fs/pstore/pmsg.c
> +++ b/fs/pstore/pmsg.c
> @@ -7,9 +7,10 @@
>  #include <linux/device.h>
>  #include <linux/fs.h>
>  #include <linux/uaccess.h>
> +#include <linux/rtmutex.h>
>  #include "internal.h"
>  
> -static DEFINE_MUTEX(pmsg_lock);
> +static DEFINE_RT_MUTEX(pmsg_lock);
>  
>  static ssize_t write_pmsg(struct file *file, const char __user *buf,
>  			  size_t count, loff_t *ppos)
> @@ -28,9 +29,9 @@ static ssize_t write_pmsg(struct file *file, const char __user *buf,
>  	if (!access_ok(buf, count))
>  		return -EFAULT;
>  
> -	mutex_lock(&pmsg_lock);
> +	rt_mutex_lock(&pmsg_lock);
>  	ret = psinfo->write_user(&record, buf);
> -	mutex_unlock(&pmsg_lock);
> +	rt_mutex_unlock(&pmsg_lock);
>  	return ret ? ret : count;
>  }
>  
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog
> 

-- 
Kees Cook

Re: [RFC PATCH] pstore: Switch pmsg_lock to an rt_mutex to avoid priority inversion

Posted by John Stultz 3 years, 3 months ago

On Wed, Dec 14, 2022 at 2:53 PM Kees Cook <keescook@chromium.org> wrote:
> On Mon, Dec 05, 2022 at 08:32:53PM +0000, John Stultz wrote:
> > Wei Wang reported seeing priority inversion caused latencies
> > caused by contention on pmsg_lock, and suggested it be switched
> > to a rt_mutex.
> >
> > I was initially hesitant this would help, as the tasks in that
> > trace all seemed to be SCHED_NORMAL, so the benefit would be
> > limited to only nice boosting.
> >
> > However, another similar issue was raised where the priority
> > inversion was seen did involve a blocked RT task so it is clear
> > this would be helpful in that case.
> >
> > Feedback would be appreciate!
>
> This looks fine to me. Is there an appropriate "Fixes:" tag that could
> be used?

Not other than the ~3.19 era original introduction of the pmsg.c file
where that mutex was introduced, I don't think.

But I'll respin with that.

thanks
-john