[PATCH v2] mm/oom_kill: simplify remaining RCU sections with scoped_guard(rcu)

[PATCH v2] mm/oom_kill: simplify remaining RCU sections with scoped_guard(rcu)

[Ye Liu]

From: Ye Liu <liuye@kylinos.cn>

Replace the remaining manual rcu_read_lock()/rcu_read_unlock() pairs
in oom_cpuset_eligible(), select_bad_process(), dump_tasks(),
task_will_free_mem(), and __oom_kill_process() with scoped_guard(rcu)
for consistency and simpler control flow.  scoped_guard(rcu) limits
the RCU critical section to the loop body rather than the entire
remaining function scope, making the protected region explicit and
safer for future changes.

Signed-off-by: Ye Liu <liuye@kylinos.cn>
---
v2:
 - Use scoped_guard instead of guard.
 - Link：https://lore.kernel.org/all/20260813032634.344946-1-ye.liu@linux.dev/

 mm/oom_kill.c | 124 +++++++++++++++++++++++++-------------------------
 1 file changed, 62 insertions(+), 62 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 5f372f6e26fa..0e8982e5be51 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -94,27 +94,27 @@ static bool oom_cpuset_eligible(struct task_struct *start,
 	bool ret = false;
 	const nodemask_t *mask = oc->nodemask;
 
-	rcu_read_lock();
-	for_each_thread(start, tsk) {
-		if (mask) {
-			/*
-			 * If this is a mempolicy constrained oom, tsk's
-			 * cpuset is irrelevant.  Only return true if its
-			 * mempolicy intersects current, otherwise it may be
-			 * needlessly killed.
-			 */
-			ret = mempolicy_in_oom_domain(tsk, mask);
-		} else {
-			/*
-			 * This is not a mempolicy constrained oom, so only
-			 * check the mems of tsk's cpuset.
-			 */
-			ret = cpuset_mems_allowed_intersects(current, tsk);
+	scoped_guard(rcu) {
+		for_each_thread(start, tsk) {
+			if (mask) {
+				/*
+				 * If this is a mempolicy constrained oom, tsk's
+				 * cpuset is irrelevant.  Only return true if its
+				 * mempolicy intersects current, otherwise it may be
+				 * needlessly killed.
+				 */
+				ret = mempolicy_in_oom_domain(tsk, mask);
+			} else {
+				/*
+				 * This is not a mempolicy constrained oom, so only
+				 * check the mems of tsk's cpuset.
+				 */
+				ret = cpuset_mems_allowed_intersects(current, tsk);
+			}
+			if (ret)
+				break;
 		}
-		if (ret)
-			break;
 	}
-	rcu_read_unlock();
 
 	return ret;
 }
@@ -368,11 +368,11 @@ static void select_bad_process(struct oom_control *oc)
 	else {
 		struct task_struct *p;
 
-		rcu_read_lock();
-		for_each_process(p)
-			if (oom_evaluate_task(p, oc))
-				break;
-		rcu_read_unlock();
+		scoped_guard(rcu) {
+			for_each_process(p)
+				if (oom_evaluate_task(p, oc))
+					break;
+		}
 	}
 }
 
@@ -430,14 +430,14 @@ static void dump_tasks(struct oom_control *oc)
 		struct task_struct *p;
 		int i = 0;
 
-		rcu_read_lock();
-		for_each_process(p) {
-			/* Avoid potential softlockup warning */
-			if ((++i & 1023) == 0)
-				touch_softlockup_watchdog();
-			dump_task(p, oc);
+		scoped_guard(rcu) {
+			for_each_process(p) {
+				/* Avoid potential softlockup warning */
+				if ((++i & 1023) == 0)
+					touch_softlockup_watchdog();
+				dump_task(p, oc);
+			}
 		}
-		rcu_read_unlock();
 	}
 }
 
@@ -894,17 +894,17 @@ static bool task_will_free_mem(struct task_struct *task)
 	 * are dying as well to make sure that a) nobody pins its mm and
 	 * b) the task is also reapable by the oom reaper.
 	 */
-	rcu_read_lock();
-	for_each_process(p) {
-		if (!process_shares_mm(p, mm))
-			continue;
-		if (same_thread_group(task, p))
-			continue;
-		ret = __task_will_free_mem(p);
-		if (!ret)
-			break;
+	scoped_guard(rcu) {
+		for_each_process(p) {
+			if (!process_shares_mm(p, mm))
+				continue;
+			if (same_thread_group(task, p))
+				continue;
+			ret = __task_will_free_mem(p);
+			if (!ret)
+				break;
+		}
 	}
-	rcu_read_unlock();
 
 	return ret;
 }
@@ -960,29 +960,29 @@ static void __oom_kill_process(struct task_struct *victim, const char *message)
 	 * That thread will now get access to memory reserves since it has a
 	 * pending fatal signal.
 	 */
-	rcu_read_lock();
-	for_each_process(p) {
-		if (!process_shares_mm(p, mm))
-			continue;
-		if (same_thread_group(p, victim))
-			continue;
-		if (is_global_init(p)) {
-			can_oom_reap = false;
-			mm_flags_set(MMF_OOM_SKIP, mm);
-			pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
-					task_pid_nr(victim), victim->comm,
-					task_pid_nr(p), p->comm);
-			continue;
+	scoped_guard(rcu) {
+		for_each_process(p) {
+			if (!process_shares_mm(p, mm))
+				continue;
+			if (same_thread_group(p, victim))
+				continue;
+			if (is_global_init(p)) {
+				can_oom_reap = false;
+				mm_flags_set(MMF_OOM_SKIP, mm);
+				pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
+						task_pid_nr(victim), victim->comm,
+						task_pid_nr(p), p->comm);
+				continue;
+			}
+			/*
+			 * No kthread_use_mm() user needs to read from the userspace so
+			 * we are ok to reap it.
+			 */
+			if (unlikely(p->flags & PF_KTHREAD))
+				continue;
+			do_send_sig_info(SIGKILL, SEND_SIG_PRIV, p, PIDTYPE_TGID);
 		}
-		/*
-		 * No kthread_use_mm() user needs to read from the userspace so
-		 * we are ok to reap it.
-		 */
-		if (unlikely(p->flags & PF_KTHREAD))
-			continue;
-		do_send_sig_info(SIGKILL, SEND_SIG_PRIV, p, PIDTYPE_TGID);
 	}
-	rcu_read_unlock();
 
 	if (can_oom_reap)
 		queue_oom_reaper(victim);
-- 
2.25.1

Re: [PATCH v2] mm/oom_kill: simplify remaining RCU sections with scoped_guard(rcu)

[Michal Hocko]

On Thu 13-08-26 17:29:32, Ye Liu wrote:
> From: Ye Liu <liuye@kylinos.cn>
> 
> Replace the remaining manual rcu_read_lock()/rcu_read_unlock() pairs
> in oom_cpuset_eligible(), select_bad_process(), dump_tasks(),
> task_will_free_mem(), and __oom_kill_process() with scoped_guard(rcu)
> for consistency and simpler control flow.  scoped_guard(rcu) limits
> the RCU critical section to the loop body rather than the entire
> remaining function scope, making the protected region explicit and
> safer for future changes.
> 
> Signed-off-by: Ye Liu <liuye@kylinos.cn>

It almost looks like for_each_thread and for_each_process could gain an
rcu varian that would do the rcu thing internally. Have you considered
that? I am pretty sure there will be more cases like these.

Anyway
Acked-by: Michal Hocko <mhocko@suse.com>
Thanks

> ---
> v2:
>  - Use scoped_guard instead of guard.
>  - Link：https://lore.kernel.org/all/20260813032634.344946-1-ye.liu@linux.dev/
> 
>  mm/oom_kill.c | 124 +++++++++++++++++++++++++-------------------------
>  1 file changed, 62 insertions(+), 62 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 5f372f6e26fa..0e8982e5be51 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -94,27 +94,27 @@ static bool oom_cpuset_eligible(struct task_struct *start,
>  	bool ret = false;
>  	const nodemask_t *mask = oc->nodemask;
>  
> -	rcu_read_lock();
> -	for_each_thread(start, tsk) {
> -		if (mask) {
> -			/*
> -			 * If this is a mempolicy constrained oom, tsk's
> -			 * cpuset is irrelevant.  Only return true if its
> -			 * mempolicy intersects current, otherwise it may be
> -			 * needlessly killed.
> -			 */
> -			ret = mempolicy_in_oom_domain(tsk, mask);
> -		} else {
> -			/*
> -			 * This is not a mempolicy constrained oom, so only
> -			 * check the mems of tsk's cpuset.
> -			 */
> -			ret = cpuset_mems_allowed_intersects(current, tsk);
> +	scoped_guard(rcu) {
> +		for_each_thread(start, tsk) {
> +			if (mask) {
> +				/*
> +				 * If this is a mempolicy constrained oom, tsk's
> +				 * cpuset is irrelevant.  Only return true if its
> +				 * mempolicy intersects current, otherwise it may be
> +				 * needlessly killed.
> +				 */
> +				ret = mempolicy_in_oom_domain(tsk, mask);
> +			} else {
> +				/*
> +				 * This is not a mempolicy constrained oom, so only
> +				 * check the mems of tsk's cpuset.
> +				 */
> +				ret = cpuset_mems_allowed_intersects(current, tsk);
> +			}
> +			if (ret)
> +				break;
>  		}
> -		if (ret)
> -			break;
>  	}
> -	rcu_read_unlock();
>  
>  	return ret;
>  }
> @@ -368,11 +368,11 @@ static void select_bad_process(struct oom_control *oc)
>  	else {
>  		struct task_struct *p;
>  
> -		rcu_read_lock();
> -		for_each_process(p)
> -			if (oom_evaluate_task(p, oc))
> -				break;
> -		rcu_read_unlock();
> +		scoped_guard(rcu) {
> +			for_each_process(p)
> +				if (oom_evaluate_task(p, oc))
> +					break;
> +		}
>  	}
>  }
>  
> @@ -430,14 +430,14 @@ static void dump_tasks(struct oom_control *oc)
>  		struct task_struct *p;
>  		int i = 0;
>  
> -		rcu_read_lock();
> -		for_each_process(p) {
> -			/* Avoid potential softlockup warning */
> -			if ((++i & 1023) == 0)
> -				touch_softlockup_watchdog();
> -			dump_task(p, oc);
> +		scoped_guard(rcu) {
> +			for_each_process(p) {
> +				/* Avoid potential softlockup warning */
> +				if ((++i & 1023) == 0)
> +					touch_softlockup_watchdog();
> +				dump_task(p, oc);
> +			}
>  		}
> -		rcu_read_unlock();
>  	}
>  }
>  
> @@ -894,17 +894,17 @@ static bool task_will_free_mem(struct task_struct *task)
>  	 * are dying as well to make sure that a) nobody pins its mm and
>  	 * b) the task is also reapable by the oom reaper.
>  	 */
> -	rcu_read_lock();
> -	for_each_process(p) {
> -		if (!process_shares_mm(p, mm))
> -			continue;
> -		if (same_thread_group(task, p))
> -			continue;
> -		ret = __task_will_free_mem(p);
> -		if (!ret)
> -			break;
> +	scoped_guard(rcu) {
> +		for_each_process(p) {
> +			if (!process_shares_mm(p, mm))
> +				continue;
> +			if (same_thread_group(task, p))
> +				continue;
> +			ret = __task_will_free_mem(p);
> +			if (!ret)
> +				break;
> +		}
>  	}
> -	rcu_read_unlock();
>  
>  	return ret;
>  }
> @@ -960,29 +960,29 @@ static void __oom_kill_process(struct task_struct *victim, const char *message)
>  	 * That thread will now get access to memory reserves since it has a
>  	 * pending fatal signal.
>  	 */
> -	rcu_read_lock();
> -	for_each_process(p) {
> -		if (!process_shares_mm(p, mm))
> -			continue;
> -		if (same_thread_group(p, victim))
> -			continue;
> -		if (is_global_init(p)) {
> -			can_oom_reap = false;
> -			mm_flags_set(MMF_OOM_SKIP, mm);
> -			pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
> -					task_pid_nr(victim), victim->comm,
> -					task_pid_nr(p), p->comm);
> -			continue;
> +	scoped_guard(rcu) {
> +		for_each_process(p) {
> +			if (!process_shares_mm(p, mm))
> +				continue;
> +			if (same_thread_group(p, victim))
> +				continue;
> +			if (is_global_init(p)) {
> +				can_oom_reap = false;
> +				mm_flags_set(MMF_OOM_SKIP, mm);
> +				pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
> +						task_pid_nr(victim), victim->comm,
> +						task_pid_nr(p), p->comm);
> +				continue;
> +			}
> +			/*
> +			 * No kthread_use_mm() user needs to read from the userspace so
> +			 * we are ok to reap it.
> +			 */
> +			if (unlikely(p->flags & PF_KTHREAD))
> +				continue;
> +			do_send_sig_info(SIGKILL, SEND_SIG_PRIV, p, PIDTYPE_TGID);
>  		}
> -		/*
> -		 * No kthread_use_mm() user needs to read from the userspace so
> -		 * we are ok to reap it.
> -		 */
> -		if (unlikely(p->flags & PF_KTHREAD))
> -			continue;
> -		do_send_sig_info(SIGKILL, SEND_SIG_PRIV, p, PIDTYPE_TGID);
>  	}
> -	rcu_read_unlock();
>  
>  	if (can_oom_reap)
>  		queue_oom_reaper(victim);
> -- 
> 2.25.1

-- 
Michal Hocko
SUSE Labs