[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87cy99g3k6.ffs@tglx>
Date: Tue, 05 Aug 2025 18:02:01 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: zhongjinji@...or.com, linux-mm@...ck.org
Cc: akpm@...ux-foundation.org, mhocko@...e.com, rientjes@...gle.com,
shakeel.butt@...ux.dev, npache@...hat.com, linux-kernel@...r.kernel.org,
mingo@...hat.com, peterz@...radead.org, dvhart@...radead.org,
dave@...olabs.net, andrealmeid@...lia.com, liulu.liu@...or.com,
feng.han@...or.com
Subject: Re: [[PATCH v2] 1/2] futex: Add check_robust_futex to verify
process usage of robust_futex
On Fri, Aug 01 2025 at 23:36, zhongjinji@...or.com wrote:
Please use foo() notation for functions in subject and change log.
> The check_robust_futex function is added to detect whether a process uses
> robust_futex.
Explain the problem first and do not start with what the patch is doing.
> According to the patch discussion
> (https://lore.kernel.org/all/20220414144042.677008-1-npache@redhat.com/T/#u),
Can you properly describe what you are trying to solve as part of the
change log? A link can be provided for further information, but not
instead of a proper explanation.
> executing the OOM reaper too early on processes using robust_futex may cause
> the lock holder to wait indefinitely.
>
> Therefore, this patch introduces check_robust_futex to identify such
# git grep 'This patch' Documentation/process/
See also:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> +bool __check_robust_futex(struct task_struct *p)
> +{
> + struct task_struct *t;
> +
> + for_each_thread(p, t) {
> + if (unlikely(t->robust_list))
This is a racy access as the thread might concurrently write to it. So
it has to be annotated with data_race().
> + return true;
> +#ifdef CONFIG_COMPAT
> + if (unlikely(t->compat_robust_list))
> + return true;
> +#endif
> + }
> + return false;
> +}
> +
> +bool check_robust_futex(struct task_struct *p)
The name sucks. Public futex functions are prefixed with
futex.
But this is about checking a process, no? So something like
process_has_robust_futex() makes it clear what this is about.
> +{
> + bool has_robust;
> +
> + rcu_read_lock();
> + has_robust = __check_robust_futex(p);
> + rcu_read_unlock();
> + return has_robust;
> +}
Why do you need two functions here?
If the OOM killer is invoked, then saving a rcu_read_lock()/unlock() is
just a pointless optimization with zero value. rcu_read_lock() nests
nicely.
But I'm not convinced yet, that this is actually a sane approach.
Thanks,
tglx
Powered by blists - more mailing lists