* [PATCH] mm/damon: fix unconditionally skip last region
@ 2026-08-28 8:47 Liew Rui Yan
2026-08-28 18:29 ` SJ Park
0 siblings, 1 reply; 6+ messages in thread
From: Liew Rui Yan @ 2026-08-28 8:47 UTC (permalink / raw)
To: SJ Park
Cc: Andrew Morton, damon, linux-mm, linux-kernel, Liew Rui Yan, stable
Once quota set, the charge_{target,addr}_from unconditionally skips and
resets at the last region of the tracked target, so the last region can
be skipped even when it has not been processed.
Example:
1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
2. Quota is configured to process only 50 bytes per window.
3. Window 1: Processes R1 (0-50). Quota is full. Cursor is saved
at (Target, 50).
4. Window 2: Skips R1 (0-50). Processes R1 (50-100). Quota is
full. Cursor is saved at (Target, 100), which is exactly the
start of R2.
5. Window 3: The loop reaches R2. Because R2 is
damon_last_region(t), the old code unconditionally returns true,
skipping R2 entirely and resetting the cursor.
Result: R2 is permanently skipped even though it has never been
processed.
Fix this by only skipping the last region after it has been applied.
Fixes: 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
Cc: <stable@vger.kernel.org> # v5.16.x
Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
---
Changes from RFC v1:
- Minimal fix, only fixes the issue where the last-region is skipped.
- Add an example to the commit message to demonstrate that this error
occurs very rarely.
- RFC v1: https://lore.kernel.org/damon/20260825124616.5129-1-aethernet65535@gmail.com
---
mm/damon/core.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 644daf5a1656..21dc6b086c42 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2347,14 +2347,15 @@ static bool damos_skip_charged_region(struct damon_target *t,
if (quota->charge_target_from) {
if (t != quota->charge_target_from)
return true;
- if (r == damon_last_region(t)) {
- quota->charge_target_from = NULL;
- quota->charge_addr_from = 0;
- return true;
- }
if (quota->charge_addr_from &&
- r->ar.end <= quota->charge_addr_from)
+ r->ar.end <= quota->charge_addr_from) {
+ if (r->ar.end == quota->charge_addr_from ||
+ damon_is_last_region(r, t)) {
+ quota->charge_target_from = NULL;
+ quota->charge_addr_from = 0;
+ }
return true;
+ }
if (quota->charge_addr_from && r->ar.start <
quota->charge_addr_from) {
--
2.55.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/damon: fix unconditionally skip last region
2026-08-28 8:47 [PATCH] mm/damon: fix unconditionally skip last region Liew Rui Yan
@ 2026-08-28 18:29 ` SJ Park
2026-08-29 8:34 ` Liew Rui Yan
0 siblings, 1 reply; 6+ messages in thread
From: SJ Park @ 2026-08-28 18:29 UTC (permalink / raw)
To: Liew Rui Yan
Cc: SJ Park, Andrew Morton, damon, linux-mm, linux-kernel, stable
On Fri, 28 Aug 2026 16:47:37 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> Once quota set, the charge_{target,addr}_from unconditionally skips and
> resets at the last region of the tracked target, so the last region can
> be skipped even when it has not been processed.
>
> Example:
>
> 1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
> 2. Quota is configured to process only 50 bytes per window.
> 3. Window 1: Processes R1 (0-50). Quota is full. Cursor is saved
> at (Target, 50).
Cursor means charge_{target,addr}_from, right? Let's explain that, or just
keep using the terms (charge_{target,addr}_from).
> 4. Window 2: Skips R1 (0-50). Processes R1 (50-100). Quota is
> full. Cursor is saved at (Target, 100), which is exactly the
> start of R2.
> 5. Window 3: The loop reaches R2. Because R2 is
> damon_last_region(t), the old code unconditionally returns true,
> skipping R2 entirely and resetting the cursor.
>
> Result: R2 is permanently skipped even though it has never been
> processed.
Let's make example simpler by setting R1 (0-50 bytes) and R2 (50-100 bytes) or
quota size 100 bytes per window.
Also, it continues being skipped only in a corner case that the region
addresses and the access patterns are kept. So the user impact is mild. Let's
clarify that to not make users unnecessarily afraid.
>
> Fix this by only skipping the last region after it has been applied.
>
> Fixes: 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
> Cc: <stable@vger.kernel.org> # v5.16.x
> Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> ---
>
> Changes from RFC v1:
> - Minimal fix, only fixes the issue where the last-region is skipped.
> - Add an example to the commit message to demonstrate that this error
> occurs very rarely.
> - RFC v1: https://lore.kernel.org/damon/20260825124616.5129-1-aethernet65535@gmail.com
>
> ---
> mm/damon/core.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 644daf5a1656..21dc6b086c42 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2347,14 +2347,15 @@ static bool damos_skip_charged_region(struct damon_target *t,
> if (quota->charge_target_from) {
> if (t != quota->charge_target_from)
> return true;
> - if (r == damon_last_region(t)) {
> - quota->charge_target_from = NULL;
> - quota->charge_addr_from = 0;
> - return true;
> - }
> if (quota->charge_addr_from &&
> - r->ar.end <= quota->charge_addr_from)
> + r->ar.end <= quota->charge_addr_from) {
> + if (r->ar.end == quota->charge_addr_from ||
> + damon_is_last_region(r, t)) {
> + quota->charge_target_from = NULL;
> + quota->charge_addr_from = 0;
> + }
> return true;
> + }
>
> if (quota->charge_addr_from && r->ar.start <
> quota->charge_addr_from) {
As Sashiko pointed out, this doesn't work if the the last region's start
address is smaller than charge_addr_from and the end address is larger than
charge_addr_from, but the size to skip (charge_addr_from - r->ar.start) is
smaller than min_region_sz.
As you replied to Sashiko, let's do the last region handling in every case.
While doing that, let's do the charge_{target,addr}_from reset in only one
place, like below.
'''
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2688,36 +2688,40 @@ static bool damos_skip_charged_region(struct damon_target *t,
{
struct damos_quota *quota = &s->quota;
unsigned long sz_to_skip;
+ bool skip = false;
/* Skip previously charged regions */
if (quota->charge_target_from) {
if (t != quota->charge_target_from)
return true;
- if (r == damon_last_region(t)) {
- quota->charge_target_from = NULL;
- quota->charge_addr_from = 0;
- return true;
- }
if (quota->charge_addr_from &&
- r->ar.end <= quota->charge_addr_from)
- return true;
+ r->ar.end <= quota->charge_addr_from) {
+ skip = true;
+ goto out;
+ }
if (quota->charge_addr_from && r->ar.start <
quota->charge_addr_from) {
sz_to_skip = ALIGN_DOWN(quota->charge_addr_from -
r->ar.start, min_region_sz);
if (!sz_to_skip) {
- if (damon_sz_region(r) <= min_region_sz)
- return true;
+ if (damon_sz_region(r) <= min_region_sz) {
+ skip = true;
+ goto out;
+ }
sz_to_skip = min_region_sz;
}
damon_split_region_at(t, r, sz_to_skip);
- return true;
+ skip = true;
}
+ }
+out:
+ if (r == damon_last_region(t)) {
quota->charge_target_from = NULL;
quota->charge_addr_from = 0;
+ return true;
}
- return false;
+ return skip;
}
static void damos_update_stat(struct damos *s,
'''
Btw, I think damos_skip_charged_region() may deserve a kunit test.
[1] https://lore.kernel.org/20260828090410.40AEA1F000E9@smtp.kernel.org
[2] https://lore.kernel.org/20260828115047.332978-1-aethernet65535@gmail.com
Thanks,
SJ
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/damon: fix unconditionally skip last region
2026-08-28 18:29 ` SJ Park
@ 2026-08-29 8:34 ` Liew Rui Yan
2026-08-29 16:15 ` SJ Park
0 siblings, 1 reply; 6+ messages in thread
From: Liew Rui Yan @ 2026-08-29 8:34 UTC (permalink / raw)
To: sj; +Cc: aethernet65535, akpm, damon, linux-kernel, linux-mm, stable
On Fri, 28 Aug 2026 11:29:09 -0700 SJ Park <sj@kernel.org> wrote:
> On Fri, 28 Aug 2026 16:47:37 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
>
> > Once quota set, the charge_{target,addr}_from unconditionally skips and
> > resets at the last region of the tracked target, so the last region can
> > be skipped even when it has not been processed.
> >
> > Example:
> >
> > 1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
> > 2. Quota is configured to process only 50 bytes per window.
> > 3. Window 1: Processes R1 (0-50). Quota is full. Cursor is saved
> > at (Target, 50).
>
> Cursor means charge_{target,addr}_from, right? Let's explain that, or just
> keep using the terms (charge_{target,addr}_from).
Yes, thank you for pointing that out! I changed cursor to
charge_{target,addr}_from now.
>
> > 4. Window 2: Skips R1 (0-50). Processes R1 (50-100). Quota is
> > full. Cursor is saved at (Target, 100), which is exactly the
> > start of R2.
> > 5. Window 3: The loop reaches R2. Because R2 is
> > damon_last_region(t), the old code unconditionally returns true,
> > skipping R2 entirely and resetting the cursor.
> >
> > Result: R2 is permanently skipped even though it has never been
> > processed.
>
> Let's make example simpler by setting R1 (0-50 bytes) and R2 (50-100 bytes) or
> quota size 100 bytes per window.
This is the updated example:
'''
Example:
1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
2. Quota is configured to process only 100 bytes per window.
3. Window 1: Processes R1 (0-100). Quota is full. charge_{target,
addr}_from is saved at (Target, 100).
4. Window 2: The loop reaches R2. Because R2 is
damon_last_region(t), the old code unconditionally returns true,
skipping R2 entirely and resetting the charge_{target,addr}_from.
Result: R2 is permanently skipped even though it has never been
processed.
'''
>
> Also, it continues being skipped only in a corner case that the region
> addresses and the access patterns are kept. So the user impact is mild. Let's
> clarify that to not make users unnecessarily afraid.
I will add this clarification in the next revision:
'''
However, it is important to note that this is a very minor issue. This
is because it is triggered only when the previous window saved/kept
charge_{target,addr}_from, and in the next window, all regions except
the last region were skipped by damos_skip_charged_region().
'''
>
> >
> > Fix this by only skipping the last region after it has been applied.
> >
> > Fixes: 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
> > Cc: <stable@vger.kernel.org> # v5.16.x
> > Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> > ---
> >
> > Changes from RFC v1:
> > - Minimal fix, only fixes the issue where the last-region is skipped.
> > - Add an example to the commit message to demonstrate that this error
> > occurs very rarely.
> > - RFC v1: https://lore.kernel.org/damon/20260825124616.5129-1-aethernet65535@gmail.com
> >
> > ---
> > mm/damon/core.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 644daf5a1656..21dc6b086c42 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2347,14 +2347,15 @@ static bool damos_skip_charged_region(struct damon_target *t,
> > if (quota->charge_target_from) {
> > if (t != quota->charge_target_from)
> > return true;
> > - if (r == damon_last_region(t)) {
> > - quota->charge_target_from = NULL;
> > - quota->charge_addr_from = 0;
> > - return true;
> > - }
> > if (quota->charge_addr_from &&
> > - r->ar.end <= quota->charge_addr_from)
> > + r->ar.end <= quota->charge_addr_from) {
> > + if (r->ar.end == quota->charge_addr_from ||
> > + damon_is_last_region(r, t)) {
> > + quota->charge_target_from = NULL;
> > + quota->charge_addr_from = 0;
> > + }
> > return true;
> > + }
> >
> > if (quota->charge_addr_from && r->ar.start <
> > quota->charge_addr_from) {
>
> As Sashiko pointed out, this doesn't work if the the last region's start
> address is smaller than charge_addr_from and the end address is larger than
> charge_addr_from, but the size to skip (charge_addr_from - r->ar.start) is
> smaller than min_region_sz.
>
> As you replied to Sashiko, let's do the last region handling in every case.
> While doing that, let's do the charge_{target,addr}_from reset in only one
> place, like below.
>
> '''
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2688,36 +2688,40 @@ static bool damos_skip_charged_region(struct damon_target *t,
> {
> struct damos_quota *quota = &s->quota;
> unsigned long sz_to_skip;
> + bool skip = false;
>
> /* Skip previously charged regions */
> if (quota->charge_target_from) {
> if (t != quota->charge_target_from)
> return true;
> - if (r == damon_last_region(t)) {
> - quota->charge_target_from = NULL;
> - quota->charge_addr_from = 0;
> - return true;
> - }
> if (quota->charge_addr_from &&
> - r->ar.end <= quota->charge_addr_from)
> - return true;
> + r->ar.end <= quota->charge_addr_from) {
> + skip = true;
> + goto out;
> + }
>
> if (quota->charge_addr_from && r->ar.start <
> quota->charge_addr_from) {
> sz_to_skip = ALIGN_DOWN(quota->charge_addr_from -
> r->ar.start, min_region_sz);
> if (!sz_to_skip) {
> - if (damon_sz_region(r) <= min_region_sz)
> - return true;
> + if (damon_sz_region(r) <= min_region_sz) {
> + skip = true;
> + goto out;
> + }
> sz_to_skip = min_region_sz;
> }
> damon_split_region_at(t, r, sz_to_skip);
> - return true;
> + skip = true;
> }
> + }
> +out:
> + if (r == damon_last_region(t)) {
> quota->charge_target_from = NULL;
> quota->charge_addr_from = 0;
> + return true;
> }
> - return false;
> + return skip;
> }
>
> static void damos_update_stat(struct damos *s,
> '''
I noticed a potential subtle issue in the suggested fix above:
'''
+out:
+ if (r == damon_last_region(t)) {
quota->charge_target_from = NULL;
quota->charge_addr_from = 0;
+ return true;
}
'''
If 'skip' is false (region should be processed), but it happens to be
the last region, the condition 'if (r == damon_last_region(t))' would
still be met. This would cause it to reset the state and 'return true'
(skip it), which inadvertently re-introduces the original bug we are
trying to fix.
To ensure the reset logic is centralized and correct, I refined the fix
as follows. The comment is intended to help you and other reviewers
quickly understand the rationale behind the compound condition. I will
remove this comment in the next revision.
'''
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 644daf5a1656..82c5aed8a417 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2342,36 +2342,48 @@ static bool damos_skip_charged_region(struct damon_target *t,
{
struct damos_quota *quota = &s->quota;
unsigned long sz_to_skip;
+ bool skip = false;
/* Skip previously charged regions */
if (quota->charge_target_from) {
if (t != quota->charge_target_from)
return true;
- if (r == damon_last_region(t)) {
- quota->charge_target_from = NULL;
- quota->charge_addr_from = 0;
- return true;
- }
if (quota->charge_addr_from &&
- r->ar.end <= quota->charge_addr_from)
- return true;
+ r->ar.end <= quota->charge_addr_from) {
+ skip = true;
+ goto out;
+ }
if (quota->charge_addr_from && r->ar.start <
quota->charge_addr_from) {
sz_to_skip = ALIGN_DOWN(quota->charge_addr_from -
r->ar.start, min_region_sz);
if (!sz_to_skip) {
- if (damon_sz_region(r) <= min_region_sz)
- return true;
+ if (damon_sz_region(r) <= min_region_sz) {
+ skip = true;
+ goto out;
+ }
sz_to_skip = min_region_sz;
}
damon_split_region_at(t, r, sz_to_skip);
- return true;
+ skip = true;
}
+ }
+out:
+ /*
+ * The last region may remain unapplied for extended period due to
+ * various regions (e.g., it is invalid or has been filtered out),
+ * preventing other regions from being applied (those preceding the last
+ * region and all regions with different targets). Therefore, when
+ * encountering a region that needs to be processed, reset
+ * charge_{target,addr}_from. If necessary, this parameters will be set
+ * to the correct value in damos_do_apply() due to quota is full.
+ */
+ if ((r == damon_last_region(t) && skip) || !skip) {
quota->charge_target_from = NULL;
quota->charge_addr_from = 0;
}
- return false;
+ return skip;
}
static void damos_update_stat(struct damos *s,
'''
>
> Btw, I think damos_skip_charged_region() may deserve a kunit test.
I agree that a kunit test would be valuable. While I am still getting
familiar with the kunit and it might take me a little time, I plan to
work on it.
Should the tests include these scenarios? Note that I use 1-based index
in here since it is easier to understand.
1. Baseline test:
- Parameters: charge_target_from = NULL, charge_addr_from = 0,
nr_target = 1, nr_region = 3.
- Expected: Returns false three times in a row.
charge_{target,addr}_from remains (NULL, 0).
2. Skip test:
- Parameters: charge_target_from = target[1], charge_addr_from =
region[2]->ar.end, nr_target = 1, nr_region = 3.
- Expected: Returns true, true (for region[1] and region[2]), then
false (for region[3]). charge_{target,addr}_from is reset to
(NULL, 0) after region[1].
3. Split test:
- Parameters: charge_target_from = target[1], charge_addr_from =
midpoint of region[2], nr_target = 1, nr_region = 3.
- Other: Ensure region[2] is large enough for the split to succeed.
- Expected: Returns true (region[1]), true (region[2] first half),
false (region[3] second half), false (region[4]). Total nr_region
becomes 4. charge_{target,addr}_from is reset to (NULL, 0) after
region[0].
4. Sashiko's edge case (Split failure on last region):
- Parameters: charge_target_from = target[1], charge_addr_from =
midpoint of region[3], nr_target = 1, nr_region = 3.
- Other: Ensure region[3] is small enough so that the split is
guaranteed to fail (sz_to_skip < min_region_sz).
- Expected: Returns true three times in a row. Crucially,
charge_{target,addr}_from is reset to (NULL, 0) on the third call,
preventing permanent state leakage.
5. Last region processing test:
- Parameters: charge_target_from = target[1], charge_addr_from =
region[2]->ar.end, nr_target = 1, nr_region = 3.
- Expected:
- Round 1: Returns true (region[1]), true (region[2]), false
(region[3], resets charge_{target,addr}_from because !skip).
- Round 2: Since the charge_{target,addr}_from is now (NULL, 0),
it should return false three times in a row, proving that
subsequent regions are not incorrectly blocked.
If there are any issues or missing edge cases in these scenarios, please
let me know!
>
> [1] https://lore.kernel.org/20260828090410.40AEA1F000E9@smtp.kernel.org
> [2] https://lore.kernel.org/20260828115047.332978-1-aethernet65535@gmail.com
Best regards,
Rui Yan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/damon: fix unconditionally skip last region
2026-08-29 8:34 ` Liew Rui Yan
@ 2026-08-29 16:15 ` SJ Park
2026-08-29 19:03 ` Liew Rui Yan
0 siblings, 1 reply; 6+ messages in thread
From: SJ Park @ 2026-08-29 16:15 UTC (permalink / raw)
To: Liew Rui Yan; +Cc: SJ Park, akpm, damon, linux-kernel, linux-mm, stable
On Sat, 29 Aug 2026 16:34:26 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> On Fri, 28 Aug 2026 11:29:09 -0700 SJ Park <sj@kernel.org> wrote:
>
> > On Fri, 28 Aug 2026 16:47:37 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> >
> > > Once quota set, the charge_{target,addr}_from unconditionally skips and
> > > resets at the last region of the tracked target, so the last region can
> > > be skipped even when it has not been processed.
> > >
> > > Example:
> > >
> > > 1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
> > > 2. Quota is configured to process only 50 bytes per window.
> > > 3. Window 1: Processes R1 (0-50). Quota is full. Cursor is saved
> > > at (Target, 50).
> >
> > Cursor means charge_{target,addr}_from, right? Let's explain that, or just
> > keep using the terms (charge_{target,addr}_from).
>
> Yes, thank you for pointing that out! I changed cursor to
> charge_{target,addr}_from now.
>
> >
> > > 4. Window 2: Skips R1 (0-50). Processes R1 (50-100). Quota is
> > > full. Cursor is saved at (Target, 100), which is exactly the
> > > start of R2.
> > > 5. Window 3: The loop reaches R2. Because R2 is
> > > damon_last_region(t), the old code unconditionally returns true,
> > > skipping R2 entirely and resetting the cursor.
> > >
> > > Result: R2 is permanently skipped even though it has never been
> > > processed.
> >
> > Let's make example simpler by setting R1 (0-50 bytes) and R2 (50-100 bytes) or
> > quota size 100 bytes per window.
>
> This is the updated example:
>
> '''
> Example:
>
> 1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
> 2. Quota is configured to process only 100 bytes per window.
> 3. Window 1: Processes R1 (0-100). Quota is full. charge_{target,
> addr}_from is saved at (Target, 100).
> 4. Window 2: The loop reaches R2. Because R2 is
> damon_last_region(t), the old code unconditionally returns true,
> skipping R2 entirely and resetting the charge_{target,addr}_from.
>
> Result: R2 is permanently skipped even though it has never been
> processed.
> '''
>
> >
> > Also, it continues being skipped only in a corner case that the region
> > addresses and the access patterns are kept. So the user impact is mild. Let's
> > clarify that to not make users unnecessarily afraid.
>
> I will add this clarification in the next revision:
>
> '''
> However, it is important to note that this is a very minor issue. This
> is because it is triggered only when the previous window saved/kept
> charge_{target,addr}_from, and in the next window, all regions except
> the last region were skipped by damos_skip_charged_region().
> '''
>
> >
> > >
> > > Fix this by only skipping the last region after it has been applied.
> > >
> > > Fixes: 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
> > > Cc: <stable@vger.kernel.org> # v5.16.x
> > > Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> > > ---
> > >
> > > Changes from RFC v1:
> > > - Minimal fix, only fixes the issue where the last-region is skipped.
> > > - Add an example to the commit message to demonstrate that this error
> > > occurs very rarely.
> > > - RFC v1: https://lore.kernel.org/damon/20260825124616.5129-1-aethernet65535@gmail.com
> > >
> > > ---
> > > mm/damon/core.c | 13 +++++++------
> > > 1 file changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > index 644daf5a1656..21dc6b086c42 100644
> > > --- a/mm/damon/core.c
> > > +++ b/mm/damon/core.c
> > > @@ -2347,14 +2347,15 @@ static bool damos_skip_charged_region(struct damon_target *t,
> > > if (quota->charge_target_from) {
> > > if (t != quota->charge_target_from)
> > > return true;
> > > - if (r == damon_last_region(t)) {
> > > - quota->charge_target_from = NULL;
> > > - quota->charge_addr_from = 0;
> > > - return true;
> > > - }
> > > if (quota->charge_addr_from &&
> > > - r->ar.end <= quota->charge_addr_from)
> > > + r->ar.end <= quota->charge_addr_from) {
> > > + if (r->ar.end == quota->charge_addr_from ||
> > > + damon_is_last_region(r, t)) {
> > > + quota->charge_target_from = NULL;
> > > + quota->charge_addr_from = 0;
> > > + }
> > > return true;
> > > + }
> > >
> > > if (quota->charge_addr_from && r->ar.start <
> > > quota->charge_addr_from) {
> >
> > As Sashiko pointed out, this doesn't work if the the last region's start
> > address is smaller than charge_addr_from and the end address is larger than
> > charge_addr_from, but the size to skip (charge_addr_from - r->ar.start) is
> > smaller than min_region_sz.
> >
> > As you replied to Sashiko, let's do the last region handling in every case.
> > While doing that, let's do the charge_{target,addr}_from reset in only one
> > place, like below.
> >
> > '''
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2688,36 +2688,40 @@ static bool damos_skip_charged_region(struct damon_target *t,
> > {
> > struct damos_quota *quota = &s->quota;
> > unsigned long sz_to_skip;
> > + bool skip = false;
> >
> > /* Skip previously charged regions */
> > if (quota->charge_target_from) {
> > if (t != quota->charge_target_from)
> > return true;
> > - if (r == damon_last_region(t)) {
> > - quota->charge_target_from = NULL;
> > - quota->charge_addr_from = 0;
> > - return true;
> > - }
> > if (quota->charge_addr_from &&
> > - r->ar.end <= quota->charge_addr_from)
> > - return true;
> > + r->ar.end <= quota->charge_addr_from) {
> > + skip = true;
> > + goto out;
> > + }
> >
> > if (quota->charge_addr_from && r->ar.start <
> > quota->charge_addr_from) {
> > sz_to_skip = ALIGN_DOWN(quota->charge_addr_from -
> > r->ar.start, min_region_sz);
> > if (!sz_to_skip) {
> > - if (damon_sz_region(r) <= min_region_sz)
> > - return true;
> > + if (damon_sz_region(r) <= min_region_sz) {
> > + skip = true;
> > + goto out;
> > + }
> > sz_to_skip = min_region_sz;
> > }
> > damon_split_region_at(t, r, sz_to_skip);
> > - return true;
> > + skip = true;
> > }
> > + }
> > +out:
> > + if (r == damon_last_region(t)) {
> > quota->charge_target_from = NULL;
> > quota->charge_addr_from = 0;
> > + return true;
> > }
> > - return false;
> > + return skip;
> > }
> >
> > static void damos_update_stat(struct damos *s,
> > '''
>
> I noticed a potential subtle issue in the suggested fix above:
>
> '''
> +out:
> + if (r == damon_last_region(t)) {
> quota->charge_target_from = NULL;
> quota->charge_addr_from = 0;
> + return true;
> }
> '''
>
> If 'skip' is false (region should be processed), but it happens to be
> the last region, the condition 'if (r == damon_last_region(t))' would
> still be met. This would cause it to reset the state and 'return true'
> (skip it), which inadvertently re-introduces the original bug we are
> trying to fix.
Ah, good catch. The 'return true' is a wrong copy-pasta. Let's drop the line.
>
> To ensure the reset logic is centralized and correct, I refined the fix
> as follows. The comment is intended to help you and other reviewers
> quickly understand the rationale behind the compound condition. I will
> remove this comment in the next revision.
>
> '''
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 644daf5a1656..82c5aed8a417 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2342,36 +2342,48 @@ static bool damos_skip_charged_region(struct damon_target *t,
> {
> struct damos_quota *quota = &s->quota;
> unsigned long sz_to_skip;
> + bool skip = false;
>
> /* Skip previously charged regions */
> if (quota->charge_target_from) {
> if (t != quota->charge_target_from)
> return true;
> - if (r == damon_last_region(t)) {
> - quota->charge_target_from = NULL;
> - quota->charge_addr_from = 0;
> - return true;
> - }
> if (quota->charge_addr_from &&
> - r->ar.end <= quota->charge_addr_from)
> - return true;
> + r->ar.end <= quota->charge_addr_from) {
> + skip = true;
> + goto out;
> + }
>
> if (quota->charge_addr_from && r->ar.start <
> quota->charge_addr_from) {
> sz_to_skip = ALIGN_DOWN(quota->charge_addr_from -
> r->ar.start, min_region_sz);
> if (!sz_to_skip) {
> - if (damon_sz_region(r) <= min_region_sz)
> - return true;
> + if (damon_sz_region(r) <= min_region_sz) {
> + skip = true;
> + goto out;
> + }
> sz_to_skip = min_region_sz;
> }
> damon_split_region_at(t, r, sz_to_skip);
> - return true;
> + skip = true;
> }
> + }
> +out:
> + /*
> + * The last region may remain unapplied for extended period due to
> + * various regions (e.g., it is invalid or has been filtered out),
> + * preventing other regions from being applied (those preceding the last
> + * region and all regions with different targets). Therefore, when
> + * encountering a region that needs to be processed, reset
> + * charge_{target,addr}_from. If necessary, this parameters will be set
> + * to the correct value in damos_do_apply() due to quota is full.
> + */
Looks too verbose to me. Let's drop this.
> + if ((r == damon_last_region(t) && skip) || !skip) {
Why this becomes this complex? We should reset charge_{target,addr}_from if it
is the last region, always. Am I missing something?
> quota->charge_target_from = NULL;
> quota->charge_addr_from = 0;
> }
> - return false;
> + return skip;
> }
>
> static void damos_update_stat(struct damos *s,
> '''
>
> >
> > Btw, I think damos_skip_charged_region() may deserve a kunit test.
>
> I agree that a kunit test would be valuable. While I am still getting
> familiar with the kunit and it might take me a little time, I plan to
> work on it.
Nice. Looking forward to your patch.
>
> Should the tests include these scenarios?
Let's not add new topics. We can discuss this on your kunit patch. Please
ensure it covers at least the corner case this patch is trying to fix.
Thanks,
SJ
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/damon: fix unconditionally skip last region
2026-08-29 16:15 ` SJ Park
@ 2026-08-29 19:03 ` Liew Rui Yan
2026-08-29 19:06 ` SJ Park
0 siblings, 1 reply; 6+ messages in thread
From: Liew Rui Yan @ 2026-08-29 19:03 UTC (permalink / raw)
To: sj; +Cc: aethernet65535, akpm, damon, linux-kernel, linux-mm, stable
On Sat, 29 Aug 2026 09:15:08 -0700 SJ Park <sj@kernel.org> wrote:
> On Sat, 29 Aug 2026 16:34:26 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
>
> > On Fri, 28 Aug 2026 11:29:09 -0700 SJ Park <sj@kernel.org> wrote:
> >
> > > As you replied to Sashiko, let's do the last region handling in every case.
> > > While doing that, let's do the charge_{target,addr}_from reset in only one
> > > place, like below.
> > >
> > > '''
> > > --- a/mm/damon/core.c
> > > +++ b/mm/damon/core.c
> > > @@ -2688,36 +2688,40 @@ static bool damos_skip_charged_region(struct damon_target *t,
> > > {
> > > struct damos_quota *quota = &s->quota;
> > > unsigned long sz_to_skip;
> > > + bool skip = false;
> > >
> > > /* Skip previously charged regions */
> > > if (quota->charge_target_from) {
> > > if (t != quota->charge_target_from)
> > > return true;
> > > - if (r == damon_last_region(t)) {
> > > - quota->charge_target_from = NULL;
> > > - quota->charge_addr_from = 0;
> > > - return true;
> > > - }
> > > if (quota->charge_addr_from &&
> > > - r->ar.end <= quota->charge_addr_from)
> > > - return true;
> > > + r->ar.end <= quota->charge_addr_from) {
> > > + skip = true;
> > > + goto out;
> > > + }
> > >
> > > if (quota->charge_addr_from && r->ar.start <
> > > quota->charge_addr_from) {
> > > sz_to_skip = ALIGN_DOWN(quota->charge_addr_from -
> > > r->ar.start, min_region_sz);
> > > if (!sz_to_skip) {
> > > - if (damon_sz_region(r) <= min_region_sz)
> > > - return true;
> > > + if (damon_sz_region(r) <= min_region_sz) {
> > > + skip = true;
> > > + goto out;
> > > + }
> > > sz_to_skip = min_region_sz;
> > > }
> > > damon_split_region_at(t, r, sz_to_skip);
> > > - return true;
> > > + skip = true;
> > > }
> > > + }
> > > +out:
> > > + if (r == damon_last_region(t)) {
> > > quota->charge_target_from = NULL;
> > > quota->charge_addr_from = 0;
> > > + return true;
> > > }
> > > - return false;
> > > + return skip;
> > > }
> > >
> > > static void damos_update_stat(struct damos *s,
> > > '''
> >
> > I noticed a potential subtle issue in the suggested fix above:
> >
> > '''
> > +out:
> > + if (r == damon_last_region(t)) {
> > quota->charge_target_from = NULL;
> > quota->charge_addr_from = 0;
> > + return true;
> > }
> > '''
> >
> > If 'skip' is false (region should be processed), but it happens to be
> > the last region, the condition 'if (r == damon_last_region(t))' would
> > still be met. This would cause it to reset the state and 'return true'
> > (skip it), which inadvertently re-introduces the original bug we are
> > trying to fix.
>
> Ah, good catch. The 'return true' is a wrong copy-pasta. Let's drop the line.
>
> >
> > To ensure the reset logic is centralized and correct, I refined the fix
> > as follows. The comment is intended to help you and other reviewers
> > quickly understand the rationale behind the compound condition. I will
> > remove this comment in the next revision.
> >
> > '''
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 644daf5a1656..82c5aed8a417 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2342,36 +2342,48 @@ static bool damos_skip_charged_region(struct damon_target *t,
> > {
> > struct damos_quota *quota = &s->quota;
> > unsigned long sz_to_skip;
> > + bool skip = false;
> >
> > /* Skip previously charged regions */
> > if (quota->charge_target_from) {
> > if (t != quota->charge_target_from)
> > return true;
> > - if (r == damon_last_region(t)) {
> > - quota->charge_target_from = NULL;
> > - quota->charge_addr_from = 0;
> > - return true;
> > - }
> > if (quota->charge_addr_from &&
> > - r->ar.end <= quota->charge_addr_from)
> > - return true;
> > + r->ar.end <= quota->charge_addr_from) {
> > + skip = true;
> > + goto out;
> > + }
> >
> > if (quota->charge_addr_from && r->ar.start <
> > quota->charge_addr_from) {
> > sz_to_skip = ALIGN_DOWN(quota->charge_addr_from -
> > r->ar.start, min_region_sz);
> > if (!sz_to_skip) {
> > - if (damon_sz_region(r) <= min_region_sz)
> > - return true;
> > + if (damon_sz_region(r) <= min_region_sz) {
> > + skip = true;
> > + goto out;
> > + }
> > sz_to_skip = min_region_sz;
> > }
> > damon_split_region_at(t, r, sz_to_skip);
> > - return true;
> > + skip = true;
> > }
> > + }
> > +out:
> > + /*
> > + * The last region may remain unapplied for extended period due to
> > + * various regions (e.g., it is invalid or has been filtered out),
> > + * preventing other regions from being applied (those preceding the last
> > + * region and all regions with different targets). Therefore, when
> > + * encountering a region that needs to be processed, reset
> > + * charge_{target,addr}_from. If necessary, this parameters will be set
> > + * to the correct value in damos_do_apply() due to quota is full.
> > + */
>
> Looks too verbose to me. Let's drop this.
>
> > + if ((r == damon_last_region(t) && skip) || !skip) {
>
> Why this becomes this complex? We should reset charge_{target,addr}_from if it
> is the last region, always. Am I missing something?
Thank you for pointing this out! As long as it is the last region, we
should reset.
>
> > quota->charge_target_from = NULL;
> > quota->charge_addr_from = 0;
> > }
> > - return false;
> > + return skip;
> > }
> >
> > static void damos_update_stat(struct damos *s,
> > '''
>
> >
> > >
> > > Btw, I think damos_skip_charged_region() may deserve a kunit test.
> >
> > I agree that a kunit test would be valuable. While I am still getting
> > familiar with the kunit and it might take me a little time, I plan to
> > work on it.
>
> Nice. Looking forward to your patch.
>
> >
> > Should the tests include these scenarios?
>
> Let's not add new topics. We can discuss this on your kunit patch. Please
> ensure it covers at least the corner case this patch is trying to fix.
Okay, I will ensure that. Should the test be sent along with this
patch (as part of the same series), or should they be sent separately?
Best regards,
Rui Yan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/damon: fix unconditionally skip last region
2026-08-29 19:03 ` Liew Rui Yan
@ 2026-08-29 19:06 ` SJ Park
0 siblings, 0 replies; 6+ messages in thread
From: SJ Park @ 2026-08-29 19:06 UTC (permalink / raw)
To: Liew Rui Yan; +Cc: SJ Park, akpm, damon, linux-kernel, linux-mm, stable
On Sun, 30 Aug 2026 03:03:27 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> On Sat, 29 Aug 2026 09:15:08 -0700 SJ Park <sj@kernel.org> wrote:
>
[...]
> > Let's not add new topics. We can discuss this on your kunit patch. Please
> > ensure it covers at least the corner case this patch is trying to fix.
>
> Okay, I will ensure that. Should the test be sent along with this
> patch (as part of the same series), or should they be sent separately?
Please do separately.
Thanks,
SJ
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-08-29 19:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-28 8:47 [PATCH] mm/damon: fix unconditionally skip last region Liew Rui Yan
2026-08-28 18:29 ` SJ Park
2026-08-29 8:34 ` Liew Rui Yan
2026-08-29 16:15 ` SJ Park
2026-08-29 19:03 ` Liew Rui Yan
2026-08-29 19:06 ` SJ Park
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®