Re: [PATCH v2] pstore: Revert pmsg_lock back to a normal mutex

Re: [PATCH v2] pstore: Revert pmsg_lock back to a normal mutex

[Hillf Danton]

On Sun, 5 Mar 2023 11:36:32 -0500 Steven Rostedt <rostedt@goodmis.org>
> On Sat, 4 Mar 2023 03:10:29 +0000 John Stultz <jstultz@google.com> wrote:
> 
> > 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.
> 
> That is incorrect. rt_mutexes have pretty much the same adaptive spinning
> as normal mutexes. So that is *not* the reason for the difference in
> performance, and should not be stated so in this change log.
> 
> The difference between rt_mutex and normal mutex, is that the rt_mutex will
> trigger priority inheritance. Perhaps on high contention, that makes a
> difference. But do not state it's because rt_mutex does not have adaptive
> spinning, because it most definitely does.

Another difference between rt_mutex and mutex is that mutex has another
optimistic spin with the first waiter ignored, and that can be tested by
simply skipping it. Perhaps because of the fact that it makes no sense for
other waiters than the first one to spin on owner in rt context.

PS what sense made by spinning on owner until need_resched() with preempt
disabled in the non-rt context?

Re: [PATCH v2] pstore: Revert pmsg_lock back to a normal mutex

[Steven Rostedt]

On Mon,  6 Mar 2023 09:03:23 +0800
Hillf Danton <hdanton@sina.com> wrote:

> PS what sense made by spinning on owner until need_resched() with preempt
> disabled in the non-rt context?

Not sure what the question you have here is? If need_resched() is set, we
want to schedule out.

-- Steve

Re: [PATCH v2] pstore: Revert pmsg_lock back to a normal mutex

[Hillf Danton]

On Mon, 6 Mar 2023 10:28:44 -0500 Steven Rostedt <rostedt@goodmis.org>
> On Mon, 6 Mar 2023 09:03:23 +0800 Hillf Danton <hdanton@sina.com> wrote:
> >
> > PS what sense made by spinning on owner until need_resched() with preempt
> > disabled in the non-rt context?
> 
> Not sure what the question you have here is? If need_resched() is set, we
> want to schedule out.

Given the critical section under mutex could be preempted, what is hard to
understand is the wakeup of a ten-minute sleeper could not preempt a nice-10
mutex spinner for instance.

Re: [PATCH v2] pstore: Revert pmsg_lock back to a normal mutex

[Steven Rostedt]

On Tue,  7 Mar 2023 08:31:06 +0800
Hillf Danton <hdanton@sina.com> wrote:

> On Mon, 6 Mar 2023 10:28:44 -0500 Steven Rostedt <rostedt@goodmis.org>
> > On Mon, 6 Mar 2023 09:03:23 +0800 Hillf Danton <hdanton@sina.com> wrote:  
> > >
> > > PS what sense made by spinning on owner until need_resched() with preempt
> > > disabled in the non-rt context?  
> > 
> > Not sure what the question you have here is? If need_resched() is set, we
> > want to schedule out.  
> 
> Given the critical section under mutex could be preempted, what is hard to
> understand is the wakeup of a ten-minute sleeper could not preempt a nice-10
> mutex spinner for instance.

But it can. Most wakeups are done by interrupts or softirqs. Both of which
will happen even if a task is running with preemption disabled.

-- Steve