Re: Thread Safety Analysis and the Linux kernel

Peter Zijlstra
2025-03-06T10:08:32Z

On Wed, Mar 05, 2025 at 11:54:14PM +0000, Puchert, Aaron wrote:

> > Peter Zijlstra's feedback:
> > https://lore.kernel.org/all/20250305112041.GA16878@noisy.programming.kicks-ass.net/
> > -- much of which led to the below requests. Peter also managed to
> > crash Clang, but that's probably unrelated to -Wthread-safety
> > directly: https://github.com/llvm/llvm-project/issues/129873
>
> Technically a parser issue and not in the analysis itself. But to my
> knowledge expressions in attributes were originally introduced for the
> analysis, so in some sense it is related.

Right, so that construct came about trying to work around not being able
to reference the return value.

> > 3. Ability to refer to locks in returned reference/pointer. For example:
> > struct foo *ret_lock_struct(void) ACQUIRE(return->somelock);
> > struct foo *try_ret_lock_struct(void) TRY_ACQUIRE(1,
> > return->somelock); // locked if non-NULL
> > I expect this also requires basic alias analysis to work so that
> > assigning the returned pointer to a function-local variable and then
> > later use in an unlock function works as expected.
>
> We had a discussion about this, maybe I can find it later. The idea
> was to introduce a builtin for the return value. A rough outline:
> * Introduce the builtin to the parser, say __builtin_return_value().
> This shouldn't be hard.
> * Add handling to Sema: we can only accept the builtin in attributes
> on functions. The return type is the return type of the function.
> * In the caller we might not need alias analysis: there are no
> variables that alias. We already produce S-expressions for some return
> values from functions, and then handle a DeclStmt with initializer. (I
> introduced that for some C++ patterns in
> https://reviews.llvm.org/D129755.)
> * In the function itself we need to substitute the return expression
> for the builtin before checking the exit set. (I assume this restricts
> the possible attributes to acquire-type attributes.)
>
> We can also discuss this in more detail if you want to pick it up. It
> might be quite a bit of work, but it should fit nicely into the
> existing framework.

Yes, as far as I can follow, that should work nicely.

The thing I tried to work around yesterday, making clang ICE, was
something like this function:

struct rq *__task_rq_lock(struct task_struct *p)
__must_hold(p->pi_lock)
__acquires(_Return->__lock)
{
struct rq *rq;

lockdep_assert_held(&p->pi_lock);

for (;;) {
rq = task_rq(p);
raw_spin_rq_lock(rq);
if (likely(rq == task_rq(p) && !task_on_rq_migrating(p)))
return rq;

raw_spin_rq_unlock(rq);

while (unlikely(task_on_rq_migrating(p)))
cpu_relax();
}
}

Except for not having that _Return / __builtin_return_value() thing, I
tried __acquires(task_rq(p)->__lock), and task_rq() expands into a pile
of gunk that made it go *boom*.

Users would typically look like:

try_to_wake_up(p, state)
{
struct rq *rq;

scoped_guard (raw_spinlock_irqsave, &p->pi_lock) {
if (!ttwu_state_match(p, state))
break;

rq = __task_rq_lock(p);
// go enqueue task
raw_spin_rq_unlock(rq);
}
}

Anyway, the whole 'find-and-lock' pattern is widely used.