[PATCH] mm/damon/vaddr: remove redundant RCU lock

[PATCH] mm/damon/vaddr: remove redundant RCU lock

[IgorpetinDev]

__damon_va_three_regions() is called only by damon_va_three_regions(),
which already holds mmap_read_lock(). Since mmap_read_lock() is held,
the maple tree and VMA list are protected from concurrent changes.
Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.

Signed-off-by: IgorpetinDev <igorpetindev@gmail.com>
---
 mm/damon/vaddr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index d27147603..345b89b5d 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -84,7 +84,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 	 * If this is too slow, it can be optimised to examine the maple
 	 * tree gaps.
 	 */
-	rcu_read_lock();
 	for_each_vma(vmi, vma) {
 		unsigned long gap;
 
@@ -105,7 +104,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 next:
 		prev = vma;
 	}
-	rcu_read_unlock();
 
 	if (!sz_range(&second_gap) || !sz_range(&first_gap))
 		return -EINVAL;
-- 
2.47.3

Re: [PATCH] mm/damon/vaddr: remove redundant RCU lock

[SJ Park]

On Mon, 29 Jun 2026 19:53:49 +0300 IgorpetinDev <igorpetindev@gmail.com> wrote:

> __damon_va_three_regions() is called only by damon_va_three_regions(),
> which already holds mmap_read_lock(). Since mmap_read_lock() is held,
> the maple tree and VMA list are protected from concurrent changes.
> Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.
> 
> Signed-off-by: IgorpetinDev <igorpetindev@gmail.com>

We prefer using a real name.  I show you used 'Igor Putko' as your name before.
Any reason to change the name?

> ---
>  mm/damon/vaddr.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index d27147603..345b89b5d 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -84,7 +84,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
>  	 * If this is too slow, it can be optimised to examine the maple
>  	 * tree gaps.
>  	 */
> -	rcu_read_lock();
>  	for_each_vma(vmi, vma) {
>  		unsigned long gap;
>  
> @@ -105,7 +104,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
>  next:
>  		prev = vma;
>  	}
> -	rcu_read_unlock();

As Sashiko also commented, this may cause lockdep warning.  Actually this was
added to fix it.  Refer to the previous discussions [1] for more details.

How about moving rcu_read_[un]lock() to the caller test code?

[1] https://lore.kernel.org/20240904004534.1189-1-sj@kernel.org


Thanks,
SJ

[...]

Re: [PATCH] mm/damon/vaddr: remove redundant RCU lock

[IgorpetinDev]

[-- Attachment #1: Type: text/plain, Size: 2702 bytes --]

Hi SJ,

On Mon, 29 Jun 2026, SJ Park wrote:
> We prefer using a real name. I show you used 'Igor Putko' as your name
before.
> Any reason to change the name?

Apologies for the confusion. I recently reinstalled my OS and misconfigured
my git setup. I will revert back to 'Igor Putko' for the v2 submission.

> As Sashiko also commented, this may cause lockdep warning. Actually this
was
> added to fix it. Refer to the previous discussions [1] for more details.
>
> How about moving rcu_read_[un]lock() to the caller test code?

That makes total sense. I will move the rcu_read_lock() and
rcu_read_unlock()
calls directly into the KUnit test code in mm/damon/tests/vaddr-kunit.h to
prevent the lockdep splat.

Regarding the pre-existing high-severity issue reported by Sashiko AI about
missing check_stable_address_space(), I believe it is out of scope for this
particular cleanup and should be addressed in a separate, dedicated patch.

I will send the v2 shortly.

Thanks,
Igor

On Tue, Jun 30, 2026 at 4:00 AM SJ Park <sj@kernel.org> wrote:

> On Mon, 29 Jun 2026 19:53:49 +0300 IgorpetinDev <igorpetindev@gmail.com>
> wrote:
>
> > __damon_va_three_regions() is called only by damon_va_three_regions(),
> > which already holds mmap_read_lock(). Since mmap_read_lock() is held,
> > the maple tree and VMA list are protected from concurrent changes.
> > Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.
> >
> > Signed-off-by: IgorpetinDev <igorpetindev@gmail.com>
>
> We prefer using a real name.  I show you used 'Igor Putko' as your name
> before.
> Any reason to change the name?
>
> > ---
> >  mm/damon/vaddr.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index d27147603..345b89b5d 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> > @@ -84,7 +84,6 @@ static int __damon_va_three_regions(struct mm_struct
> *mm,
> >        * If this is too slow, it can be optimised to examine the maple
> >        * tree gaps.
> >        */
> > -     rcu_read_lock();
> >       for_each_vma(vmi, vma) {
> >               unsigned long gap;
> >
> > @@ -105,7 +104,6 @@ static int __damon_va_three_regions(struct mm_struct
> *mm,
> >  next:
> >               prev = vma;
> >       }
> > -     rcu_read_unlock();
>
> As Sashiko also commented, this may cause lockdep warning.  Actually this
> was
> added to fix it.  Refer to the previous discussions [1] for more details.
>
> How about moving rcu_read_[un]lock() to the caller test code?
>
> [1] https://lore.kernel.org/20240904004534.1189-1-sj@kernel.org
>
>
> Thanks,
> SJ
>
> [...]
>

[-- Attachment #2: Type: text/html, Size: 3539 bytes --]

[PATCH] mm/damon/vaddr: remove redundant RCU lock

[Igor Putko]

From: IgorpetinDev <igorpetindev@gmail.com>

__damon_va_three_regions() is called only by damon_va_three_regions(),
which already holds mmap_read_lock(). Since mmap_read_lock() is held,
the maple tree and VMA list are protected from concurrent changes.
Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.

Signed-off-by: IgorpetinDev <igorpetindev@gmail.com>
---
 mm/damon/vaddr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index d27147603..345b89b5d 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -84,7 +84,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 	 * If this is too slow, it can be optimised to examine the maple
 	 * tree gaps.
 	 */
-	rcu_read_lock();
 	for_each_vma(vmi, vma) {
 		unsigned long gap;
 
@@ -105,7 +104,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 next:
 		prev = vma;
 	}
-	rcu_read_unlock();
 
 	if (!sz_range(&second_gap) || !sz_range(&first_gap))
 		return -EINVAL;
-- 
2.47.3

[PATCH v2] mm/damon/vaddr: remove redundant RCU lock

[Igor Putko]

__damon_va_three_regions() is called only by damon_va_three_regions(),
which already holds mmap_read_lock(). Since mmap_read_lock() is held,
the maple tree and VMA list are protected from concurrent changes.
Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.

Signed-off-by: Igor Putko <igorpetindev@gmail.com>
---
 mm/damon/tests/vaddr-kunit.h | 2 ++
 mm/damon/vaddr.c             | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/damon/tests/vaddr-kunit.h b/mm/damon/tests/vaddr-kunit.h
index 563fbc7e3..70cb7825e 100644
--- a/mm/damon/tests/vaddr-kunit.h
+++ b/mm/damon/tests/vaddr-kunit.h
@@ -81,7 +81,9 @@ static void damon_test_three_regions_in_vmas(struct kunit *test)
 	if (__link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas)))
 		kunit_skip(test, "Failed to create VMA tree");
 
+	rcu_read_lock();
 	__damon_va_three_regions(&mm, regions);
+	rcu_read_unlock();
 
 	KUNIT_EXPECT_EQ(test, 10ul, regions[0].start);
 	KUNIT_EXPECT_EQ(test, 25ul, regions[0].end);
diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index d27147603..345b89b5d 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -84,7 +84,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 	 * If this is too slow, it can be optimised to examine the maple
 	 * tree gaps.
 	 */
-	rcu_read_lock();
 	for_each_vma(vmi, vma) {
 		unsigned long gap;
 
@@ -105,7 +104,6 @@ static int __damon_va_three_regions(struct mm_struct *mm,
 next:
 		prev = vma;
 	}
-	rcu_read_unlock();
 
 	if (!sz_range(&second_gap) || !sz_range(&first_gap))
 		return -EINVAL;
-- 
2.47.3

Re: [PATCH] mm/damon/vaddr: remove redundant RCU lock

[Igor Putko]

Hi SJ,

On Mon, 29 Jun 2026, SJ Park wrote:
> We prefer using a real name. I show you used 'Igor Putko' as your name before.
> Any reason to change the name?

Apologies for the confusion. I recently reinstalled my OS and misconfigured
my git setup. I will revert back to 'Igor Putko' for the v2 submission.

> As Sashiko also commented, this may cause lockdep warning. Actually this was
> added to fix it. Refer to the previous discussions [1] for more details.
> 
> How about moving rcu_read_[un]lock() to the caller test code?

That makes total sense. I will move the rcu_read_lock() and rcu_read_unlock()
calls directly into the KUnit test code in mm/damon/tests/vaddr-kunit.h to
prevent the lockdep splat.

Regarding the pre-existing high-severity issue reported by Sashiko AI about
missing check_stable_address_space(), I believe it is out of scope for this
particular cleanup and should be addressed in a separate, dedicated patch.

I have just sent the v2 patch out.

Thanks,
Igor

Re: [PATCH] mm/damon/vaddr: remove redundant RCU lock

[SJ Park]

On Tue, 30 Jun 2026 16:07:43 +0300 Igor Putko <igorpetindev@gmail.com> wrote:

> Hi SJ,
> 
> On Mon, 29 Jun 2026, SJ Park wrote:
> > We prefer using a real name. I show you used 'Igor Putko' as your name before.
> > Any reason to change the name?
> 
> Apologies for the confusion. I recently reinstalled my OS and misconfigured
> my git setup. I will revert back to 'Igor Putko' for the v2 submission.

No worries!  I'm glad that I helped you finding a misconfiguration!

> 
> > As Sashiko also commented, this may cause lockdep warning. Actually this was
> > added to fix it. Refer to the previous discussions [1] for more details.
> > 
> > How about moving rcu_read_[un]lock() to the caller test code?
> 
> That makes total sense. I will move the rcu_read_lock() and rcu_read_unlock()
> calls directly into the KUnit test code in mm/damon/tests/vaddr-kunit.h to
> prevent the lockdep splat.
> 
> Regarding the pre-existing high-severity issue reported by Sashiko AI about
> missing check_stable_address_space(), I believe it is out of scope for this
> particular cleanup and should be addressed in a separate, dedicated patch.

I was thinking it is not a real issue.  Do you think it is a real?

> 
> I have just sent the v2 patch out.

From next time, please give enough time before sending a new revision (say, ~12
hours at least?), to ensure the discussion on the previous revision is
concluded.


Thanks,
SJ

[...]

Re: [PATCH v2] mm/damon/vaddr: remove redundant RCU lock

[SJ Park]

Hello Igor,


Thank you for this patch.

From next time, please give enough time for others to comment before posting a
new revision of the patch.

Also, please post a new revision as a new thread with a changelog [1] including
links to previous revisions, rather than a reply to the previous revision.

On Tue, 30 Jun 2026 16:02:53 +0300 Igor Putko <igorpetindev@gmail.com> wrote:

> __damon_va_three_regions() is called only by damon_va_three_regions(),
> which already holds mmap_read_lock(). Since mmap_read_lock() is held,
> the maple tree and VMA list are protected from concurrent changes.
> Remove the unnecessary rcu_read_lock() and rcu_read_unlock() calls.

This is not explaining the test part change.  Could you please update for that?

Also, have you had a chance to theoretically or experiemently confirm the
problem [2] that motivated to add the rcu_read_lock() at that place is not
reintroduced by this change?  If so, could you please add that to the commit
log, too?


Thanks,
SJ

[1] https://docs.kernel.org/process/submitting-patches.html#commentary
[2] https://lore.kernel.org/b83651a0-5b24-4206-b860-cb54ffdf209b@roeck-us.net

[...]

Re: [PATCH] mm/damon/vaddr: remove redundant RCU lock

[Igor Putko]

Hi SJ,

Thanks for the reply and the pointers!
Regarding the issue Sashiko AI flagged - I dug into damon/vaddr.c, and I think it's a real bug, but it's isolated to __damon_va_three_regions().
damon_va_walk_page_range() is fine here, since lock_vma_under_rcu() handles stability on its own. __damon_va_three_regions() is the problematic one: right after mmap_read_lock(mm) is taken in the caller, it sets up a VMA_ITERATOR and runs for_each_vma(). But mmap_read_lock doesn't stop the OOM reaper from running concurrently, setting MMF_UNSTABLE, and leaving empty markers in the maple tree. Walking the tree in that state without a check_stable_address_space(mm) call could lead to a NULL pointer dereference.
Since this looks like a separate issue, I'll prepare a dedicated fix and send it as its own patch thread, so it doesn't hold up the current cleanup. Let me know if that approach works for you.

Thanks,
Igor

Re: [PATCH] mm/damon/vaddr: remove redundant RCU lock

[SJ Park]

On Tue, 30 Jun 2026 17:25:22 +0300 Igor Putko <igorpetindev@gmail.com> wrote:

> Hi SJ,
> 
> Thanks for the reply and the pointers!

Thank you for quick reply.  Nonetheless, from the next time, please reply to a
mail you are replying to, without top-posting [1].  Also, please wrap long
lines for reasonable length of columns (say, 80 columns?)

> Regarding the issue Sashiko AI flagged - I dug into damon/vaddr.c, and I think it's a real bug, but it's isolated to __damon_va_three_regions().
> damon_va_walk_page_range() is fine here, since lock_vma_under_rcu() handles stability on its own. __damon_va_three_regions() is the problematic one: right after mmap_read_lock(mm) is taken in the caller, it sets up a VMA_ITERATOR and runs for_each_vma(). But mmap_read_lock doesn't stop the OOM reaper from running concurrently, setting MMF_UNSTABLE, and leaving empty markers in the maple tree. Walking the tree in that state without a check_stable_address_space(mm) call could lead to a NULL pointer dereference.

Thank you for detailed explanation.

> Since this looks like a separate issue, I'll prepare a dedicated fix and send it as its own patch thread, so it doesn't hold up the current cleanup. Let me know if that approach works for you.

Makes sense!

[1] https://subspace.kernel.org/etiquette.html


Thanks,
SJ

[...]