Messages in this thread | | | Date | Sat, 4 Mar 2023 03:01:30 +0000 | From | Joel Fernandes <> | Subject | Re: [PATCH] pstore: Revert pmsg_lock back to a normal mutex |
| |
Hey Steve,
On Fri, Mar 03, 2023 at 02:38:22PM -0500, Steven Rostedt wrote: > On Fri, 3 Mar 2023 14:25:23 -0500 > Joel Fernandes <joel@joelfernandes.org> wrote: > > > On Fri, Mar 3, 2023 at 1:37 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > On Fri, 3 Mar 2023 18:11:34 +0000 > > > Joel Fernandes <joel@joelfernandes.org> wrote: > > > > > > > In the normal mutex's adaptive spinning, there is no check for if there is a > > > > change in waiter AFAICS (ignoring ww mutex stuff for a second). > > > > > > > > I can see one may want to do that waiter-check, as spinning > > > > indefinitely if the lock owner is on the CPU for too long may result in > > > > excessing power burn. But normal mutex does not seem to do that. > > > > > > > > What makes the rtmutex spin logic different from normal mutex in this > > > > scenario, so that rtmutex wants to do that but normal ones dont? > > > > > > Well, the point of the patch is that I don't think they should be different > > > ;-) > > > > But there's no "waiter change" thing for mutex_spin_on_owner right. > > > > Then, should mutex_spin_on_owner() also add a call to > > __mutex_waiter_is_first() ? > > Ah interesting, I missed the __mutex_waiter_is_first() in the mutex code, > where it looks to do basically the same thing as rt_mutex (but slightly > different).
True, I concur!
> From looking at this, it appears that mutex() has FIFO fair > logic, where the second waiter will sleep. > > Would be interesting to see why John sees such a huge difference between > normal mutex and rtmutex if they are doing the same thing. One thing is > perhaps the priority logic is causing the issue, where this will not > improve anything.
> I wonder if we add spinning to normal mutex for the other waiters if that > would improve things or make them worse?
Yeah it could improve things (or make them worse ;-). In that approach, I guess those other waiters will not be spinning for too long once the first waiter gets the lock, as mutex_spin_on_owner() it will break out of the spin:
while (__mutex_owner(lock) == owner) { ... }
But yeah it sounds like a plausible idea.
> > > > Another thought is, I am wondering if all of them spinning indefinitely might > > > > be Ok for rtmutex as well, since as you mentioned, preemption is enabled. So > > > > adding the if (top_waiter != last_waiter) {...} might be unnecessary? In fact > > > > may be even harmful as you are disabling interrupts in the process. > > > > > > The last patch only does the interrupt disabling if the top waiter changes. > > > Which in practice is seldom. > > > > > > But, I don't think a non top waiter should spin if the top waiter is not > > > running. The top waiter is the one that will get the lock next. If the > > > owner releases the lock and gives it to the top waiter, then it has to go > > > through the wake up of the top waiter.
Ah ok. I see what you're doing now!
I guess that check will help whenever the top-waiter keeps changing. In that case you want only the latest top-waiter as the spinner, all while the lock owner is not changing.
> > Correct me if I'm wrong, but I don't think it will go through > > schedule() after spinning, which is what adds the overhead for John. > > Only if the lock becomes free.
Ah yeah, true!
> > > I don't see why a task should spin > > > to save a wake up if a wake up has to happen anyway. > > > > What about regular mutexes, happens there too or no? > > Yes, but in a FIFO order, where in rt_mutex, a second, higher priority task > can make the first ones sleep. So maybe it's just the priority logic that > is causing the issues.
True! So in the FIFO thing, there's no way a high prio task can get ahead in line of the first waiter, makes sense.
> > > > Either way, I think a comment should go on top of the "if (top_waiter != > > > > waiter)" check IMO. > > > > > > What type of comment? > > > > Comment explaining why "if (top_waiter != waiter)" is essential :-). >
Maybe "/* Only the top waiter needs to spin. If we are no longer the top-waiter, no point in spinning, as we do not get the lock next anyway. */"
?
thanks,
- Joel
| |