Re: [PATCH 6/8] fs: convert process/thread iterators to for_each_*_rcu

[Lorenzo Stoakes (ARM)]

On Tue, Sep 08, 2026 at 08:26:54PM +0300, Alexey Dobriyan wrote:
> On Tue, Sep 08, 2026 at 06:10:17PM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Tue, Sep 08, 2026 at 07:45:15PM +0300, Alexey Dobriyan wrote:
> > > On Fri, Sep 04, 2026 at 04:29:58PM +0800, Ye Liu wrote:
> > > > --- a/fs/proc/base.c
> > > > +++ b/fs/proc/base.c
> > > > @@ -1160,8 +1160,7 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > > >  	if (mm) {
> > > >  		struct task_struct *p;
> > > >
> > > > -		rcu_read_lock();
> > > > -		for_each_process(p) {
> > > > +		for_each_process_rcu(p) {
> > > >  			if (same_thread_group(task, p))
> > > >  				continue;
> > > >
> > > > @@ -1177,7 +1176,6 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
> > > >  			}
> > > >  			task_unlock(p);
> > > >  		}
> > > > -		rcu_read_unlock();
> > > >  		mmdrop(mm);
> > > >  	}
> > > >  err_unlock:
> > >
> > > What's going on here?
> > > Now it's RCU unlock after mmdrop().
> >
> > No it's not, scoped_guard() is scoped to the for_each_process_rcu() block which
> > ends before mmdrop(), i.e. the exact same scope as before.
> >
> > That closing '}' is attached to the if (mm).
>
> OK.
>
> I think naming (and macro) are misguided:
> * _rcu means "use under RCU section opened elsewhere",
>   this one is "I open and close RCU section myself".

You're replying to a v1 of a series that has a v2 which changes the naming.

>
> * not every 2 lines should be combined into new interface.

Nobody is suggesting every 2 lines should be combined into a new interface.

They're suggesting that explicitly tying lock lifetime to block scope is a
really good idea.

And having debugged lots of locking issues, I agree.

Anyway the v2 is at:

https://lore.kernel.org/all/20260907081334.1152889-1-ye.liu@xxxxxxxxx/


--
Cheers, Lorenzo