Re: [PATCH v2 1/3] mm/damon/core: prevent size quota overflow in the temporal goal tuner

[Donggeun Yoo]

Thank you for the detailed review.

On Sun, 20 Sep 2026 03:37:13 -0700 SJ Park <sj@xxxxxxxxxx> wrote:
> Let's break commit message lines with 72 columns limit.

Done in v3, here and in 2/3.

> Why 256 MiB, not 429,496 bytes?

They are two different numbers.  429496 is where the multiply starts
to wrap, and every size above it is wrong.  256 MiB is where the
wrapped value lands on exactly zero: 256 MiB * 10000 is 625 * 2^32, so
on 32-bit the product wraps around 625 times and ends at zero.

> So, the way to work around is updating the size quota to smaller value,
> correct?

Correct.  v3 says so.

> But why a sane user would set such huge number?

They would not, which is why v3 no longer argues the 64-bit case in its
own paragraph.  The threshold is now stated once, as part of what it
takes to reach the bug: above ULONG_MAX / 10000, which is 429496 bytes
on 32-bit and 1844674407370955 on 64-bit.

> Because this patch Cc stable@, let's make super clear about the user
> impact [...]

v3:

  Triggering this needs a scheme with a quota goal, the temporal goal
  tuner, and a size quota above ULONG_MAX / 10000 -- 429496 bytes on
  32-bit, 1844674407370955 on 64-bit -- so it is unlikely to be hit on
  a tested setup.  Nothing is corrupted and nothing leaks.  The scheme
  makes no progress for as long as the goal is unachieved, which is
  easy to notice, and writing a smaller size quota restores it.

> > Bound the conversion, so a size quota it cannot represent falls to the
> > ULONG_MAX the function already writes for a scheme with no size quota.
>
> I don't understand the above sentence.  Is the grammar correct?

You are right that it is hard to read.  The sentence was too long and
tried to say two things at once.  v3:

  Bound the multiply.  A size quota too large to convert now takes the
  same ULONG_MAX branch as a scheme with no size quota, so the
  effective quota becomes ULONG_MAX / 10000 instead of a wrapped value.

> > Widening esz_bp instead would reach the consist tuner [...]
>
> I don't quite understand above.  could you please elaborate?

Sorry, that paragraph was not clear.  It was meant to explain why I did
not simply widen the type.

The other way to fix the overflow is to make esz_bp wider than unsigned
long, u64 for example.  But esz_bp is not used only here.  The consist
tuner keeps its own value in the same field, and
damos_goal_tune_esz_bp_consist() passes it to
damon_feed_loop_next_input(), which takes unsigned long and returns
unsigned long.  So widening esz_bp means widening that function too,
and the consist tuner would change for a problem it does not have.
Bounding the multiply is one line, and only the temporal tuner is
touched.

I dropped the paragraph in v3.  It argues against a fix nobody
proposed, so it only makes the changelog harder to read.

Thanks,
Donggeun