Re: [PATCH 1/2] hung_task: Show the blocker task if the task is hung on mutex

[Waiman Long]

On 2/20/25 4:29 AM, Masami Hiramatsu (Google) wrote:
> On Wed, 19 Feb 2025 22:37:04 -0500
> Waiman Long <llong@xxxxxxxxxx> wrote:
>
> > On 2/19/25 9:59 PM, Masami Hiramatsu (Google) wrote:
> > > On Wed, 19 Feb 2025 21:15:08 -0500
> > > Waiman Long <llong@xxxxxxxxxx> wrote:
> > >
> > > > On 2/19/25 8:41 PM, Steven Rostedt wrote:
> > > > > On Wed, 19 Feb 2025 20:36:13 -0500
> > > > > Waiman Long <llong@xxxxxxxxxx> wrote:
> > > > >
> > > > >     
> > > > > > > > > > this field, we don't need to take lock, though taking the wait_lock may
> > > > > > > > > > still be needed to examine other information inside the mutex.
> > > > > > > Do we need to take it just for accessing owner, which is in an atomic?
> > > > > > Right. I forgot it is an atomic_long_t. In that case, no lock should be
> > > > > > needed.
> > > > > Now if we have a two fields to read:
> > > > >
> > > > > 	block_flags (for the type of lock) and blocked_on (for the lock)
> > > > >
> > > > > We need a way to synchronize the two. What happens if we read the type, and
> > > > > the task wakes up and and then blocks on a different type of lock?
> > > > >
> > > > > Then the lock read from blocked_on could be a different type of lock than
> > > > > what is expected.
> > > > That is different from reading the owner. In this case, we need to use
> > > > smp_rmb()/wmb() to sequence the read and write operations unless it is
> > > > guaranteed that they are in the same cacheline. One possible way is as
> > > > follows:
> > > >
> > > > Writer - setting them:
> > > >
> > > >        WRITE_ONCE(lock)
> > > >        smp_wmb()
> > > >        WRITE_ONCE(type)
> > > >
> > > > Clearing them:
> > > >
> > > >        WRITE_ONCE(type, 0)
> > > >        smp_wmb()
> > > >        WRITE_ONCE(lock, NULL)
> > > >
> > > > Reader:
> > > >
> > > >        READ_ONCE(type)
> > > > again:
> > > >        smp_rmb()
> > > >        READ_ONCE(lock)
> > > >        smp_rmb()
> > > >        if (READ_ONCE(type) != type)
> > > >            goto again
> > > What about mutex-rwsem-mutex case?
> > >
> > > mutex_lock(&lock1);
> > > down_read(&lock2);
> > > mutex_lock(&lock3);
> > >
> > > The worst scenario is;
> > >
> > > WRITE_ONCE(lock, &lock1)
> > > smp_wmb()
> > > WRITE_ONCE(type, MUTEX)     READ_ONCE(type) -> MUTEX
> > > WRITE_ONCE(type, 0)
> > > smp_wmb()
> > > WRITE_ONCE(lock, NULL)
> > > WRITE_ONCE(lock, &lock2)    READ_ONCE(lock) -> &lock2
> > > smp_wmb()
> > > WRITE_ONCE(type, RWSEM)
> > > WRITE_ONCE(type, 0)
> > > smp_wmb()
> > > WRITE_ONCE(lock, NULL)
> > > WRITE_ONCE(lock, &lock3)
> > > smp_wmb()
> > > WRITE_ONCE(type, MUTEX)     READ_ONCE(type) -> MUTEX == MUTEX
> > > WRITE_ONCE(type, 0)
> > > smp_wmb()
> > > WRITE_ONCE(lock, NULL)
> > >
> > >                               "OK, lock2 is a MUTEX!"
> > >
> > > So unless stopping the blocker task, we can not ensure this works.
> > > But unless decode the lock, we don't know the blocker task.
> > That could only happen if the reader can get interrupted/preempted for a
> > long time. In that case, we may need to reread the lock again to be sure
> > that they are stable.
> Hm, actually read side should run under rcu read locked, so only interrupt
> matters. So I think this rarely happens.
>
> BTW, we don't need the lock address itself, but we need to know who is the
> owner. Maybe we can point the address of atomic_long_t?
>
> struct task_struct {
> 	atomic_long_t *blocked_on_owner;
> };
Yes, we can use a pointer to the owner field. However, the way that 
owner field is encoded varies depends on the lock type. So we still need 
to know what lock it is. As mentioned in the other thread, we can encode 
the lock type into the lowest 2 bits of the pointer.
> The problem is that mutex and rwsem are OK, but rt_mutex uses task_struct *.
> Maybe we can use atomic_long_t in rt_mutex too if the new Kconfig is enabled.

It shouldn't be a problem to use atomic_long_t for the owner.

Cheers,
Longman