Re: Problem with WARN_ON in mutex_trylock() and rxrpc

[Peter Zijlstra]

On Tue, Dec 10, 2019 at 07:33:17PM +0100, Thomas Gleixner wrote:
> Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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.