From: Liew Rui Yan <aethernet65535@gmail.com>
To: sj@kernel.org
Cc: aethernet65535@gmail.com, akpm@linux-foundation.org,
damon@lists.linux.dev, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, stable@vger.kernel.org
Subject: Re: [PATCH] mm/damon: fix unconditionally skip last region
Date: Sat, 29 Aug 2026 16:34:26 +0800 [thread overview]
Message-ID: <20260829083744.73299-1-aethernet65535@gmail.com> (raw)
In-Reply-To: <20260828182910.70304-1-sj@kernel.org>
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
prev parent reply other threads:[~2026-08-29 8:37 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260829083744.73299-1-aethernet65535@gmail.com \
--to=aethernet65535@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=damon@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=sj@kernel.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox