Messages in this thread | | | Date | Tue, 10 Dec 2019 20:25:38 +0100 | From | Peter Zijlstra <> | Subject | Re: Problem with WARN_ON in mutex_trylock() and rxrpc |
| |
On Tue, Dec 10, 2019 at 07:33:17PM +0100, Thomas Gleixner wrote: > Peter Zijlstra <peterz@infradead.org> writes: > > On Thu, Dec 05, 2019 at 12:02:24PM +0000, David Howells wrote:
> > To recap the IRC discussion; the intended mutex semantics are such to > > allow Priority Inheritance. This means that the mutex must be locked and > > unlocked in the same (task) context. Otherwise there is no distinct > > owner to boost for contending mutex_lock() operations. > > > > Since (soft)irq context doesn't (necessarily) have a task context, these > > operations don't strictly make sense, and that is what the patch in > > question tries to WARN about. > > Not only that. Acquiring something which is _NOT_ designed for non > thread context works by chance not by design. IOW it makes assumptions > about the underlying mutex implementation and any change to that which > actually assumes thread context will break that. So, no we don't want > I'm clever and can do that as the implementation allows, simply because > this is a blatant layering violation.
AFAICT the only assumption it relies on are:
- that the softirq will cleanly preempt a task. That is, the task context must not change under the softirq execution.
- that the softirq runs non-preemptible.
Now, both these properties are rather fundamental to how our softirqs work. And can, therefore, be relied upon, irrespective of the mutex implementation.
> > As it happens, you do mutex_unlock() from the very same softirq context > > you do that mutex_trylock() in, so lockdep will never have had cause to > > complain, 'current' is the same at acquire and release. > > > > Now, either we're in non-preemptible softirq context and a contending > > mutex_lock() would spuriously boost a random task, which is harmless due > > to the non-preemptive nature of softirq, or we're running in ksoftirqd > > and that gets boosted, which actually makes some sense. > > > > For PREEMPT_RT (the only case that really matters, since that actually > > replaces struct mutex with rt_mutex) this would result in boosting > > whatever (soft)irq thread ended up running the thing. > > Well, that'd "work". Actually in RT this makes even sense as the > contending waiter wants the owner out of the critical region ASAP>
The only funny I could come up with is if current == idle, because in that case we'll attempt to boost idle. And that is a major no-no. The proxy execution patches will actually run into this :/
> > (Also, I'm not entire sure on the current softirq model for -RT) > > > > Is this something we want to allow? > > I'm not a fan. See above.
Yeah, I'm pretty adverse to it too. But I'm not sure what to suggest David do instead. Clearly semaphores are an option, but perhaps there's something better; I've not yet tried to understand his code.
| |