public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: John Stultz <jstultz@google.com>
To: LKML <linux-kernel@vger.kernel.org>
Cc: "John Stultz" <jstultz@google.com>, "Wei Wang" <wvw@google.com>,
	"Midas Chien" <midaschieh@google.com>,
	"Chunhui Li (李春辉)" <chunhui.li@mediatek.com>,
	"Steven Rostedt" <rostedt@goodmis.org>,
	"Kees Cook" <keescook@chromium.org>,
	"Anton Vorontsov" <anton@enomsg.org>,
	"Guilherme G. Piccoli" <gpiccoli@igalia.com>,
	"Tony Luck" <tony.luck@intel.com>,
	kernel-team@android.com
Subject: [PATCH v2] pstore: Revert pmsg_lock back to a normal mutex
Date: Sat,  4 Mar 2023 03:10:29 +0000	[thread overview]
Message-ID: <20230304031029.3037914-1-jstultz@google.com> (raw)
In-Reply-To: <20230302062741.483079-1-jstultz@google.com>

This reverts commit 76d62f24db07f22ccf9bc18ca793c27d4ebef721.

So while priority inversion on the pmsg_lock is an occasional
problem that an rt_mutex would help with, in uses where logging
is writing to pmsg heavily from multiple threads, the pmsg_lock
can be heavily contended.

Normal mutexes can do adaptive spinning, which keeps the
contention overhead fairly low maybe adding on the order of 10s
of us delay waiting, but the slowpath w/ rt_mutexes makes the
blocked tasks sleep & wake. This makes matters worse when there
is heavy contentention, as it just allows additional threads to
run and line up to try to take the lock.

It devolves to a worse case senerio where the lock acquisition
and scheduling overhead dominates, and each thread is waiting on
the order of ~ms to do ~us of work.

Obviously, having tons of threads all contending on a single
lock for logging is non-optimal, so the proper fix is probably
reworking pstore pmsg to have per-cpu buffers so we don't have
contention.

But in the short term, lets revert the change to the rt_mutex
and go back to normal mutexes to avoid a potentially major
performance regression.

Cc: Wei Wang <wvw@google.com>
Cc: Midas Chien<midaschieh@google.com>
Cc: "Chunhui Li (李春辉)" <chunhui.li@mediatek.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: kernel-team@android.com
Fixes: 76d62f24db07 ("pstore: Switch pmsg_lock to an rt_mutex to avoid priority inversion")
Reported-by: "Chunhui Li (李春辉)" <chunhui.li@mediatek.com>
Tested-by: Chunhui Li <chunhui.li@mediatek.com>
Signed-off-by: John Stultz <jstultz@google.com>
---
I know Steven is working on a fix to address the rtmutex not
spinning, but as the earlier version of it didn't resolve the
issue for Chunhui Li, I wanted to resend this out again w/
Tested-by tags, so it is ready to go if needed. I am looking
to get a local reproducer so I can help validate Steven's
efforts.

v2:
* Fix quoting around Chunhui Li's email name (so they are actually
  cc'ed)
* Added tested by tag
---
 fs/pstore/pmsg.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

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


  parent reply	other threads:[~2023-03-04  3:10 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-02  6:27 [PATCH] pstore: Revert pmsg_lock back to a normal mutex John Stultz
2023-03-02 13:24 ` Steven Rostedt
2023-03-02 19:39   ` John Stultz
2023-03-02 20:21     ` Steven Rostedt
2023-03-02 21:32       ` Steven Rostedt
2023-03-02 21:36         ` Steven Rostedt
2023-03-02 21:56           ` Steven Rostedt
2023-03-03  1:01             ` Steven Rostedt
2023-03-03 18:11               ` Joel Fernandes
2023-03-03 18:37                 ` Steven Rostedt
2023-03-03 19:25                   ` Joel Fernandes
2023-03-03 19:38                     ` Steven Rostedt
2023-03-03 20:36                       ` Qais Yousef
2023-03-04  3:21                         ` Joel Fernandes
2023-03-06 19:19                           ` Qais Yousef
2023-03-04  3:01                       ` Joel Fernandes
2023-03-04  3:23                         ` Joel Fernandes
2023-03-07 14:08                 ` Peter Zijlstra
2023-03-07 20:19                   ` Joel Fernandes
2023-03-06 18:30               ` John Stultz
2023-03-08  1:31               ` Steven Rostedt
2023-03-08 20:04                 ` John Stultz
2023-03-08 20:41                   ` Steven Rostedt
2023-03-02 22:41       ` David Laight
2023-03-02 22:53         ` Steven Rostedt
2023-03-04  3:10 ` John Stultz [this message]
2023-03-05 16:36   ` [PATCH v2] " Steven Rostedt
2023-03-06 18:27     ` John Stultz
     [not found]   ` <20230306010323.2909-1-hdanton@sina.com>
2023-03-06 15:28     ` Steven Rostedt
     [not found]     ` <20230307003106.1768-1-hdanton@sina.com>
2023-03-07  1:58       ` Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230304031029.3037914-1-jstultz@google.com \
    --to=jstultz@google.com \
    --cc=anton@enomsg.org \
    --cc=chunhui.li@mediatek.com \
    --cc=gpiccoli@igalia.com \
    --cc=keescook@chromium.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=midaschieh@google.com \
    --cc=rostedt@goodmis.org \
    --cc=tony.luck@intel.com \
    --cc=wvw@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox