Re: [PATCH] mm/damon: fix unconditionally skip last region

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch introduces a logic error that can permanently leak `quota->charge_target_from` if a target's last region straddles `charge_addr_from`. This causes DAMOS to permanently skip the target and all subsequent targets, effectively disabling the scheme.
--

commit b16b899bcf256d33234a0a128af51071c5723a6e
Author: Liew Rui Yan <aethernet65535@gmail.com>

mm/damon: fix unconditionally skip last region

This commit modifies damos_skip_charged_region() to only skip the last region
after it has been applied, rather than unconditionally skipping it and
resetting the cursor. It achieves this by moving the last region reset logic
into the condition checking if the region has already been fully charged.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 644daf5a16560..21dc6b086c42f 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) {

[Severity: High]
If a target's last region straddles quota->charge_addr_from (meaning
r->ar.start < quota->charge_addr_from but r->ar.end > quota->charge_addr_from),
does nesting the damon_is_last_region() check strictly inside the
r->ar.end <= quota->charge_addr_from condition cause it to bypass the reset
logic entirely?

Because the unconditional r == damon_last_region(t) reset at the start of
the function was removed, the code now proceeds to the next block where it
attempts to skip the prefix:

    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;
            sz_to_skip = min_region_sz;
        }
        damon_split_region_at(t, r, sz_to_skip);
        return true;
    }

If the prefix length is smaller than min_region_sz and
damon_sz_region(r) <= min_region_sz, or if the subsequent
damon_split_region_at() fails due to -ENOMEM, the function returns true early
without splitting and without clearing quota->charge_target_from.

Does this permanently leak quota->charge_target_from?

If the function returns true early without clearing quota->charge_target_from,
the region loop in damos_apply_target() would naturally terminate since it is
the last region, leaving the cursor permanently pointing to the current target.

In subsequent time windows, could this leaked target repeatedly trigger the
same early return, causing all other targets to be skipped permanently
because t != quota->charge_target_from?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828084737.290024-1-aethernet65535@gmail.com?part=1